Adds a basic offline check to InstallableManager.

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

[dominickn, 2017-03-16 06:21:55]

dominickn@chromium.org changed reviewers:
+ dominickn@chromium.org

[dominickn, 2017-03-16 06:21:57]

Looks pretty good! Great work for your first CL. :)

Please edit the subject of the CL and the first line of the description to be
something like:

Add a basic offline check to InstallableManager.

And make sure to wrap the description to 80 characters, and add an overall
summary of what you're doing ("This CL expands the CheckHasServiceWorker method
to return an enum...")

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_logging.cc (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.cc:68: "a fetch handler is not
present in the service worker";
Nit: let's just make this "the page does not work offline". That way this
message will stay true over time as we ratchet up the checks.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_logging.h (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.h:49:
NO_SERVICE_WORKER_FETCH_HANDLER,
Nit: SITE_NOT_OFFLINE_CAPABLE ? I think it's best to avoid specifying what we're
exactly checking because the overall intent is to check offline capability.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.h:50: MAX_ERROR_CODE,
You'll need to edit the (giant giant) tools/metrics/histograms/histograms.xml
file to add this value to make sure the histograms backend picks this up (this
enum is used for sending metrics back to us).

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:416:
content::ServiceWorkerStatus sw_status) {
Nit: Call the variable status for consistency with the header file

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.h (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.h:200:
content::ServiceWorkerStatus offline_capability);
Nit: call the variable status

https://codereview.chromium.org/2751343002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2751343002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:901:
content::CheckFetchHandlerOfRegisteredServiceWorker(callback, registration);
Nit: this is all inside the content namespace, so I think you don't need the
content:: qualifier.

https://codereview.chromium.org/2751343002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:913:
content::CheckFetchHandlerOfRegisteredServiceWorker(callback, registration);
Nit: this is all inside the content namespace, so I think you don't need the
content:: qualifier.

[piotrs, 2017-03-17 02:06:18]

Description was changed from

==========
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.

BUG=601337
==========

to

==========
Add 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
==========

[piotrs, 2017-03-17 02:06:58]

Description was changed from

==========
Add 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
==========

to

==========
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
==========

[piotrs, 2017-03-17 02:21:38]

Thanks Dom! All done.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_logging.cc (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.cc:68: "a fetch handler is not
present in the service worker";
On 2017/03/16 06:21:56, dominickn wrote:
> Nit: let's just make this "the page does not work offline". That way this
> message will stay true over time as we ratchet up the checks.

Like it, thanks. Also it doesn't expose our internal (and arguably not very
comprehensive) check.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_logging.h (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.h:49:
NO_SERVICE_WORKER_FETCH_HANDLER,
On 2017/03/16 06:21:56, dominickn wrote:
> Nit: SITE_NOT_OFFLINE_CAPABLE ? I think it's best to avoid specifying what
we're
> exactly checking because the overall intent is to check offline capability.

Done.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.h:50: MAX_ERROR_CODE,
On 2017/03/16 06:21:56, dominickn wrote:
> You'll need to edit the (giant giant) tools/metrics/histograms/histograms.xml
> file to add this value to make sure the histograms backend picks this up (this
> enum is used for sending metrics back to us).

Thanks, missed that :) Done.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:416:
content::ServiceWorkerStatus sw_status) {
On 2017/03/16 06:21:56, dominickn wrote:
> Nit: Call the variable status for consistency with the header file

Done.

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.h (right):

https://codereview.chromium.org/2751343002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.h:200:
content::ServiceWorkerStatus offline_capability);
On 2017/03/16 06:21:56, dominickn wrote:
> Nit: call the variable status

Done.

https://codereview.chromium.org/2751343002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2751343002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:901:
content::CheckFetchHandlerOfRegisteredServiceWorker(callback, registration);
On 2017/03/16 06:21:57, dominickn wrote:
> Nit: this is all inside the content namespace, so I think you don't need the
> content:: qualifier.

Done.

https://codereview.chromium.org/2751343002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:913:
content::CheckFetchHandlerOfRegisteredServiceWorker(callback, registration);
On 2017/03/16 06:21:57, dominickn wrote:
> Nit: this is all inside the content namespace, so I think you don't need the
> content:: qualifier.

Done.

[piotrs, 2017-03-17 02:48:02]

The CQ bit was checked by piotrs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 02:48:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:05:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 03:05:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piotrs, 2017-03-17 03:34:07]

