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

Fix various active document tests #22031

Merged
merged 3 commits into from
Mar 3, 2020

Conversation

TimothyGu
Copy link
Member

Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Nasty... but makes sense :)

Copy link
Member

@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.

Rubber-stamping this PR since you definitely know more about this than I do :-)

Do note that I mostly copied this test from payment-request/rejects_if_not_active-manual.https.html, so it probably needs to be adjusted as well.

@stephenmcgruer
Copy link
Contributor

cc @bzbarsky as the author of promise_rejects_dom for the comment on line 33; mostly just an FYI, but perhaps you have some insights too.

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 2, 2020

I'm not sure I'm following the comment, actually. promise_rejects_dom just calls promise.then on the given promise; it does not use await.

The real issue here, I would expect, is the await call on the return value of promise_rejects_dom. Since it calls promise.then, it gets a new promise from inside the navigated-away-from thing, and returns it. When the caller then awaits that promise, it ends up hitting the situation described in whatwg/html#5319

In terms of whether we can remove this papercut... We could replace this bit in promise_rejects_dom (and the other promise_rejects_* methods):

return promise.then(...);

with:

return Promise.prototype.then.call(promise, ...);

with a comment explaining that this is an attempt to prevent returning promises from non-active contexts, because awaiting them does not work. That said, I haven't looked at the subclassing stuff in Promise.prototype.then carefully enough recently enough to see which global it would actually create the new Promise, so even that might now work. @TimothyGu any idea offhand?

If we can't remove the papercut, we should at least document the behavior clearly, and this comment should clearly say that it's the awaiting in the test, not in the harness, that is a problem...

@TimothyGu
Copy link
Member Author

@bzbarsky Hmm, I guess I didn't look as carefully as I should have. In either case, any comment with whatwg/html#5319 would be appreciated.

Regarding the suggestion to use Promise.prototype.then.call(promise, ...), sadly that wouldn't quite work since then uses the constructor property to figure out which Promise to use for construction. But I think I've made something that should work, or works on Chrome at least (since I couldn't see how to enable the wakelock API in Firefox). PTAL.

Use DOMException constructor from the proper realm. This is a regression
after b7f2dd3.
Use DOMException constructor from the proper realm. This is a regression
after b7f2dd3.
@TimothyGu
Copy link
Member Author

@rakuco Thanks for the pointer. I've adjusted it as well.

@TimothyGu TimothyGu changed the title wakelock: Fix active document test Fix various active document tests Mar 2, 2020
@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 3, 2020

any comment with whatwg/html#5319 would be appreciated

Yeah, still working on that. Need to page a lot of context in to comment intelligently there. ;)

resources/testharness.js Show resolved Hide resolved
@TimothyGu TimothyGu merged commit 01a3d65 into web-platform-tests:master Mar 3, 2020
@TimothyGu TimothyGu deleted the wakelock branch March 3, 2020 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants