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

WebKit export of https://bugs.webkit.org/show_bug.cgi?id=181239 #8894

Merged

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Jan 4, 2018

Implement https://fetch.spec.whatwg.org/#main-fetch default referrer policy setting

@ghost
Copy link

ghost commented Jan 4, 2018

Build BROKEN

Started: 2018-01-04 08:08:55
Finished: 2018-01-04 08:19:56

Failing Jobs

  • lint
  • safari:10.0

View more information about this build on:

@youennf
Copy link
Contributor Author

youennf commented Feb 1, 2018

The lint issue is about the use of window.internals.
In this case, if window.internals is there, it will lower non mandatory security resource loading checks.
This test is indeed doing some HTTP loads from an HTTPS web page.

@youennf
Copy link
Contributor Author

youennf commented Feb 1, 2018

@foolip, @gsnedders, is it fine to go on with this PR as is.

@foolip
Copy link
Member

foolip commented Feb 1, 2018

@youennf, do you mean that you think it should be merged as-is, with a lint exception added, or not merged?

I suppose that to follow the model of Chromium, Gecko and Servo exports we shouldn't be doing any reviewing here and instead do that in follow-ups. But a few things to note:

  • Is it possible to integrate wpt lint into WebKit bot steps so that failing lints are noticed during WebKit review?
  • If this is imported into Chromium, it'll break there, because setAllowDisplayOfInsecureContent doesn't exist, but window.internals.settings does.

Not sure how best to proceed. Will the test simply fail again without the setAllowDisplayOfInsecureContent in WebKit? Does the test go beyond what any spec requires, or why does optional behavior need to be toggled on here?

On internals and eventSender generally, I think it would be wise to avoid that and try to put the required APIs into testdriver.js, although it is more work, I realize that. @kereliuk FYI that this is a test automation challenge, maybe a new one.

@leonhsl who created this test in https://chromium-review.googlesource.com/c/chromium/src/+/567963 for opinions.

@@ -7,12 +7,22 @@
<script>
var worker = 'resources/fetch-event-test-worker.js';

if (window.internals && window.internals.settings)
internals.settings.setAllowDisplayOfInsecureContent(true);
Copy link
Member

@wanderview wanderview Feb 1, 2018

Choose a reason for hiding this comment

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

@youennf What is this? Generally this sort of thing is managed in the harness integration with the browser vendor's test suite. We don't usually put non-standard stuff like this in the WPT to my knowledge.

Edit: Sorry, I didn't see @foolip's comment above as I was working through my email.

Copy link
Member

Choose a reason for hiding this comment

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

JFYI for Blink we're somehow able to use testharnessreport.js to add internal settings. Can WebKit have a similar setup? For reference here's the change to the file in the patch foolip linked to above:
https://chromium-review.googlesource.com/c/chromium/src/+/567963/3/third_party/WebKit/LayoutTests/resources/testharnessreport.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @matto.
We are also using testharnessreport.js for these kind of setup but not on a per test basis right now.
I guess this might be fine as a temporary solution.
Adding this kind of configuration within the test itself using something like window.test_driver might be a better approach as other browser teams will more easily understand what needs to be done.

@youennf youennf force-pushed the wpt-export-for-webkit-181239 branch from f1417b9 to 207759d Compare February 1, 2018 16:43
@ghost
Copy link

ghost commented Feb 1, 2018

Build PASSED

Started: 2018-02-01 17:06:28
Finished: 2018-02-01 17:11:46

View more information about this build on:

@youennf
Copy link
Contributor Author

youennf commented Feb 1, 2018

Is it possible to integrate wpt lint into WebKit bot steps so that failing lints are noticed during WebKit review?

This is indeed a good idea and is on the table.
Hopefully, there will be some progress on our side soon.

@foolip
Copy link
Member

foolip commented Feb 1, 2018

What should we do to get this merged? Just cut out the window.internals bits? Or wait for a follow-up change in WebKit and that export PR, so that we can merged both PRs at once?

@youennf
Copy link
Contributor Author

youennf commented Feb 4, 2018

For now, I removed the window.internals bits.
I will look at how using testdriver.js to relax the security checks for this particular test.

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the tests as that's been done in WebKit, but approving in case that's all that's blocking merging this.

@gsnedders
Copy link
Member

Is there any reason this hasn't landed?

@youennf youennf merged commit 44e61df into web-platform-tests:master Sep 7, 2018
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