Set skip_service_worker in ServiceWorkerControlleeRequestHandler only for subresource requests.

Set skip_service_worker in ServiceWorkerControlleeRequestHandler only for subresource requests.

https://codereview.chromium.org/1274973002/ accidentally changed the
Service Worker behavior of handling redirected page requests.

If we set skip_service_worker for page requests, the final location of redirects
will not correctly be handled by Service Worker.

So this cl change to set |skip_service_worker| only for subresource requests.

https://codereview.chromium.org/1280243002 will add LayoutTests for fallbacked subresource requests.

BUG=517364, 512764, 518713
Committed: https://crrev.com/bc0572e34764db29e996a95470a2339b6f3eb242
Cr-Commit-Position: refs/heads/master@{#342722}

[horo, 2015-08-10 03:46:09]

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

[horo, 2015-08-10 03:46:09]

falken@
Could you please review this?

[falken, 2015-08-10 04:56:15]

I see there's layout tests for the subresource case in
https://codereview.chromium.org/1280243002 (this could be added to the CL
description). Do you plan to add layout tests for the main resource case? If you
want to split the work, I can add my tests from
https://code.google.com/p/chromium/issues/detail?id=495384

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

https://codereview.chromium.org/1280133002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_controllee_request_handler.cc:122:
skip_service_worker_ = true;
Can you factor these out to a helper function so we don't have duplicated
code/comment.

[horo, 2015-08-10 05:43:41]

> I see there's layout tests for the subresource case in
> https://codereview.chromium.org/1280243002 (this could be added to the CL
> description). Do you plan to add layout tests for the main resource case? If
you
> want to split the work, I can add my tests from
> https://code.google.com/p/chromium/issues/detail?id=495384

I planned to create LayoutTest for it. But not yet.
I'd be happy if you create it :)

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

https://codereview.chromium.org/1280133002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_controllee_request_handler.cc:122:
skip_service_worker_ = true;
On 2015/08/10 04:56:15, falken wrote:
> Can you factor these out to a helper function so we don't have duplicated
> code/comment.

Done.

[falken, 2015-08-10 07:05:59]

lgtm

Don't you need to add the function to the .h file?

I'll do the main resource tests :)

https://codereview.chromium.org/1280133002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/1280133002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:278:
// |skip_service_worker_| and the request shoud not go to the service
nit: and -> because (?)
nit: shoud -> should

[horo, 2015-08-10 07:23:56]

Thank you (^^)

> Don't you need to add the function to the .h file?

Done.

https://codereview.chromium.org/1280133002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/1280133002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:278:
// |skip_service_worker_| and the request shoud not go to the service
On 2015/08/10 07:05:59, falken wrote:
> nit: and -> because (?)
> nit: shoud -> should

Done.

[horo, 2015-08-10 13:00:36]

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

[horo, 2015-08-10 13:00:37]

michaeln@
Could you please review this?

[michaeln, 2015-08-10 19:21:53]

lgtm, thnx!!!!

[horo, 2015-08-10 22:51:34]

The CQ bit was checked by horo@chromium.org

[horo, 2015-08-10 22:51:35]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1280133002/#ps40001
(title: "add .h file. fix comment")

[commit-bot: I haz the power, 2015-08-10 22:52:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280133002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280133002/40001

[commit-bot: I haz the power, 2015-08-10 22:57:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-10 22:58:41]

Patchset 3 (id:??) landed as
https://crrev.com/bc0572e34764db29e996a95470a2339b6f3eb242
Cr-Commit-Position: refs/heads/master@{#342722}