Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2751343002: Adds a basic offline check to InstallableManager. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by piotrs
Modified:
4 days, 10 hours ago
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Adds a basic offline check to InstallableManager. Adds a check for existence of a fetch handler in the service worker as a small step towards checking offline capabilities for the A2HS feature. Modifies CheckHasServiceWorker method to return an enum distinguishing a service worker with and without a registered fetch handler. BUG=601337 Review-Url: https://codereview.chromium.org/2751343002 Cr-Commit-Position: refs/heads/master@{#458643} Committed: https://chromium.googlesource.com/chromium/src/+/3e32ed5683dd6e1cf0178e07c6e58b0e8198a7c6

Patch Set 1 #

Total comments: 14

Patch Set 2 : applying fixes #

Patch Set 3 : Few renames (s/sw_status/status/) #

Patch Set 4 : Renames InstallableStatusCode enum value #

Patch Set 5 : Updated tests #

Total comments: 19

Patch Set 6 : Changes requested by Falken #

Patch Set 7 : Updating comments #

Total comments: 7

Patch Set 8 : Updating comments #

Patch Set 9 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -29 lines) Patch
M chrome/browser/installable/installable_logging.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/installable/installable_logging.cc View 1 2 3 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/installable/installable_manager.h View 1 2 3 4 5 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/installable/installable_manager.cc View 1 2 3 4 5 6 7 8 1 chunk +14 lines, -6 lines 0 comments Download
M chrome/browser/installable/installable_manager_browsertest.cc View 1 2 3 4 1 chunk +24 lines, -0 lines 0 comments Download
A chrome/test/data/banners/no_sw_fetch_handler_test_page.html View 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/test/data/banners/service_worker.js View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
A + chrome/test/data/banners/service_worker_no_fetch_handler.js View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M content/browser/service_worker/service_worker_context_core.cc View 1 2 3 4 5 6 7 4 chunks +31 lines, -6 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.cc View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -4 lines 0 comments Download
M content/public/browser/service_worker_context.h View 1 2 3 4 5 6 7 3 chunks +15 lines, -5 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 44 (26 generated)
dominickn
Looks pretty good! Great work for your first CL. :) Please edit the subject of ...
1 week, 3 days ago (2017-03-16 06:21:57 UTC) #2
piotrs
Thanks Dom! All done. https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/installable_logging.cc File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/installable_logging.cc#newcode68 chrome/browser/installable/installable_logging.cc:68: "a fetch handler is not ...
1 week, 2 days ago (2017-03-17 02:21:38 UTC) #5
dominickn
chrome/browser/installable lgtm, thanks Piotr
1 week, 2 days ago (2017-03-17 04:03:34 UTC) #12
piotrs
1 week, 2 days ago (2017-03-17 05:33:22 UTC) #14
piotrs
Hi Falken, can you please take a look at this change? Thanks!
1 week, 2 days ago (2017-03-17 05:35:41 UTC) #15
falken
This looks pretty good. I have some mostly naming nits. https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banners/service_worker.js File chrome/test/data/banners/service_worker.js (right): https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banners/service_worker.js#newcode5 ...
1 week, 2 days ago (2017-03-17 07:54:37 UTC) #18
piotrs
Thanks Falken for the comments, all done pretty much. PTAL. https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banners/service_worker.js File chrome/test/data/banners/service_worker.js (right): https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banners/service_worker.js#newcode5 ...
5 days, 16 hours ago (2017-03-21 00:00:44 UTC) #19
falken
lgtm https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/service_worker_context.h File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/service_worker_context.h#newcode21 content/public/browser/service_worker_context.h:21: enum class ServiceWorkerStatus { On 2017/03/21 00:00:44, piotrs ...
5 days, 14 hours ago (2017-03-21 01:53:24 UTC) #22
piotrs
Thanks for your comments Falken. Avi, can you please take a look at content/public? Ilya, ...
5 days, 14 hours ago (2017-03-21 02:12:49 UTC) #24
falken
https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histograms/histograms.xml#newcode80609 tools/metrics/histograms/histograms.xml:80609: + <int value="29" label="Service worker not offline capable"/> On ...
5 days, 14 hours ago (2017-03-21 02:15:23 UTC) #25
Avi
content/public lgtm
5 days, 14 hours ago (2017-03-21 02:17:24 UTC) #26
Ilya Sherman
histograms.xml lgtm
4 days, 20 hours ago (2017-03-21 20:45:51 UTC) #27
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/2751343002/160001
4 days, 17 hours ago (2017-03-21 23:25:09 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/176936)
4 days, 17 hours ago (2017-03-21 23:37:25 UTC) #35
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/2751343002/160001
4 days, 14 hours ago (2017-03-22 02:21:53 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder ...
4 days, 12 hours ago (2017-03-22 04:23:41 UTC) #39
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/2751343002/160001
4 days, 11 hours ago (2017-03-22 04:58:57 UTC) #41
commit-bot: I haz the power
4 days, 10 hours ago (2017-03-22 06:40:24 UTC) #44
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3e32ed5683dd6e1cf0178e07c6e5...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62