Bug 190609 – [Curl] Enable Server-Timing Experimental Feature by default

• Status: NEW
• Component: New Bugs

Enable Server-Timing Experimental Feature by default

Created attachment 352401 [details]
Patch

Server-Timing originally landed here [0] and the devtools just landed here [1]. This CR just flips the experimental flag to ON by default. I _think_ this is the only place it needs to be done.

[0] https://bugs.webkit.org/show_bug.cgi?id=175569
[1] https://bugs.webkit.org/show_bug.cgi?id=190440

[Joseph Pecoraro]

(In reply to cvazac from comment #2)
> I _think_ this is the only place it needs to be done.

We should add a "Server Timing" entry to the Source/WebCore/features.json file as well, that lists WebKit's level of support. This will allow an entry to show up on the Feature Status page <https://webkit.org/status/>.

[Ryosuke Niwa]

Comment on attachment 352401 [details]
Patch

We also need to update the default value for m_isServerTimingEnabled in RuntimeEnabledFeatures in order to enable it in WebKit1.

[Ryosuke Niwa]

Does server timing API work in GTK+, Windows, WinCairo, and WPE ports? If not, we shouldn't be enabling this feature everywhere by default.

[Ryosuke Niwa]

By the way, enabling a feature like this which can have a platform-specific implementation issues should be announced on webkit-dev. In theory, every feature development should be announced on webkit-dev but we're not really doing that these days. In this particular case, the chance of the feature being broken on some exotic network stack is pretty high so it warrants a warning for port maintainers who might be about to branch for their product release.

Also, we ask that this feature NOT be turned in macOS, iOS, tvOS, and watchOS ports Apple maintains.

[Ryosuke Niwa]

Comment on attachment 352401 [details]
Patch

r- due to all the comments I've made above.

[Ryosuke Niwa]

Note that I'm not saying that we'd never enable server timing API in Apple's ports. We're simply asking you not do to that at this point in time.

(In reply to Ryosuke Niwa from comment #8)
> Note that I'm not saying that we'd never enable server timing API in Apple's
> ports. We're simply asking you not do to that at this point in time.

Understood, thanks for that and the review!

Just as an FYI, I sent out an intent to implement [0] last year. If I need any sort of intent to ship, please let me know.

If I’m reading the build results on my last patch [1] right, those ports should be covered and are all green. I verified that the LayoutTests aren’t being excluded on those builds in the TestExpectations files. 

[0] https://lists.webkit.org/pipermail/webkit-dev/2017-August/029442.html
[1] https://bugs.webkit.org/show_bug.cgi?id=175569

Created attachment 352504 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 352504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352504&action=review

> Source/WebCore/page/RuntimeEnabledFeatures.h:356
> -    bool m_isServerTimingEnabled { false };
> +    bool m_isServerTimingEnabled { true };

Again the ask here is to NOT enable this on PLATFORM(COCOA) and PLATFORM(WIN).

> Source/WebKit/Shared/WebPreferences.yaml:1291
> -  defaultValue: false
> +  defaultValue: true

Ditto.

(In reply to Ryosuke Niwa from comment #11)
> Comment on attachment 352504 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352504&action=review
> 
> > Source/WebCore/page/RuntimeEnabledFeatures.h:356
> > -    bool m_isServerTimingEnabled { false };
> > +    bool m_isServerTimingEnabled { true };
> 
> Again the ask here is to NOT enable this on PLATFORM(COCOA) and
> PLATFORM(WIN).
> 
> > Source/WebKit/Shared/WebPreferences.yaml:1291
> > -  defaultValue: false
> > +  defaultValue: true
> 
> Ditto.

Is it ok to leave this open until we are ready to ship it? Or would you prefer we close this and I just hang onto the code in a local branch?

[Ryosuke Niwa]

Yeah, we can certainly keep this bug open. Also, we can enable it by default for other ports assuming other port maintainers agree to it. Regardless, it seems like this warrants a webkit-dev posting.

Thanks for confirming that the tests pass on other ports.

[Don Olmstead]

(In reply to Ryosuke Niwa from comment #13)
> Yeah, we can certainly keep this bug open. Also, we can enable it by default
> for other ports assuming other port maintainers agree to it. Regardless, it
> seems like this warrants a webkit-dev posting.
> 
> Thanks for confirming that the tests pass on other ports.

Maybe make a separate bug for adding to the features.json? That way its at least advertised.

[Don Olmstead]

Charles,

We talked internally and we'll see what happens with this when we enable it on WinCairo. If it works then you can just use us as the default.

[Don Olmstead]

We tested it. Please put PLATFORM(WIN_CAIRO) as the condition in the yaml then I think this is good to land.

[Don Olmstead]

Created attachment 354123 [details]
Path for USE(CURL)

This gets you part of the way. We're happy to run this with USE(CURL).

[EWS Watchlist]

Comment on attachment 354123 [details]
Path for USE(CURL)

Attachment 354123 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9897496

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/resource_timing_idl.https.html
imported/w3c/web-platform-tests/server-timing/resource_timing_idl.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.https.html

[EWS Watchlist]

Created attachment 354138 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Comment on attachment 354123 [details]
Path for USE(CURL)

View in context: https://bugs.webkit.org/attachment.cgi?id=354123&action=review

> Source/WebKit/Shared/WebPreferences.yaml:1308
> +  condition: USE(CURL)

This isn't the right way of enabling the feature only where CURL is used since we still need to enable this feature by default in DRT/WTR.
You'd have to change defaultValue to a macro and then conditionally define that macro.

[Ryosuke Niwa]

Comment on attachment 352504 [details]
Patch

r- because we're asking not to enable this feature on macOS & iOS ports at this point in time.