Bug 190609 - [Curl] Enable Server-Timing Experimental Feature by default
Summary: [Curl] Enable Server-Timing Experimental Feature by default
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-15 16:37 PDT by cvazac
Modified: 2021-06-14 06:15 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.15 KB, patch)
2018-10-15 16:38 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (3.45 KB, patch)
2018-10-16 14:15 PDT, cvazac
rniwa: review-
Details | Formatted Diff | Diff
Path for USE(CURL) (2.43 KB, patch)
2018-11-07 12:05 PST, Don Olmstead
rniwa: review-
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.23 MB, application/zip)
2018-11-07 13:27 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description cvazac 2018-10-15 16:37:57 PDT
Enable Server-Timing Experimental Feature by default
Comment 1 cvazac 2018-10-15 16:38:59 PDT
Created attachment 352401 [details]
Patch
Comment 2 cvazac 2018-10-15 16:44:11 PDT
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
Comment 3 Joseph Pecoraro 2018-10-15 16:54:48 PDT
(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/>.
Comment 4 Ryosuke Niwa 2018-10-15 21:45:16 PDT
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.
Comment 5 Ryosuke Niwa 2018-10-15 21:45:47 PDT
Does server timing API work in GTK+, Windows, WinCairo, and WPE ports? If not, we shouldn't be enabling this feature everywhere by default.
Comment 6 Ryosuke Niwa 2018-10-15 23:45:31 PDT
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.
Comment 7 Ryosuke Niwa 2018-10-15 23:46:04 PDT
Comment on attachment 352401 [details]
Patch

r- due to all the comments I've made above.
Comment 8 Ryosuke Niwa 2018-10-16 13:40:46 PDT
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.
Comment 9 cvazac 2018-10-16 14:03:26 PDT
(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
Comment 10 cvazac 2018-10-16 14:15:07 PDT
Created attachment 352504 [details]
Patch
Comment 11 Ryosuke Niwa 2018-10-16 15:06:16 PDT
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.
Comment 12 cvazac 2018-10-17 08:07:02 PDT
(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?
Comment 13 Ryosuke Niwa 2018-10-17 13:19:02 PDT
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.
Comment 14 Don Olmstead 2018-10-18 10:21:23 PDT
(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.
Comment 15 Don Olmstead 2018-11-07 11:03:36 PST
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.
Comment 16 Don Olmstead 2018-11-07 11:55:52 PST
We tested it. Please put PLATFORM(WIN_CAIRO) as the condition in the yaml then I think this is good to land.
Comment 17 Don Olmstead 2018-11-07 12:05:00 PST
Created attachment 354123 [details]
Path for USE(CURL)

This gets you part of the way. We're happy to run this with USE(CURL).
Comment 18 EWS Watchlist 2018-11-07 13:27:07 PST
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
Comment 19 EWS Watchlist 2018-11-07 13:27:09 PST
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
Comment 20 Ryosuke Niwa 2018-11-07 13:30:36 PST
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.
Comment 21 Ryosuke Niwa 2018-11-07 13:31:05 PST
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.