Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(231)

Issue 2637763002: Enable ServiceWorkerNavigationPreload by default for Origin-Trial. (Closed)

Created:
3 years, 11 months ago by horo
Modified:
3 years, 11 months ago
Reviewers:
falken, kinuko
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, blink-reviews, darin-cc_chromium.org, kinuko+watch, tzik, falken+watch_chromium.org, blink-worker-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Enable ServiceWorkerNavigationPreload by default for Origin-Trial. The CL https://codereview.chromium.org/2627023002/ introduced the mechanism for Origin-Trial experiment of Service Worker Navigation Preload. But base::Feature of the feature was disabled by default. So when we start the Origin-Trial experiment, we had to enable the feature by the field trial mechanism. But it is possible that the feature is not correctly enabled by the field trial mechanism (Ex: in the first boot of Chrome). In order to avoid the confusion, this CL enables the base::Feature of the feature by default. If something goes wrong and we will want to stop the origin-trial, we can stop the experiment by disabling this feature using the field trial mechanism. * Origin Trial: Have an effective token. Command line Default Enable Disabled Default *A* A B Field trial Enabled A A B Disabled B A B * Origin Trial: No token. Command line Default Enable Disabled Default C A C Field trial Enabled C A C Disabled C A C - A: Navigation Preload related methods and attributes are available in JS and work correctly. - B: Navigation Preload related methods and attributes are available in JS. But NavigationPreloadManager's enable, disable and setHeaderValue methods always return a rejected promise. And FetchEvent's preloadResponse attribute returns a promise which always resolve with undefined. - C: Navigation Preload related methods and attributes are not available in JS. BUG=649558 Review-Url: https://codereview.chromium.org/2637763002 Cr-Commit-Position: refs/heads/master@{#443851} Committed: https://chromium.googlesource.com/chromium/src/+/9ad964459156c4bd27fa68a0bec4155409ec492d

Patch Set 1 #

Total comments: 2

Patch Set 2 : incorporated kinuko's comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -20 lines) Patch
M content/browser/service_worker/service_worker_version.h View 1 chunk +1 line, -1 line 0 comments Download
M content/child/runtime_features.cc View 1 1 chunk +7 lines, -7 lines 0 comments Download
M content/public/common/content_features.cc View 1 1 chunk +5 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods-expected.txt View 1 chunk +10 lines, -9 lines 0 comments Download

Messages

Total messages: 20 (13 generated)
horo
falken@ Please review this.
3 years, 11 months ago (2017-01-16 04:45:51 UTC) #9
falken
lgtm
3 years, 11 months ago (2017-01-16 04:55:53 UTC) #10
horo
kinuko@ Could you please review content/child/runtime_features.cc content/public/common/content_features.cc?
3 years, 11 months ago (2017-01-16 04:58:13 UTC) #12
kinuko
lgtm https://codereview.chromium.org/2637763002/diff/1/content/public/common/content_features.cc File content/public/common/content_features.cc (right): https://codereview.chromium.org/2637763002/diff/1/content/public/common/content_features.cc#newcode195 content/public/common/content_features.cc:195: // runtime_features.cc and service_worker_version.h for the details. nit: ...
3 years, 11 months ago (2017-01-16 05:07:10 UTC) #13
horo
Thank you. https://codereview.chromium.org/2637763002/diff/1/content/public/common/content_features.cc File content/public/common/content_features.cc (right): https://codereview.chromium.org/2637763002/diff/1/content/public/common/content_features.cc#newcode195 content/public/common/content_features.cc:195: // runtime_features.cc and service_worker_version.h for the details. ...
3 years, 11 months ago (2017-01-16 05:30:37 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2637763002/20001
3 years, 11 months ago (2017-01-16 05:31:52 UTC) #17
commit-bot: I haz the power
3 years, 11 months ago (2017-01-16 06:55:51 UTC) #20
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9ad964459156c4bd27fa68a0bec4...

Powered by Google App Engine
This is Rietveld 408576698