Bug 184758 – Runtime feature flag for Server-Timing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Runtime feature flag for Server-Timing

Created attachment 338263 [details]
Patch

[youenn fablet]

Comment on attachment 338263 [details]
Patch

Since this is off by default, you will need to activate it for WebKitTestRunner/DumpRenderTree.

[youenn fablet]

Comment on attachment 338263 [details]
Patch

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

> Source/WTF/wtf/FeatureDefines.h:687
> +#endif

These are compile flags. I am not sure we need this one actually.
Instead, I would go just with a runtime flag.

> Source/WebCore/page/RuntimeEnabledFeatures.h:254
> +    bool serverTimingEnabled() const { return m_isServerTimingEnabled; }

This is the runtime flag there.
To allow STP users to enable it as an experimental feature, you would need to add it to WebPreferences.yaml.
You will also need to activate it in WebKitTestRunner and DumpRenderTree by default. To ServiceWorker processes as well probably.
See for instance restrictedHTTPResponseAccess above as an example.

To validate that it is working, you could for instance add:
- some IDL behind the runtime flag: PerformanceServerTiming.idl/.h/.cpp and the change to PerformanceResourceTiming.idl
- a test verifying that the property is there on PerformanceResourceTiming.

[Ryosuke Niwa]

Comment on attachment 338263 [details]
Patch

r-'ing based on Youenn's comment.

Created attachment 338990 [details]
Patch

Created attachment 338996 [details]
Patch

[youenn fablet]

For win, you need to make some changes to Source/WebKitLegacy. You can use https://github.com/WebKit/webkit/commit/ecc55615907980a2ab27df68230081bd5ce07838 as an example. Would be nice to add the IDLs to validate that the flag is working as expected

Created attachment 339381 [details]
Patch

Created attachment 339383 [details]
Patch

[EWS Watchlist]

Comment on attachment 339383 [details]
Patch

Attachment 339383 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7546790

New failing tests:
imported/w3c/web-platform-tests/server-timing/resource_timing_idl.html
imported/w3c/web-platform-tests/server-timing/navigation_timing_idl.html

[EWS Watchlist]

Created attachment 339389 [details]
Archive of layout-test-results from ews116 for mac-sierra

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

[Ryosuke Niwa]

Comment on attachment 339383 [details]
Patch

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

> Source/WebCore/page/PerformanceServerTiming.h:46
> +    double m_duration;

We don't use double to store time anymore.
Is this monotonically increasing time or a wall time?

> Source/WebCore/page/RuntimeEnabledFeatures.h:396
> +    bool m_minDeviceWidthEnabled { false };
> +

Please remove this.

(In reply to Ryosuke Niwa from comment #12)
> Comment on attachment 339383 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339383&action=review
> 
> > Source/WebCore/page/PerformanceServerTiming.h:46
> > +    double m_duration;
> 
> We don't use double to store time anymore.
> Is this monotonically increasing time or a wall time?

it's just a user-defined double:
https://w3c.github.io/server-timing/#-dfn-duration-dfn-attribute


> 
> > Source/WebCore/page/RuntimeEnabledFeatures.h:396
> > +    bool m_minDeviceWidthEnabled { false };
> > +
> 
> Please remove this.

removed :)

Created attachment 339725 [details]
Patch

[youenn fablet]

Comment on attachment 339725 [details]
Patch

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

> Source/WebCore/page/RuntimeEnabledFeatures.h:395
> +    bool m_minDeviceWidthEnabled { false };

Probably do not need that one.

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:132
> +    RuntimeEnabledFeatures::sharedFeatures().setServerTimingEnabled(store.getBoolValueForKey(WebPreferencesKey::serverTimingEnabledKey()));

It would be nice to have a test for service worker like done for window.

Created attachment 339726 [details]
Patch

Created attachment 339732 [details]
Patch

Created attachment 339816 [details]
Patch

Created attachment 339818 [details]
Patch

[Don Olmstead]

With regards to WinCairo being red I believe this has to due with an issue for our build not regenerating things properly when IWebPreferencesPrivate.idl is modified. Since this is known we would just need to do a clean build on the bots when landed.

I can't make wincairo happy, although I'm hoping it's an issue with the build process not knowing when to regenerate things. 

Also, I added a WPT to verify that serverTiming is exposed on PerformanceResourceTiming from inside a Service Worker here:
https://github.com/w3c/web-platform-tests/pull/10901

The test passes when I manually copy it into my webkit tree. I will properly import it and add it to this bug when after it's merged into WPT.

[youenn fablet]

r = me.

> Also, I added a WPT to verify that serverTiming is exposed on
> PerformanceResourceTiming from inside a Service Worker here:
> https://github.com/w3c/web-platform-tests/pull/10901

Nice!
You could also have included it in this patch.
Once it is reviewed here, it is ok to land it directly in WPT.

> The test passes when I manually copy it into my webkit tree. I will properly
> import it and add it to this bug when after it's merged into WPT.

Created attachment 339866 [details]
Patch

[EWS Watchlist]

Comment on attachment 339866 [details]
Patch

Attachment 339866 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7611895

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

Created attachment 339880 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 339866 [details]
Patch

Attachment 339866 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7613065

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 339890 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 339866 [details]
Patch

Attachment 339866 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7613136

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

Created attachment 339891 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 339866 [details]
Patch

Attachment 339866 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7613423

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

Created attachment 339893 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 339866 [details]
Patch

Attachment 339866 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7617375

New failing tests:
http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients.html
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

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

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

Created attachment 339965 [details]
Patch

[EWS Watchlist]

Comment on attachment 339965 [details]
Patch

Attachment 339965 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7624045

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

Created attachment 339970 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 339965 [details]
Patch

Attachment 339965 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7624078

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

Created attachment 339975 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

The builds mac and mac-debug appear to be failing because service-workers aren't enabled. Do you know how to fix that Youenn? Do I have to do something in TestController.cpp?

Created attachment 340025 [details]
Patch

[youenn fablet]

(In reply to cvazac from comment #39)
> The builds mac and mac-debug appear to be failing because service-workers
> aren't enabled. Do you know how to fix that Youenn? Do I have to do
> something in TestController.cpp?

Any service worker test needs to be skipped for WK1.
Either you move the test in a folder that is already skipped or you need to update LayoutTests/platform/mac-wk1/TestExpectations and LayoutTests/platform/win/TestExpectations.

[Chris Dumez]

Comment on attachment 340025 [details]
Patch

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

> LayoutTests/platform/mac-wk1/TestExpectations:152
> +imported/w3c/web-platform-tests/server-timing/service_worker_idl.html [ Skip ]

You'll probably need to skip it on win as well. Also ios-wk1 for good measure.

(In reply to Chris Dumez from comment #42)
> Comment on attachment 340025 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340025&action=review
> 
> > LayoutTests/platform/mac-wk1/TestExpectations:152
> > +imported/w3c/web-platform-tests/server-timing/service_worker_idl.html [ Skip ]
> 
> You'll probably need to skip it on win as well. Also ios-wk1 for good
> measure.

Do I need still need to SKIP on win and ios-wk1? The only failures are here (https://webkit-queues.webkit.org/patch/340025/win-ews) and they appear unrelated.

[youenn fablet]

Comment on attachment 340025 [details]
Patch

r=me.
I do not know why the test does not show up as failing in WK1 win bot.
Anyway, I think it is nice to help maintainers by skipping it since we know service worker is not supported in WK1.

Created attachment 340095 [details]
Patch

Created attachment 340096 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 340096 [details]
Patch

Clearing flags on attachment: 340096

Committed r231709: <https://trac.webkit.org/changeset/231709>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40169546>