Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add device posture test driver and web platform tests #4

Open
wants to merge 25 commits into
base: DevicePosture
Choose a base branch
from

Conversation

JuhaVainio
Copy link
Owner

No description provided.

Copy link
Collaborator

@rakuco rakuco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are programming issues in the converted tests that must be fixed. I've added inline comments pointing them out.

You should also use EventWatcher everywhere but the one test that wants to exercise addEventListener behavior.

Missing newline from tools/wptrunner/wptrunner/executors/protocol.py.

Remove comma from resources/testdriver.js.

Unused commented code in device-posture/device-posture.https.html.
Removed the requirement from all tests to set up cleanup function.

All tests are now wrapped around device_posture_test call. This enables
that adding of cleanup function for each test is done in one location.
Two tests where similar, so other was delete.
EventWatcher promise was waited with mixed style. Code was fixed to use
only one style of waiting. After change EventWatcher eventTypes
parameter value needed to change also to able to run test correctly.
@JuhaVainio JuhaVainio changed the title Add device posture tests Add device posture and viewport tests Mar 5, 2024
@JuhaVainio JuhaVainio changed the title Add device posture and viewport tests Add device posture test driver and web platform tests Apr 5, 2024
Copy link
Collaborator

@rakuco rakuco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you were waiting for my review, but I've added some comments about things that need to be fixed in addition to anything else that was already on your list.

Event type was changed from 'onchange' to 'change'. This led change in
waiting for event. Which solved the problem that 'continuous' posture
had to be the first posture.
Removed because other tests already had same code and read posture
through navigator.devicePosture.type.
Copy link
Collaborator

@rakuco rakuco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that's just occurred to me while working on the content_shell CL upstream: all these tests that call set_device_posture() need a

t.add_cleanup(async () => {
  await test_driver.clear_device_posture();
});

call to make sure nothing leaks between tests.

JuhaVainio pushed a commit that referenced this pull request Jun 25, 2024
Since @page border box layout objects aren't in the the layout tree, any
code that wants to walk up the tree to find the containing block will be
in for a surprise.

This would happen if percentage-based @page padding was used [1].
Recomputing padding during painting when we have already done it during
layout is rather pointless anyway. Read it out directly from the
fragment.

[1] #1 blink::LayoutBox::ContainingBlockLogicalWidthForContent()
    #2 blink::LayoutBoxModelObject::ComputedCSSPadding()
    #3 blink::LayoutBoxModelObject::PaddingTop()
    #4 blink::LayoutBoxModelObject::PaddingOutsets()
    web-platform-tests#5 blink::BoxPainterBase::PaintFillLayer()
    web-platform-tests#6 blink::BoxPainterBase::PaintFillLayers()
    web-platform-tests#7 blink::BoxFragmentPainter::PaintBackground()

Bug: 40286153
Change-Id: I1e6e92c2ce1d81aab2673ec9a877eac455534102
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5526469
Commit-Queue: Morten Stenshorne <[email protected]>
Reviewed-by: Xianzhu Wang <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1300711}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants