Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Turn Server-Timing experimental feature on #621

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cvazac
Copy link

@cvazac cvazac commented Oct 29, 2018

I'm attempting to improve the server-timing results on Safari.

image

I will remove this if/when Safari enables this by default.

@cvazac
Copy link
Author

cvazac commented Oct 30, 2018

@jugglinmike can you PTAL?

@jugglinmike
Copy link
Collaborator

Thanks for the patch, @cvazac!

In order to help others reproduce the results they find on wpt.fyi, we should document this step more widely. Can you patch WPT's instructions on running the tests in Safari (as published at https://web-platform-tests.org/running-tests/safari.html)?

As for the change itself, this may not the best place to insert the new setting. The name "safari-disable-popup-blocker.sh` doesn't describe a change like this. Also, note that this script only runs for the stable release of Safari. Should this configuration also apply to Safari Technology Preview?

I can help work this in to another script, though it'll be a few days before I have the time.

@cvazac
Copy link
Author

cvazac commented Nov 5, 2018

Thx for the review @jugglinmike!

I made a new file for the experimental flag code - but I couldn't figure out how to test it locally.

Yes, this should run in TP as well.

PR for safari.md documentation change.

@jugglinmike
Copy link
Collaborator

Yes, this should run in TP as well.

Excuse me, I forgot that we have a precedent for this kind of thing.

We only apply Chrome's --enable-experimental-web-platform-features when testing the browser's experimental release. That makes the results for the "stable" release representative of a typical user's experience using the stock browser, and it also gives implementers visibility into the cutting edge. We should apply the same policy to Safari and Server Timing.

In addition to updating the script itself, there's another small change we'll need to make to support STP. We never know when the next release will be published, so in order to always test the latest version, we install the browser every time we attempt to collect results. In order for the new script to modify the freshly-installed browser, we'll have to invoke it after that "build step" in the Buildbot configuration file.

@jugglinmike
Copy link
Collaborator

Also cc @foolip, who has been automating Safari in another environment.

@foolip
Copy link
Member

foolip commented Nov 7, 2018

It would be great to get this into tools/wpt/run.py:
https://github.com/web-platform-tests/wpt/blob/bfc7ce508b754e84e95830c2186f1fa8eb9f179e/tools/wpt/run.py#L378-L385

See Chrome.setup_kwargs and Firefox.setup_kwargs in that file for precedent. If we put it into wpt itself, it'll also work for the Safari TP runs we'll be doing on PRs. cc @lukebjerring

@jugglinmike
Copy link
Collaborator

This isn't directly analogous to the settings for Chrome and Firefox because it involves system-wide configuration (not a command-line argument). Contributors might be surprised to find their browser behaving differently after invoking wpt run. Maybe that could be alright if we prompted the user for permission or if we attempted to clean up the setting. A separate concern is how these changes are applied. If the browser is running, we have to terminate it before the changes take effect. Doing that in wpt run could also be disruptive for a contributor working from their development environment.

Discussion along these lines should probably take place in an issue filed against the WPT CLI. We've already asked @cvazac to do a fair amount of leg work (and they've been very accommodating!), so I've written something up at web-platform-tests/wpt#13976. In the mean time, I'd be happy to accept a patch for this project--we can remove it when/if it becomes superfluous.

@youennf
Copy link

youennf commented Nov 8, 2018

There are other flags that we might want to turn on as well (IntersectionObserver, MediaRecorder, WebRTC MDNS ICE candidates...) on a case by case basis since not all features will be stable enough to survive WPT testing.

I am not sure we should turn these features on within stable Safari.
It makes more sense for STP.

@cvazac
Copy link
Author

cvazac commented Nov 9, 2018

Should safari-disable-popup-blocker.sh be moved after install-browser.sh as well?

@foolip
Copy link
Member

foolip commented Nov 10, 2018

@cvazac @youennf are setting shared by Safari stable and TP, or are there settings that are only used by STP? Something that would make this easier is if it were possible to pass these settings to Safari as command line arguments to safaridriver. Do you think that'd make sense? cc @burg

@burg
Copy link

burg commented Nov 10, 2018 via email

@foolip
Copy link
Member

foolip commented Nov 10, 2018

Yes, that would be great I think, if they then have no effect beyond the WebDriver session.

@cvazac
Copy link
Author

cvazac commented Dec 14, 2018

Picking back up on this... if I'm re-reading this correctly, we want to turn on some features for TP only. Has anything upstream made this easier - or should I make those changes from this PR?

cc @jugglinmike @foolip @burg

@foolip
Copy link
Member

foolip commented Dec 14, 2018

@cvazac the code can live in wpt no problem, only issue is that it changes global state for a browser the user might also be using for browsing. Just putting it behind a flag that's enabled when running in CI would work for now.

@foolip
Copy link
Member

foolip commented Mar 15, 2019

ping @cvazac, are you still interested in this?

@cvazac
Copy link
Author

cvazac commented Mar 15, 2019

@foolip Yes, I would love to clean up the Safari results. Unfortunately, the path to get there (^^^) is not at all clear to me. :(

@foolip
Copy link
Member

foolip commented Mar 15, 2019

@cvazac is putting the behavior behind a flag that we only use in CI an OK path? Are there other blockers too?

@foolip
Copy link
Member

foolip commented Apr 1, 2019

I've sent web-platform-tests/wpt#16177 to do this for the Azure Pipelines setup which is now providing most of the results seen on wpt.fyi.

foolip added a commit to web-platform-tests/wpt that referenced this pull request Apr 1, 2019
foolip added a commit to web-platform-tests/wpt that referenced this pull request Apr 2, 2019
foolip added a commit to web-platform-tests/wpt that referenced this pull request Apr 5, 2019
gsnedders pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 11, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 5, 2019
…rver Timing in Safari TP, a=testonly

Automatic update from web-platform-tests
[Azure Pipelines] Enable experimental Server Timing in Safari TP (#16177)

Like web-platform-tests/results-collection#621.

Also update documentation to match.
--

wpt-commits: d11578defbed3c7c175de5510c4bb122bc81702b
wpt-pr: 16177
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jun 6, 2019
…rver Timing in Safari TP, a=testonly

Automatic update from web-platform-tests
[Azure Pipelines] Enable experimental Server Timing in Safari TP (#16177)

Like web-platform-tests/results-collection#621.

Also update documentation to match.
--

wpt-commits: d11578defbed3c7c175de5510c4bb122bc81702b
wpt-pr: 16177
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 23, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…rver Timing in Safari TP, a=testonly

Automatic update from web-platform-tests
[Azure Pipelines] Enable experimental Server Timing in Safari TP (#16177)

Like web-platform-tests/results-collection#621.

Also update documentation to match.
--

wpt-commits: d11578defbed3c7c175de5510c4bb122bc81702b
wpt-pr: 16177

UltraBlame original commit: e899ae7fd548d7a3e30d6e537e7f617e7ffa7aba
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
…rver Timing in Safari TP, a=testonly

Automatic update from web-platform-tests
[Azure Pipelines] Enable experimental Server Timing in Safari TP (#16177)

Like web-platform-tests/results-collection#621.

Also update documentation to match.
--

wpt-commits: d11578defbed3c7c175de5510c4bb122bc81702b
wpt-pr: 16177

UltraBlame original commit: e899ae7fd548d7a3e30d6e537e7f617e7ffa7aba
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…rver Timing in Safari TP, a=testonly

Automatic update from web-platform-tests
[Azure Pipelines] Enable experimental Server Timing in Safari TP (#16177)

Like web-platform-tests/results-collection#621.

Also update documentation to match.
--

wpt-commits: d11578defbed3c7c175de5510c4bb122bc81702b
wpt-pr: 16177

UltraBlame original commit: e899ae7fd548d7a3e30d6e537e7f617e7ffa7aba
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants