Teach CrOS's OfflineResourceThrottle about ServiceWorkers so it know longer presents its offline in…

Teach CrOS's OfflineResourceThrottle about ServiceWorkers so it no longer presents its offline interstitial page when a ServiceWorker can handle the request.

TBR=jochen@chromium.org
BUG=439876
Committed: https://crrev.com/463cbca202c52fcae7ef7f05cd957d2f8d517bd5
Cr-Commit-Position: refs/heads/master@{#310937}

[michaeln, 2014-12-15 23:28:17]

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

[michaeln, 2014-12-15 23:28:18]

ptal

[falken, 2014-12-16 08:16:50]

Thanks for working on this. Looks pretty good.

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/offline_resource_throttle.cc (right):

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/offline_resource_throttle.cc:157:
service_worker_completion_callback_.Cancel();
Would it improve readability to set pending_callbacks_ = 0 here and above, then
DCHECK(!pending_callbacks_) in WillStartRequest, and then use
++pending_callbacks_ there instead of setting to 1 and 2 directly? Just a
suggestion, WDYT.

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/offline_resource_throttle.h (right):

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/offline_resource_throttle.h:47: // A callback to
tell if an appcache exists.
This comment should generalized now that SW also exists.

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:202: static
void DidFindRegistrationForDocument(
nit: chromium codebase tends to put these in unnamed namespace instead of as
static functions

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:217: if
(!url.SchemeIsSecure()) {
Is this needed? Can FindRegistrationForDocument return true for a non-secure
URL?

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:222: // check
for 3rd party cookie usage.
I'm wondering what is the consequence of not doing this check? I guess the
offline interstitial doesn't get displayed when it could, but later on the
request handler fails and the user sees some offline message then... which
doesn't seem so bad. Why does Chrome OS have this special offline page?

https://codereview.chromium.org/810583002/diff/60001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/810583002/diff/60001/content/public/browser/s...
content/public/browser/service_worker_context.h:76: // Determines if a request
for 'url' can be satisfied while offline.
nit: use |url| style instead of 'url'

[falken, 2015-01-08 05:51:10]

Hi Michael, how is this patch going? Do you think it can get in by M41?

[michaeln, 2015-01-09 01:46:06]

I think this is ready for a final review. I don't see any existing tests for the
CrOS OfflineThrottle, I didn't cons up a test harness for this new case.

[michaeln, 2015-01-09 01:47:36]

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/offline_resource_throttle.cc (right):

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/offline_resource_throttle.cc:157:
service_worker_completion_callback_.Cancel();
On 2014/12/16 08:16:49, falken wrote:
> Would it improve readability to set pending_callbacks_ = 0 here and above,
then
> DCHECK(!pending_callbacks_) in WillStartRequest, and then use
> ++pending_callbacks_ there instead of setting to 1 and 2 directly? Just a
> suggestion, WDYT.

Done, kinda

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/offline_resource_throttle.h (right):

https://codereview.chromium.org/810583002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/offline_resource_throttle.h:47: // A callback to
tell if an appcache exists.
On 2014/12/16 08:16:49, falken wrote:
> This comment should generalized now that SW also exists.

i removed the comments since they were just stating the obvious

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:202: static
void DidFindRegistrationForDocument(
On 2014/12/16 08:16:49, falken wrote:
> nit: chromium codebase tends to put these in unnamed namespace instead of as
> static functions

True, but this class file isn't following that convention atm, so when in
rome...

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:217: if
(!url.SchemeIsSecure()) {
On 2014/12/16 08:16:49, falken wrote:
> Is this needed? Can FindRegistrationForDocument return true for a non-secure
> URL?

Done

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:222: // check
for 3rd party cookie usage.
On 2014/12/16 08:16:50, falken wrote:
> I'm wondering what is the consequence of not doing this check? I guess the
> offline interstitial doesn't get displayed when it could, but later on the
> request handler fails and the user sees some offline message then... which
> doesn't seem so bad.

I've removed the todo. They can't be registered when !AllowServiceWorker, so
Find would be expected to find nothing. There's a corner case where the setting
is changed sometime after a registration is done, but as you noticed the failure
modes are mild.

> Why does Chrome OS have this special offline page? 

Idk.

[falken, 2015-01-09 02:27:44]

+oshima for background info

lgtm

I hope we can add regression tests for this but I see it's painful if the
throttle doesn't have existing tests. Maybe open a new bug or put a comment in
the current one?

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:202: static
void DidFindRegistrationForDocument(
On 2015/01/09 01:47:36, michaeln wrote:
> On 2014/12/16 08:16:49, falken wrote:
> > nit: chromium codebase tends to put these in unnamed namespace instead of as
> > static functions
> 
> True, but this class file isn't following that convention atm, so when in
> rome...

oops didn't notice, sorry

https://codereview.chromium.org/810583002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:222: // check
for 3rd party cookie usage.
On 2015/01/09 01:47:36, michaeln wrote:
> On 2014/12/16 08:16:50, falken wrote:
> > I'm wondering what is the consequence of not doing this check? I guess the
> > offline interstitial doesn't get displayed when it could, but later on the
> > request handler fails and the user sees some offline message then... which
> > doesn't seem so bad.
> 
> I've removed the todo. They can't be registered when !AllowServiceWorker, so
> Find would be expected to find nothing. There's a corner case where the
setting
> is changed sometime after a registration is done, but as you noticed the
failure
> modes are mild.
> 
> > Why does Chrome OS have this special offline page? 
> 
> Idk.

+oshima: do you know why this is CrOS-specific?

[oshima, 2015-01-09 16:28:53]

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

[oshima, 2015-01-09 16:28:54]

+rdsmith@, ellyjones@


A bit of history: the interstitial based offline page predates online/offline
event in html5, and it is added to address ChromeOS specific issue (network
connection may not be established at the time of login).

I thought all OSes are now using the same offline detection and reload
mechanism.

rdsmith@, ellyjones@, any idea why this is still used on ChromeOS (only)?

[Randy Smith (Not in Mondays), 2015-01-09 16:56:47]

On 2015/01/09 16:28:54, oshima wrote:
> +rdsmith@, ellyjones@
> 
> 
> A bit of history: the interstitial based offline page predates online/offline
> event in html5, and it is added to address ChromeOS specific issue (network
> connection may not be established at the time of login).
> 
> I thought all OSes are now using the same offline detection and reload
> mechanism.
> 
> rdsmith@, ellyjones@, any idea why this is still used on ChromeOS (only)?

We had originally planned to switch ChromeOS over to the new system once we had
data the new system was working well, but that got dropped on the floor for
reasons that have a lot to do with me getting distracted.  Unfortunately, it's
not on anyone's road map at the moment; it just should be done.  Mea Culpa :-{.

[michaeln, 2015-01-10 01:09:11]

michaeln@chromium.org changed reviewers:
+ jochen@chromium.org

[michaeln, 2015-01-10 01:10:14]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2015-01-10 01:11:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/810583002/100001

[commit-bot: I haz the power, 2015-01-10 01:51:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-10 01:54:45]

Patchset 6 (id:??) landed as
https://crrev.com/463cbca202c52fcae7ef7f05cd957d2f8d517bd5
Cr-Commit-Position: refs/heads/master@{#310937}