ServiceWorker: Strip away fragments from document url

ServiceWorker: Strip away fragments from document url

Fragments (e.g. "#hoge") in document url should be stripped before it's passed
to the SW because it's not necessary to match registrations.


BUG=379048
TEST=manual (open a controlled page whose URL contains fragments)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274479

[nhiroki, 2014-05-30 08:09:54]

Hi, can you take a look? Thanks!

[michaeln, 2014-05-30 23:38:47]

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

https://codereview.chromium.org/308053003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:57:
const GURL& stripped_url(net::SimplifyUrlForRequest(request->url()));
const GURL&, doesn't look right here

i'd put this line inside the PrepareForMainResource method to be closer to where
its used, that way it would be more clear that this value is not used for the
RequestJob

https://codereview.chromium.org/308053003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:86:
context_->storage()->FindRegistrationForDocument(
maybe add some DCHECKs to set_document_url(url) and FindRegistrationForDocument

[nhiroki, 2014-06-02 06:26:08]

Updated. Can you take another look? Thanks!

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

https://codereview.chromium.org/308053003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:57:
const GURL& stripped_url(net::SimplifyUrlForRequest(request->url()));
On 2014/05/30 23:38:47, michaeln wrote:
> const GURL&, doesn't look right here

Ah, it's a slip of the finger. Good catch!

> i'd put this line inside the PrepareForMainResource method to be closer to
where
> its used, that way it would be more clear that this value is not used for the
> RequestJob

Done.

https://codereview.chromium.org/308053003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:86:
context_->storage()->FindRegistrationForDocument(
On 2014/05/30 23:38:47, michaeln wrote:
> maybe add some DCHECKs to set_document_url(url) and
FindRegistrationForDocument

Added.

[michaeln, 2014-06-02 19:00:20]

lgtm!

[nhiroki, 2014-06-02 22:12:05]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2014-06-02 22:12:19]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2014-06-03 01:40:43]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-06-03 01:42:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/308053003/80001

[commit-bot: I haz the power, 2014-06-03 03:14:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-03 04:48:39]

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

[commit-bot: I haz the power, 2014-06-03 04:48:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[nhiroki, 2014-06-03 04:57:36]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-06-03 04:59:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/308053003/80001

[commit-bot: I haz the power, 2014-06-03 06:58:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-03 12:13:51]

Change committed as 274479