Bug 184758 - Runtime feature flag for Server-Timing
Summary: Runtime feature flag for Server-Timing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 175569
  Show dependency treegraph
 
Reported: 2018-04-18 14:34 PDT by cvazac
Modified: 2018-05-11 11:35 PDT (History)
15 users (show)

See Also:


Attachments
Patch (44.07 KB, patch)
2018-04-18 14:41 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2018-04-27 08:29 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2018-04-27 09:39 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (35.76 KB, patch)
2018-05-02 21:00 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (35.03 KB, patch)
2018-05-02 21:08 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.11 MB, application/zip)
2018-05-02 22:40 PDT, Build Bot
no flags Details
Patch (34.56 KB, patch)
2018-05-07 08:55 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (35.10 KB, patch)
2018-05-07 09:11 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (35.05 KB, patch)
2018-05-07 11:12 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (34.81 KB, patch)
2018-05-08 07:11 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (34.65 KB, patch)
2018-05-08 07:17 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (36.77 KB, patch)
2018-05-08 13:39 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.58 MB, application/zip)
2018-05-08 14:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.91 MB, application/zip)
2018-05-08 15:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (8.94 MB, application/zip)
2018-05-08 16:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.12 MB, application/zip)
2018-05-08 16:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.98 MB, application/zip)
2018-05-08 17:52 PDT, Build Bot
no flags Details
Patch (36.81 KB, patch)
2018-05-09 08:27 PDT, cvazac
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.27 MB, application/zip)
2018-05-09 09:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.12 MB, application/zip)
2018-05-09 10:08 PDT, Build Bot
no flags Details
Patch (37.70 KB, patch)
2018-05-09 14:26 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (39.46 KB, patch)
2018-05-10 09:31 PDT, cvazac
no flags Details | Formatted Diff | Diff
Patch (39.64 KB, patch)
2018-05-10 09:58 PDT, cvazac
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cvazac 2018-04-18 14:34:57 PDT
Runtime feature flag for Server-Timing
Comment 1 cvazac 2018-04-18 14:41:40 PDT
Created attachment 338263 [details]
Patch
Comment 2 youenn fablet 2018-04-23 14:51:49 PDT
Comment on attachment 338263 [details]
Patch

Since this is off by default, you will need to activate it for WebKitTestRunner/DumpRenderTree.
Comment 3 youenn fablet 2018-04-23 15:01:32 PDT
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.
Comment 4 Ryosuke Niwa 2018-04-23 19:28:16 PDT
Comment on attachment 338263 [details]
Patch

r-'ing based on Youenn's comment.
Comment 5 cvazac 2018-04-27 08:29:03 PDT
Created attachment 338990 [details]
Patch
Comment 6 cvazac 2018-04-27 09:39:54 PDT
Created attachment 338996 [details]
Patch
Comment 7 youenn fablet 2018-04-27 11:01:31 PDT
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
Comment 8 cvazac 2018-05-02 21:00:55 PDT
Created attachment 339381 [details]
Patch
Comment 9 cvazac 2018-05-02 21:08:12 PDT
Created attachment 339383 [details]
Patch
Comment 10 Build Bot 2018-05-02 22:40:33 PDT
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
Comment 11 Build Bot 2018-05-02 22:40:34 PDT
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
Comment 12 Ryosuke Niwa 2018-05-03 16:07:22 PDT
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.
Comment 13 cvazac 2018-05-03 16:58:43 PDT
(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 :)
Comment 14 cvazac 2018-05-07 08:55:47 PDT
Created attachment 339725 [details]
Patch
Comment 15 youenn fablet 2018-05-07 09:01:11 PDT
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.
Comment 16 cvazac 2018-05-07 09:11:31 PDT
Created attachment 339726 [details]
Patch
Comment 17 cvazac 2018-05-07 11:12:52 PDT
Created attachment 339732 [details]
Patch
Comment 18 cvazac 2018-05-08 07:11:32 PDT
Created attachment 339816 [details]
Patch
Comment 19 cvazac 2018-05-08 07:17:40 PDT
Created attachment 339818 [details]
Patch
Comment 20 Don Olmstead 2018-05-08 11:31:04 PDT
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.
Comment 21 cvazac 2018-05-08 11:31:39 PDT
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.
Comment 22 youenn fablet 2018-05-08 11:43:34 PDT
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.
Comment 23 cvazac 2018-05-08 13:39:41 PDT
Created attachment 339866 [details]
Patch
Comment 24 Build Bot 2018-05-08 14:38:20 PDT
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
Comment 25 Build Bot 2018-05-08 14:38:22 PDT
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
Comment 26 Build Bot 2018-05-08 15:54:32 PDT
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
Comment 27 Build Bot 2018-05-08 15:54:43 PDT
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
Comment 28 Build Bot 2018-05-08 16:01:08 PDT
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
Comment 29 Build Bot 2018-05-08 16:01:10 PDT
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
Comment 30 Build Bot 2018-05-08 16:11:41 PDT
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
Comment 31 Build Bot 2018-05-08 16:11:43 PDT
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
Comment 32 Build Bot 2018-05-08 17:52:29 PDT
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
Comment 33 Build Bot 2018-05-08 17:52:32 PDT
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
Comment 34 cvazac 2018-05-09 08:27:10 PDT
Created attachment 339965 [details]
Patch
Comment 35 Build Bot 2018-05-09 09:39:10 PDT
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
Comment 36 Build Bot 2018-05-09 09:39:11 PDT
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
Comment 37 Build Bot 2018-05-09 10:08:57 PDT
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
Comment 38 Build Bot 2018-05-09 10:08:59 PDT
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
Comment 39 cvazac 2018-05-09 13:35:51 PDT
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?
Comment 40 cvazac 2018-05-09 14:26:59 PDT
Created attachment 340025 [details]
Patch
Comment 41 youenn fablet 2018-05-09 14:29:26 PDT
(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.
Comment 42 Chris Dumez 2018-05-09 14:32:25 PDT
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.
Comment 43 cvazac 2018-05-10 08:57:06 PDT
(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.
Comment 44 youenn fablet 2018-05-10 09:04:35 PDT
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.
Comment 45 cvazac 2018-05-10 09:31:25 PDT
Created attachment 340095 [details]
Patch
Comment 46 cvazac 2018-05-10 09:58:07 PDT
Created attachment 340096 [details]
Patch
Comment 47 WebKit Commit Bot 2018-05-11 11:33:50 PDT
Comment on attachment 340096 [details]
Patch

Clearing flags on attachment: 340096

Committed r231709: <https://trac.webkit.org/changeset/231709>
Comment 48 WebKit Commit Bot 2018-05-11 11:33:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 49 Radar WebKit Bug Importer 2018-05-11 11:35:55 PDT
<rdar://problem/40169546>