Closed
Bug 1437680
Opened 7 years ago
Closed 7 years ago
Avoid assertions in dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html with the patch for bug 1193394
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
| Tracking | Status | |
|---|---|---|
| firefox60 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=701a987d4fcddd24a4f67f08df6f9ee2b9b81f3a&selectedJob=161715873
GECKO(2914) | Assertion failure: !presContext->HasPendingMediaQueryUpdates() (Someone forgot to update media queries?), at /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:110
I thought initially it happened for the same reason as bug 1436625, but it seems not.
|
Assignee | |
Updated•7 years ago
|
Flags: needinfo?(hikezoe)
|
|
Assignee | |
Comment 1•7 years ago
|
||
This is pretty unfortunate. The test case changes dppx inside the callback for MediaQueryListListener, it's processed in DoFlushPendingNotifications() [1].
The right thing we should do is process the callbacks for MediaQueryListListener between dispatching scroll events and animation events (bug 1437688).
That's said, we might want to do a workaround here.
[1] https://hg.mozilla.org/mozilla-central/file/6d8f470b2579/layout/base/PresShell.cpp#l4173
Depends on: 1437688
Flags: needinfo?(hikezoe)
|
|
Assignee | |
Comment 2•7 years ago
|
||
I'd suggest to do workaround here in this bug. The test does not need to change media feature inside the callbacks for media query list events, so we can add setTimeout() there. (I am pretty sure the assertion for pending media feature change is the right thing to do)
Here is a try with the setTimeout call.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=538331f2e7bb20bf22f2020fa7743965533f7b64
Summary: Fix assertions in dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html with the patch for bug 1193394 → Avoid assertions in dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html with the patch for bug 1193394
|
|
Assignee | |
Comment 3•7 years ago
|
||
And a try based on the patch for bug 1193394.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f5aa2c7919e463d0dcdcaaea36cf16d1e39c407
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
| Comment hidden (mozreview-request) |
|
||
Comment 5•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8950425 [details]
Bug 1437680 - Don't change media feature in the callback for MediaQueryListEvent.
https://reviewboard.mozilla.org/r/219662/#review225424
Thanks! Seems okay overall, but please be sure to add a comment.
::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:86
(Diff revision 1)
> +const waitForMediaQueryListEvent = (mediaQueryList) => {
> + return new Promise(resolve => {
> + mediaQueryList.addListener(function listener() {
> + ok(true, "MediaQueryList's listener invoked for " + mediaQueryList.media);
> + mediaQueryList.removeListener(listener);
> + setTimeout(resolve, 0);
Please add a comment here explaining why this is needed for future readers.
::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:262
(Diff revision 1)
> setOverrideDPPX(dppx);
> },
> "test OverrideDPPX with MediaQueryList and fullZoom": (done) => {
> assertValuesAreInitial();
>
> let overridden = false;
This line seems unused now, please remove it if so.
Attachment #8950425 -
Flags: review?(jryans) → review+
|
|
Assignee | |
Comment 6•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Comment on attachment 8950425 [details]
> Bug 1437680 - Don't change media feature in the callback for
> MediaQueryListEvent.
>
> https://reviewboard.mozilla.org/r/219662/#review225424
>
> Thanks! Seems okay overall, but please be sure to add a comment.
>
> ::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:86
> (Diff revision 1)
> > +const waitForMediaQueryListEvent = (mediaQueryList) => {
> > + return new Promise(resolve => {
> > + mediaQueryList.addListener(function listener() {
> > + ok(true, "MediaQueryList's listener invoked for " + mediaQueryList.media);
> > + mediaQueryList.removeListener(listener);
> > + setTimeout(resolve, 0);
>
> Please add a comment here explaining why this is needed for future readers.
Indeed. I added a comment there locally.
> ::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:262
> (Diff revision 1)
> > setOverrideDPPX(dppx);
> > },
> > "test OverrideDPPX with MediaQueryList and fullZoom": (done) => {
> > assertValuesAreInitial();
> >
> > let overridden = false;
>
> This line seems unused now, please remove it if so.
Good catch!
Thanks for the review!
| Comment hidden (mozreview-request) |
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04a85bd02d1e
Don't change media feature in the callback for MediaQueryListEvent. r=jryans
|
||
Comment 9•7 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•