The CQ bit was checked by piotrs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 03:34:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-03-17 04:03:34]

chrome/browser/installable lgtm, thanks Piotr

[piotrs, 2017-03-17 05:33:21]

piotrs@chromium.org changed reviewers:
+ falken@chromium.org

[piotrs, 2017-03-17 05:35:41]

Hi Falken, can you please take a look at this change? Thanks!

[commit-bot: I haz the power, 2017-03-17 05:38:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 05:38:29]

Dry run: This issue passed the CQ dry run.

[falken, 2017-03-17 07:54:37]

This looks pretty good. I have some mostly naming nits.

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
File chrome/test/data/banners/service_worker.js (right):

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
chrome/test/data/banners/service_worker.js:5: self.addEventListener('activate',
function(event) {});
Ditto, not sure I understand the poitn of this activate event handler.

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
chrome/test/data/banners/service_worker.js:8: self.addEventListener('fetch', e
=> e.respondWith(fetch(e.request)));
Better to just have an empty function. respondWith(fetch(request) ) is
surprisingly slow and complex.

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
File chrome/test/data/banners/service_worker_no_fetch_handler.js (right):

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
chrome/test/data/banners/service_worker_no_fetch_handler.js:5:
self.addEventListener('activate', function(event) {});
Should we have a comment about why this activate event handler is registered?

