Move Service Worker %2f path validation logic from browser into Blink (1)

Move Service Worker %2f path validation logic from browser into Blink (1)

This patch adds an interface by which the path validation logic can be
kept in Chromium but called from within Blink. It will be eventually
used to ensure that navigator.serviceWorker.register() rejects with a
TypeError instead of a DOMException (see spec).

1. (Chromium) This CL.
2. (Blink) https://codereview.chromium.org/1260003003/
3. (Chromium) https://codereview.chromium.org/1256833004/

BUG=513622
R=nhiroki,falken

Committed: https://crrev.com/86e6e3346b35b06badaaa0c7e4edc14151bf5c10
Cr-Commit-Position: refs/heads/master@{#342279}

[nhiroki, 2015-07-28 09:16:03]

https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
content/child/service_worker/web_service_worker_provider_impl.cc:83: return
!ServiceWorkerUtils::ContainsDisallowedCharacter(
This is a module dependency violation. We cannot include content/browser/* from
content/child/*. Can you move ServiceWorkerUtils into content/common/ which can
be accessed from content/{browser,child,renderer}/?

https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
File content/child/service_worker/web_service_worker_provider_impl.h (right):

https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
content/child/service_worker/web_service_worker_provider_impl.h:44: WTF::String*
error_message);
We cannot use WTF:: namespace in chromium. Can you replace WTF::String with
blink::WebString?

[jeremyarcher, 2015-07-29 02:29:48]

On 2015/07/28 09:16:03, nhiroki wrote:
>
https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
> File content/child/service_worker/web_service_worker_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
> content/child/service_worker/web_service_worker_provider_impl.cc:83: return
> !ServiceWorkerUtils::ContainsDisallowedCharacter(
> This is a module dependency violation. We cannot include content/browser/*
from
> content/child/*. Can you move ServiceWorkerUtils into content/common/ which
can
> be accessed from content/{browser,child,renderer}/?
> 
>
https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
> File content/child/service_worker/web_service_worker_provider_impl.h (right):
> 
>
https://codereview.chromium.org/1259213002/diff/1/content/child/service_worke...
> content/child/service_worker/web_service_worker_provider_impl.h:44:
WTF::String*
> error_message);
> We cannot use WTF:: namespace in chromium. Can you replace WTF::String with
> blink::WebString?

Fixed; PTAL

[nhiroki, 2015-07-29 09:09:37]

https://codereview.chromium.org/1259213002/diff/20001/content/child/service_w...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/1259213002/diff/20001/content/child/service_w...
content/child/service_worker/web_service_worker_provider_impl.cc:84: nullptr);
How about returning an error message provided by ContainsDC()? If we return it,
we don't have to compose a similar error message in SWContainer.

[jeremyarcher, 2015-07-31 05:50:38]

On 2015/07/29 09:09:37, nhiroki wrote:
>
https://codereview.chromium.org/1259213002/diff/20001/content/child/service_w...
> File content/child/service_worker/web_service_worker_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/1259213002/diff/20001/content/child/service_w...
> content/child/service_worker/web_service_worker_provider_impl.cc:84: nullptr);
> How about returning an error message provided by ContainsDC()? If we return
it,
> we don't have to compose a similar error message in SWContainer.

Mm, good call. How's this?

[nhiroki, 2015-07-31 06:07:05]

lgtm with a nit

https://codereview.chromium.org/1259213002/diff/40001/content/child/service_w...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/1259213002/diff/40001/content/child/service_w...
content/child/service_worker/web_service_worker_provider_impl.cc:84: std::string
error = "";
nit: you don't have to assign "" ("std::string error;" is ok)

[jeremyarcher, 2015-07-31 08:10:30]

On 2015/07/31 06:07:05, nhiroki wrote:
> lgtm with a nit
> 
>
https://codereview.chromium.org/1259213002/diff/40001/content/child/service_w...
> File content/child/service_worker/web_service_worker_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/1259213002/diff/40001/content/child/service_w...
> content/child/service_worker/web_service_worker_provider_impl.cc:84:
std::string
> error = "";
> nit: you don't have to assign "" ("std::string error;" is ok)

Fixed.

[jeremyarcher, 2015-07-31 08:11:50]

Patchset #5 (id:80001) has been deleted

[jeremyarcher, 2015-08-03 03:42:28]

The CQ bit was checked by jeremyarcher@google.com

[jeremyarcher, 2015-08-03 03:42:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1259213002/#ps60001
(title: "Ensure that upstream CL tests pass.")

[commit-bot: I haz the power, 2015-08-03 03:42:49]

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

[commit-bot: I haz the power, 2015-08-03 03:52:35]

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

[commit-bot: I haz the power, 2015-08-03 03:52:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jeremyarcher, 2015-08-03 03:55:59]

jeremyarcher@google.com changed reviewers:
+ kinuko@chromium.org

[falken, 2015-08-03 03:56:32]

https://codereview.chromium.org/1259213002/diff/60001/content/child/service_w...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/1259213002/diff/60001/content/child/service_w...
content/child/service_worker/web_service_worker_provider_impl.cc:89:
blink::WebString::fromUTF8(kServiceWorkerRegisterErrorPrefix + error);
There's some discussion on the Blink patch about this. I agree with nhiroki that
kServiceWorkerRegisterErrorPrefix probably shouldn't be here since this function
could conceivably be used outside of registration.

nit: I like braces for multiline body

[jeremyarcher, 2015-08-03 06:08:08]

On 2015/08/03 03:56:32, falken wrote:
>
https://codereview.chromium.org/1259213002/diff/60001/content/child/service_w...
> File content/child/service_worker/web_service_worker_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/1259213002/diff/60001/content/child/service_w...
> content/child/service_worker/web_service_worker_provider_impl.cc:89:
> blink::WebString::fromUTF8(kServiceWorkerRegisterErrorPrefix + error);
> There's some discussion on the Blink patch about this. I agree with nhiroki
that
> kServiceWorkerRegisterErrorPrefix probably shouldn't be here since this
function
> could conceivably be used outside of registration.
> 
> nit: I like braces for multiline body

Sounds good. Prefix removed and brackets added. (PTAL)

[nhiroki, 2015-08-05 06:25:49]

still lgtm

https://codereview.chromium.org/1259213002/diff/100001/content/child/service_...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/1259213002/diff/100001/content/child/service_...
content/child/service_worker/web_service_worker_provider_impl.cc:89: }
nit: This is no longer a multiline body, so you don't have to add braces :)

[falken, 2015-08-05 06:46:38]

lgtm 2

https://codereview.chromium.org/1259213002/diff/100001/content/common/service...
File content/common/service_worker/service_worker_utils.h (right):

https://codereview.chromium.org/1259213002/diff/100001/content/common/service...
content/common/service_worker/service_worker_utils.h:5: #ifndef
CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_UTILS_H_
This include guard should be CONTENT_COMMON... now.

https://codereview.chromium.org/1259213002/diff/100001/content/common/service...
content/common/service_worker/service_worker_utils.h:60: #endif  //
CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_UTILS_H_
and here

[jeremyarcher, 2015-08-05 07:06:02]

The CQ bit was checked by jeremyarcher@google.com

[jeremyarcher, 2015-08-05 07:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1259213002/#ps120001
(title: "Fix remaining nits.")

[commit-bot: I haz the power, 2015-08-05 07:06:21]

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

[nhiroki, 2015-08-05 07:11:05]

FYI: you need an owner review for
content/browser/navigator_connect/navigator_connect_service_worker_service_factory.cc

[jeremyarcher, 2015-08-05 07:15:27]

The CQ bit was unchecked by jeremyarcher@google.com

[jeremyarcher, 2015-08-06 01:54:54]

jeremyarcher@google.com changed reviewers:
+ mek@chromium.org

[jeremyarcher, 2015-08-06 01:54:55]

+mek@chromium.org

[Marijn Kruisselbrink, 2015-08-06 16:16:53]

On 2015/08/06 at 01:54:55, jeremyarcher wrote:
> +mek@chromium.org

lgtm

[jeremyarcher, 2015-08-07 01:03:51]

The CQ bit was checked by jeremyarcher@google.com

[commit-bot: I haz the power, 2015-08-07 01:06:14]

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

[commit-bot: I haz the power, 2015-08-07 03:05:29]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-08-07 03:06:29]

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