I might have just done instead:
// empty service worker

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:47: void
CheckFetchHandlerOfRegisteredServiceWorker(
This name is a bit inconsistent with SW nomenclature. All service workers are
"registered" in the sense that all service workers are associated with a
registration.

I might recommend CheckFetchHandlerOfInstalledServiceWorker or just
CheckFetchHandler and with maybe a comment that the registration needs to have
an installed worker. Or maybe the caller should just pass the
ServiceWorkerVersion directly?

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:52: :
registration->waiting_version();
Are you sure you want the active version to override the waiting version? The
waiting version is the newest installed version. So when making decisions about
whether this SW registration has offline capability, it might be more
appropriate to use the waiting version.

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:59:
DCHECK(existence != ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
nit: Is it possible to use DCHECK_NE here?

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:21: enum class
ServiceWorkerStatus {
I'm a bit worried we have too many types named "Status", and this is a pretty
generic name. It's hard to distinguish this from
content::ServiceWorkerStatusCode by name.

How about ServiceWorkerExistence?

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:114: // which has a longer match
for |other_url|.
It'd be worth clarifying in this comment what the function does in case of a
registration with only a .installing worker.

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:118: virtual void
CheckServiceWorkerStatus(
I think the original name may be clearer.

[piotrs, 2017-03-21 00:00:44]

Thanks Falken for the comments, all done pretty much. PTAL.

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
File chrome/test/data/banners/service_worker.js (right):

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
chrome/test/data/banners/service_worker.js:5: self.addEventListener('activate',
function(event) {});
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> Ditto, not sure I understand the poitn of this activate event handler.

Done.

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
chrome/test/data/banners/service_worker.js:8: self.addEventListener('fetch', e
=> e.respondWith(fetch(e.request)));
On 2017/03/17 07:54:36, falken (ooo Mon Mar 20) wrote:
> Better to just have an empty function. respondWith(fetch(request) ) is
> surprisingly slow and complex.

Done.

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
File chrome/test/data/banners/service_worker_no_fetch_handler.js (right):

https://codereview.chromium.org/2751343002/diff/80001/chrome/test/data/banner...
chrome/test/data/banners/service_worker_no_fetch_handler.js:5:
self.addEventListener('activate', function(event) {});
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> Should we have a comment about why this activate event handler is registered?
> 
> I might have just done instead:
> // empty service worker

Done.

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:47: void
CheckFetchHandlerOfRegisteredServiceWorker(
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> This name is a bit inconsistent with SW nomenclature. All service workers are
> "registered" in the sense that all service workers are associated with a
> registration.
> 
> I might recommend CheckFetchHandlerOfInstalledServiceWorker or just
> CheckFetchHandler and with maybe a comment that the registration needs to have
> an installed worker. Or maybe the caller should just pass the
> ServiceWorkerVersion directly?

Chunk of the work of this method is taking right service worker version from
registration, so I went for your suggestion to use
CheckFetchHandlerOfInstalledServiceWorker.

Done.

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:52: :
registration->waiting_version();
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> Are you sure you want the active version to override the waiting version? The
> waiting version is the newest installed version. So when making decisions
about
> whether this SW registration has offline capability, it might be more
> appropriate to use the waiting version.

Done + added a comment for posterity as it's possibly not obvious for every
reader.

https://codereview.chromium.org/2751343002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:59:
DCHECK(existence != ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> nit: Is it possible to use DCHECK_NE here?

Done.

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:21: enum class
ServiceWorkerStatus {
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> I'm a bit worried we have too many types named "Status", and this is a pretty
> generic name. It's hard to distinguish this from
> content::ServiceWorkerStatusCode by name.
> 
> How about ServiceWorkerExistence?
> 

It's not just about existence but about the extent to which it can handle
offline requests, so I'm not sold on 'existence'. At least that's the current
intention and we expect to extend this check in the future. I don't know the
codebase well enough yet, but I expected that status is probably overloaded
already. Some other options: ServiceWorker{Capability/Capacity/Class/Type/Kind}.

I went for ServiceWorkerCapability, please comment if you would prefer this to
change.

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:114: // which has a longer match
for |other_url|.
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> It'd be worth clarifying in this comment what the function does in case of a
> registration with only a .installing worker.

Good point, I've rewritten it a bit as well, as I should have updated it. Take a
look!

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:118: virtual void
CheckServiceWorkerStatus(
On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> I think the original name may be clearer.

Reverted, Done.

[piotrs, 2017-03-21 00:10:34]

The CQ bit was checked by piotrs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 00:11:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[falken, 2017-03-21 01:53:24]

lgtm

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2751343002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:21: enum class
ServiceWorkerStatus {
On 2017/03/21 00:00:44, piotrs wrote:
> On 2017/03/17 07:54:37, falken (ooo Mon Mar 20) wrote:
> > I'm a bit worried we have too many types named "Status", and this is a
pretty
> > generic name. It's hard to distinguish this from
> > content::ServiceWorkerStatusCode by name.
> > 
> > How about ServiceWorkerExistence?
> > 
> 
> It's not just about existence but about the extent to which it can handle
> offline requests, so I'm not sold on 'existence'. At least that's the current
> intention and we expect to extend this check in the future. I don't know the
> codebase well enough yet, but I expected that status is probably overloaded
> already. Some other options:
ServiceWorker{Capability/Capacity/Class/Type/Kind}.
> 
> I went for ServiceWorkerCapability, please comment if you would prefer this to
> change.

SGTM.

https://codereview.chromium.org/2751343002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2751343002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:50: // Waiting
Service Worker is a newer version, prefer that if available;
nit: trailing semi-colon :)

https://codereview.chromium.org/2751343002/diff/120001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2751343002/diff/120001/content/public/browser...
content/public/browser/service_worker_context.h:116: // |other_url|. In case the
service worker is being installed as of calling
Thanks for expanding this comment! For some reason I have trouble reading:
"this doesn't ensure to describe a Service Worker registration with the longest
possible match for |other_url|"

Is the following better?
"Note that the longest matching registration for |url| is described, which is
not necessarily the longest matching registration for |other_url|."

https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:80609: +  <int value="29" label="Service
worker not offline capable"/>
nit: Without knowing the code, I would interpret this to mean a service worker
exists. Maybe "No offline-capable service worker" ?

[piotrs, 2017-03-21 02:12:48]

piotrs@chromium.org changed reviewers:
+ avi@chromium.org, isherman@chromium.org

[piotrs, 2017-03-21 02:12:49]

Thanks for your comments Falken.

Avi, can you please take a look at content/public?

Ilya, can I ask you to review tools/metrics?

Cheers!

https://codereview.chromium.org/2751343002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2751343002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:50: // Waiting
Service Worker is a newer version, prefer that if available;
On 2017/03/21 01:53:24, falken wrote:
> nit: trailing semi-colon :)

Done.

https://codereview.chromium.org/2751343002/diff/120001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2751343002/diff/120001/content/public/browser...
content/public/browser/service_worker_context.h:116: // |other_url|. In case the
service worker is being installed as of calling
On 2017/03/21 01:53:24, falken wrote:
> Thanks for expanding this comment! For some reason I have trouble reading:
> "this doesn't ensure to describe a Service Worker registration with the
longest
> possible match for |other_url|"
> 
> Is the following better?
> "Note that the longest matching registration for |url| is described, which is
> not necessarily the longest matching registration for |other_url|."

Done.

https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:80609: +  <int value="29" label="Service
worker not offline capable"/>
On 2017/03/21 01:53:24, falken wrote:
> nit: Without knowing the code, I would interpret this to mean a service worker
> exists. Maybe "No offline-capable service worker" ?

Your understanding as in "a service worker exists" is correct. It exists, but we
know it is not capable of handling offline calls.

I'm not sure I'm reading you comment correctly though.

[falken, 2017-03-21 02:15:23]

https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2751343002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:80609: +  <int value="29" label="Service
worker not offline capable"/>
On 2017/03/21 02:12:49, piotrs wrote:
> On 2017/03/21 01:53:24, falken wrote:
> > nit: Without knowing the code, I would interpret this to mean a service
worker
> > exists. Maybe "No offline-capable service worker" ?
> 
> Your understanding as in "a service worker exists" is correct. It exists, but
we
> know it is not capable of handling offline calls.
> 
> I'm not sure I'm reading you comment correctly though.

Ah, I misread the code, sorry for the noise.

[Avi (use Gerrit), 2017-03-21 02:17:24]

content/public lgtm

[Ilya Sherman, 2017-03-21 20:45:51]

histograms.xml lgtm

[piotrs, 2017-03-21 23:22:39]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-03-21 23:22:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2751343002/#ps140001
(title: "Updating comments")

[piotrs, 2017-03-21 23:22:42]

The CQ bit was unchecked by piotrs@chromium.org

[piotrs, 2017-03-21 23:24:48]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-03-21 23:24:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, falken@chromium.org, avi@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2751343002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-21 23:25:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 23:37:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:37:25]

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/bui...)

[piotrs, 2017-03-22 02:21:16]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-03-22 02:21:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 04:23:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 04:23:41]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[piotrs, 2017-03-22 04:58:11]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-03-22 04:58:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 06:38:40]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490158691405690,
"parent_rev": "62c02186d3769b4e84cdbcba232fb8ceee6b567f", "commit_rev":
"3e32ed5683dd6e1cf0178e07c6e58b0e8198a7c6"}

[commit-bot: I haz the power, 2017-03-22 06:40:20]

Description was changed from

==========
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
==========

to

==========
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/+/3e32ed5683dd6e1cf0178e07c6e5...
==========

[commit-bot: I haz the power, 2017-03-22 06:40:24]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3e32ed5683dd6e1cf0178e07c6e5...