[Extension ServiceWorkers] Allow service worker registration

[Extension ServiceWorkers] Allow service worker registration

change SchemeisHTTPOrHTTPS checks to OriginCanAccessServiceWorker in service worker files.
Service Workers require a secure origin, such as HTTPS.  chrome-extension:// pages are not HTTP/HTTPS, but are secure so this change allows extensions to register a Service Worker.

Added an OriginCanAccessServiceWorker function in origin_util and replaced the SecureSchemeAndOriginSet with CustomSchemeAndOriginSet to add service worker schemes.

Also included a function for extensions to have response codes of 200 in extension protocols file.
A response code in the 200s (successful HTTP request) is required to install the Service Worker
(ServiceWorkerWriteToCacheJob::OnResponseStarted, line 630).  Extensions returned a response code of -1 because it was not overridden.  A GetResponseCode function was written in extensions_protocol.cc in the URLRequestExtensionJob class.

Added testing for fetch events.

BUG=501569
Committed: https://crrev.com/1db36fd5516b2b067c05d31a2736e6dfd90fa215
Cr-Commit-Position: refs/heads/master@{#340908}

[annekao, 2015-06-17 21:55:23]

annekao@google.com changed reviewers:
+ rdevlin.cronin@chromium.org

[annekao, 2015-07-13 23:20:27]

Due to the previous discussion with the URL scheme checks
(https://codereview.chromium.org/1191793003/), rather than just check
IsOriginSecure like in Patch#1, I tried to deviate minimally from the original
check and added an or SchemeIs("chrome-extension") to what was already there.

I'm wondering if it would make more sense to add a function in the GURL class or
if it's OK the way it is.

This patch would allow service workers to register successfully

[Devlin, 2015-07-14 15:47:36]

On 2015/07/13 23:20:27, annekao wrote:
> Due to the previous discussion with the URL scheme checks
> (https://codereview.chromium.org/1191793003/), rather than just check
> IsOriginSecure like in Patch#1, I tried to deviate minimally from the original
> check and added an or SchemeIs("chrome-extension") to what was already there.

Minor deviation is always good, but layering principles take priority. :)

> I'm wondering if it would make more sense to add a function in the GURL class
or
> if it's OK the way it is.

GURL is too low-level a place to put this.  I think there should be a helper
method, but probably not on GURL itself.

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

https://codereview.chromium.org/1192013003/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:56: return
(url.SchemeIsHTTPOrHTTPS() || url.SchemeIs("chrome-extension")) &&
This is a layering violation - we can't really mention "chrome-extension" (or
anything like it) anywhere in /content.

I think we'll have to do something similar to what we did for blink, and have a
set of origins that allow service workers.  We do the same thing for secure
origins, so if you look at the implementation of IsOriginSecure, that should be
a good guess.  My initial suggestion would be to move
OriginCanAccessServiceWorkers into origin_util, and set up a similar
registration to secure schemes for service workers.

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

https://codereview.chromium.org/1192013003/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_request_handler.cc:67: if
(!request->url().SchemeIsHTTPOrHTTPS() &&
ditto.

https://codereview.chromium.org/1192013003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/1192013003/diff/20001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:198: int GetResponseCode() const
override { return 200; }
The fact that this always returns 200 looks a little odd at first, since it
essentially guarantees success.  This could be correct, though, since we only
create the job if the file exists (we handle this in the "MaybeCreateJob()"
method).  Can you add a comment explaining that, so that people don't think it's
a bug that this always returns success?

[Devlin, 2015-07-14 15:47:50]

Oh, also - I think the test will need to be updated with this patch, right?

[annekao, 2015-07-15 17:56:45]

Patchset #3 (id:40001) has been deleted

[annekao, 2015-07-15 18:20:05]

Added a similar function as IsOriginSecure to origin_utils.

Updated the test since service workers can now be registered.

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

https://codereview.chromium.org/1192013003/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:56: return
(url.SchemeIsHTTPOrHTTPS() || url.SchemeIs("chrome-extension")) &&
On 2015/07/14 15:47:36, Devlin wrote:
> This is a layering violation - we can't really mention "chrome-extension" (or
> anything like it) anywhere in /content.
> 
> I think we'll have to do something similar to what we did for blink, and have
a
> set of origins that allow service workers.  We do the same thing for secure
> origins, so if you look at the implementation of IsOriginSecure, that should
be
> a good guess.  My initial suggestion would be to move
> OriginCanAccessServiceWorkers into origin_util, and set up a similar
> registration to secure schemes for service workers.

Done.

https://codereview.chromium.org/1192013003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/1192013003/diff/20001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:198: int GetResponseCode() const
override { return 200; }
On 2015/07/14 15:47:36, Devlin wrote:
> The fact that this always returns 200 looks a little odd at first, since it
> essentially guarantees success.  This could be correct, though, since we only
> create the job if the file exists (we handle this in the "MaybeCreateJob()"
> method).  Can you add a comment explaining that, so that people don't think
it's
> a bug that this always returns success?

Done.

[Devlin, 2015-07-15 19:47:48]

https://codereview.chromium.org/1192013003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:27: ASSERT_EQ("success",
listener.message());
Awesome!  Can we add at least one test for the fetch api?

https://codereview.chromium.org/1192013003/diff/60001/content/common/origin_u...
File content/common/origin_util.cc (right):

https://codereview.chromium.org/1192013003/diff/60001/content/common/origin_u...
content/common/origin_util.cc:35: class ServiceWorkerSchemeSet {
Rather than having a full separate class, let's just expand the current one. 
Maybe rename SecureSchemeAndOriginSet to CustomSchemesAndOrigins (someone may
have a better suggestion), and have it contain a std::set secure_schemes_,
std::set secure_origins_, and std::set service_worker_origins_.

https://codereview.chromium.org/1192013003/diff/60001/content/public/common/o...
File content/public/common/origin_util.h (right):

https://codereview.chromium.org/1192013003/diff/60001/content/public/common/o...
content/public/common/origin_util.h:25: // http (localhost only), https, or
chrome-extension.
Even in comments, mentioning chrome-extension is a layering violation.  Maybe
change "or chrome-extension" to be "or a custom-set secure scheme".

[Devlin, 2015-07-20 21:01:39]

How's this one coming along, Anne?

[annekao, 2015-07-20 21:04:19]

On 2015/07/20 21:01:39, Devlin wrote:
> How's this one coming along, Anne?

Funny you ask, just finished the test!  Took a while to properly test fetch
events (it's required to reload the page/extension), but I got it.  Will upload
soon.

[annekao, 2015-07-20 21:14:48]

Patchset #4 (id:80001) has been deleted

[annekao, 2015-07-20 21:27:14]

Patchset #4 (id:100001) has been deleted

[annekao, 2015-07-20 21:29:43]

Patchset #4 (id:120001) has been deleted

[annekao, 2015-07-20 21:36:56]

https://codereview.chromium.org/1192013003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:27: ASSERT_EQ("success",
listener.message());
On 2015/07/15 19:47:48, Devlin wrote:
> Awesome!  Can we add at least one test for the fetch api?

Done.

https://codereview.chromium.org/1192013003/diff/60001/content/common/origin_u...
File content/common/origin_util.cc (right):

https://codereview.chromium.org/1192013003/diff/60001/content/common/origin_u...
content/common/origin_util.cc:35: class ServiceWorkerSchemeSet {
On 2015/07/15 19:47:48, Devlin wrote:
> Rather than having a full separate class, let's just expand the current one. 
> Maybe rename SecureSchemeAndOriginSet to CustomSchemesAndOrigins (someone may
> have a better suggestion), and have it contain a std::set secure_schemes_,
> std::set secure_origins_, and std::set service_worker_origins_.

Done.  Also had to update the function name in secure_origin_whitelist_unittest.

https://codereview.chromium.org/1192013003/diff/60001/content/public/common/o...
File content/public/common/origin_util.h (right):

https://codereview.chromium.org/1192013003/diff/60001/content/public/common/o...
content/public/common/origin_util.h:25: // http (localhost only), https, or
chrome-extension.
On 2015/07/15 19:47:48, Devlin wrote:
> Even in comments, mentioning chrome-extension is a layering violation.  Maybe
> change "or chrome-extension" to be "or a custom-set secure scheme".

Done.

[Devlin, 2015-07-20 21:54:47]

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:53: // Page must refresh for
service worker to take control
nitty nit: Comments should usually be sentences, so this should end in a '.'
(there are exceptions if the comment is only a word or two, documenting e.g. a
function argument).

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:54:
ReloadExtension(GetSingleLoadedExtension()->id());
Hmm... Reloading the extension might cause the page to be reloaded, but it's a
little different.  Let's just refresh the page using
content::NavigationController::Reload().  Doing so will need a little bit of
plumbing, so the full line will look like:
browser()->tab_strip_model()->GetActiveWebContents()->GetController()->Reload(true);

https://codereview.chromium.org/1192013003/diff/140001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1192013003/diff/140001/content/public/common/...
content/public/common/content_client.h:144: virtual void
AddServiceWorkerSchemes(std::set<std::string>* schemes) {}
Add a comment here, something like:
// Gives the embedder a chance to register additional schemes that
// should be allowed to register service workers. Only secure and
// trustworthy schemes should be added.

[annekao, 2015-07-20 22:59:38]

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:54:
ReloadExtension(GetSingleLoadedExtension()->id());
On 2015/07/20 21:54:47, Devlin wrote:
> Hmm... Reloading the extension might cause the page to be reloaded, but it's a
> little different.  Let's just refresh the page using
> content::NavigationController::Reload().  Doing so will need a little bit of
> plumbing, so the full line will look like:
>
browser()->tab_strip_model()->GetActiveWebContents()->GetController()->Reload(true);

Doesn't seem like this reloads the background page which has the service worker
registered.  Is there another way that might?

[Devlin, 2015-07-20 23:04:36]

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:54:
ReloadExtension(GetSingleLoadedExtension()->id());
On 2015/07/20 22:59:38, annekao wrote:
> On 2015/07/20 21:54:47, Devlin wrote:
> > Hmm... Reloading the extension might cause the page to be reloaded, but it's
a
> > little different.  Let's just refresh the page using
> > content::NavigationController::Reload().  Doing so will need a little bit of
> > plumbing, so the full line will look like:
> >
>
browser()->tab_strip_model()->GetActiveWebContents()->GetController()->Reload(true);
> 
> Doesn't seem like this reloads the background page which has the service
worker
> registered.  Is there another way that might?

Whoops! I missed the part that this is the background page.  Let's change this
to be a non-background page part of the extension (i.e., just add another page
in the same file, perhaps called "page.html"), and navigate to that directly in
the test.

The fact that extensions can register service workers for their background page
is pretty interesting, and perhaps undesirable (though maybe we should allow it
and just lump it in with other potential shooting-yourself-in-the-foot moves). 
I'll have to think about that more.

[annekao, 2015-07-21 01:05:45]

On 2015/07/20 23:04:36, Devlin wrote:
>
https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
> File chrome/browser/extensions/service_worker_apitest.cc (right):
> 
>
https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
> chrome/browser/extensions/service_worker_apitest.cc:54:
> ReloadExtension(GetSingleLoadedExtension()->id());
> On 2015/07/20 22:59:38, annekao wrote:
> > On 2015/07/20 21:54:47, Devlin wrote:
> > > Hmm... Reloading the extension might cause the page to be reloaded, but
it's
> a
> > > little different.  Let's just refresh the page using
> > > content::NavigationController::Reload().  Doing so will need a little bit
of
> > > plumbing, so the full line will look like:
> > >
> >
>
browser()->tab_strip_model()->GetActiveWebContents()->GetController()->Reload(true);
> > 
> > Doesn't seem like this reloads the background page which has the service
> worker
> > registered.  Is there another way that might?
> 
> Whoops! I missed the part that this is the background page.  Let's change this
> to be a non-background page part of the extension (i.e., just add another page
> in the same file, perhaps called "page.html"), and navigate to that directly
in
> the test.
> 
> The fact that extensions can register service workers for their background
page
> is pretty interesting, and perhaps undesirable (though maybe we should allow
it
> and just lump it in with other potential shooting-yourself-in-the-foot moves).

> I'll have to think about that more.

Hm, I registered the service worker on a separate html, but I couldn't get
Reload() to work.  The result is still the same as before calling the function. 
ReloadExtension() still works though.

[annekao, 2015-07-21 18:04:09]

Patchset #5 (id:160001) has been deleted

[annekao, 2015-07-21 18:05:18]

Looks like Reload() does work, but has to be done twice (not sure why).

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:53: // Page must refresh for
service worker to take control
On 2015/07/20 21:54:47, Devlin wrote:
> nitty nit: Comments should usually be sentences, so this should end in a '.'
> (there are exceptions if the comment is only a word or two, documenting e.g. a
> function argument).

Done.

https://codereview.chromium.org/1192013003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:54:
ReloadExtension(GetSingleLoadedExtension()->id());
On 2015/07/20 21:54:47, Devlin wrote:
> Hmm... Reloading the extension might cause the page to be reloaded, but it's a
> little different.  Let's just refresh the page using
> content::NavigationController::Reload().  Doing so will need a little bit of
> plumbing, so the full line will look like:
>
browser()->tab_strip_model()->GetActiveWebContents()->GetController()->Reload(true);

Done.

https://codereview.chromium.org/1192013003/diff/140001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1192013003/diff/140001/content/public/common/...
content/public/common/content_client.h:144: virtual void
AddServiceWorkerSchemes(std::set<std::string>* schemes) {}
On 2015/07/20 21:54:47, Devlin wrote:
> Add a comment here, something like:
> // Gives the embedder a chance to register additional schemes that
> // should be allowed to register service workers. Only secure and
> // trustworthy schemes should be added.

Done.

[annekao, 2015-07-21 21:59:56]

On 2015/07/21 18:05:18, annekao wrote:
> Looks like Reload() does work, but has to be done twice (not sure why).
> 

So the issue is that when it reloads for the first time, it re-registers the
service worker when it should already have one. Then, the second reload updates
the pages.  Very rarely it doesn't re-register and actually updates on the first
reload.

[annekao, 2015-07-21 22:29:36]

Patchset #6 (id:200001) has been deleted

[annekao, 2015-07-21 22:34:08]

Took out runTests in fetch/page.js which didn't allow a service worker to stay
registered.

Also added a page.html and page.js to register/.  page.js registers a service
worker instead of the background page.

[Devlin, 2015-07-21 22:42:28]

https://codereview.chromium.org/1192013003/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:55: ASSERT_EQ("No Fetch
Event yet.", output);
These ASSERT_EQ should probably be EXPECT_EQ.

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/fetch/manifest.json
(right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/fetch/manifest.json:7:
"tabs"
this is kinda wonky indentation.  You can just put it all on one line, like
"permissions": ["tabs"],

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/fetch/page.js (right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/fetch/page.js:18:
log('ServiceWorker registration failed: ' + errorMsg);
chrome.test.fail();

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/fetch/sw.js (right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/fetch/sw.js:6:
event.respondWith(new Response('Caught a fetch!'));
nit: 2-space indentation.

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/register/manifest.json
(right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/register/manifest.json:6:
"permissions": [
The "register" extension shouldn't need to change, right?

[annekao, 2015-07-21 22:48:20]

On 2015/07/21 22:42:28, Devlin wrote:
>
https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
> chrome/test/data/extensions/api_test/service_worker/register/manifest.json:6:
> "permissions": [
> The "register" extension shouldn't need to change, right?

The reason I added the tabs permission is so that it would register the service
worker somewhere other than the background page.

[Devlin, 2015-07-21 22:52:53]

On 2015/07/21 22:48:20, annekao wrote:
> On 2015/07/21 22:42:28, Devlin wrote:
> >
>
https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
> >
chrome/test/data/extensions/api_test/service_worker/register/manifest.json:6:
> > "permissions": [
> > The "register" extension shouldn't need to change, right?
> 
> The reason I added the tabs permission is so that it would register the
service
> worker somewhere other than the background page.

Ah, right.  Ignore that one.

[annekao, 2015-07-21 22:55:45]

https://codereview.chromium.org/1192013003/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:55: ASSERT_EQ("No Fetch
Event yet.", output);
On 2015/07/21 22:42:27, Devlin wrote:
> These ASSERT_EQ should probably be EXPECT_EQ.

Done.

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/fetch/manifest.json
(right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/fetch/manifest.json:7:
"tabs"
On 2015/07/21 22:42:28, Devlin wrote:
> this is kinda wonky indentation.  You can just put it all on one line, like
> "permissions": ["tabs"],

Done. It's actually like that on the developer page haha.

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/fetch/page.js (right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/fetch/page.js:18:
log('ServiceWorker registration failed: ' + errorMsg);
On 2015/07/21 22:42:28, Devlin wrote:
> chrome.test.fail();

Done.

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/fetch/sw.js (right):

https://codereview.chromium.org/1192013003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/fetch/sw.js:6:
event.respondWith(new Response('Caught a fetch!'));
On 2015/07/21 22:42:28, Devlin wrote:
> nit: 2-space indentation.

Done.

[Devlin, 2015-07-21 22:59:27]

LGTM. Now you'll need one (or two) content/ OWNERS.  I'd recommend falken@ for
content/browser/service_worker, since he's more familiar with it, and then
someone from content/OWNERS for the other files.

[annekao, 2015-07-21 23:16:44]

Patchset #8 (id:260001) has been deleted

[annekao, 2015-07-21 23:19:47]

annekao@google.com changed reviewers:
+ falken@chromium.org, kinuko@chromium.org

[annekao, 2015-07-21 23:25:04]

Hi falken@ and kinuko@, please take a look at the changes in the content/ files.

I changed SchemeisHTTPOrHTTPS checks to OriginCanAccessServiceWorker in service
worker files so chrome-extension can be an allowed scheme.

The function is in origin_util (where IsOriginSecure is from) and I replaced the
SecureSchemeAndOriginSet with CustomSchemeAndOriginSet to add a service worker
scheme set.

[falken, 2015-07-23 00:14:07]

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
content/public/common/content_client.h:146: // trustworthy schemes should be
added.
This comment makes the function feel redundant with AddSecureSchemesAndOrigins,
especially when it says only secure schemes should be added. Why can't the
embedder just use that function?

[Devlin, 2015-07-23 02:31:02]

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
content/public/common/content_client.h:146: // trustworthy schemes should be
added.
On 2015/07/23 00:14:07, falken wrote:
> This comment makes the function feel redundant with
AddSecureSchemesAndOrigins,
> especially when it says only secure schemes should be added. Why can't the
> embedder just use that function?

Just because it's not a 1:1 between secure schemes and schemes that should allow
a service worker.  For instance, chrome:// schemes are secure, but shouldn't
(yet) allow service workers.  Similarly for chrome-extension-resource scheme. 
And there are the other secure schemes that don't allow service workers, like
file schemes.  I think most of the discussion in the previous patch led to the
conclusion that having "secure" and "service-worker-able" should be two
different sets (though, in theory, service worker is a direct subset of secure).

[falken, 2015-07-23 07:21:26]

lgtm

Nit in the CL description:
> "A response code in the 200s (successful HTTP request) is required for the
Service Worker to write to the cache
(ServiceWorkerWriteToCacheJob::OnResponseStarted, line 630)"

This class writes the Service Worker itself to the "cache" where cache really
means the thing storing service worker scripts. Usually when we say "cache" in
SW land it's the "Cache Storage API" which is totally different. I'd revise it
to "... is required to install the Service Worker
(ServiceWorkerWriteToCacheJob::OnResponseStarted, line 630)"

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
content/public/common/content_client.h:146: // trustworthy schemes should be
added.
On 2015/07/23 02:31:02, Devlin wrote:
> On 2015/07/23 00:14:07, falken wrote:
> > This comment makes the function feel redundant with
> AddSecureSchemesAndOrigins,
> > especially when it says only secure schemes should be added. Why can't the
> > embedder just use that function?
> 
> Just because it's not a 1:1 between secure schemes and schemes that should
allow
> a service worker.  For instance, chrome:// schemes are secure, but shouldn't
> (yet) allow service workers.  Similarly for chrome-extension-resource scheme. 
> And there are the other secure schemes that don't allow service workers, like
> file schemes.  I think most of the discussion in the previous patch led to the
> conclusion that having "secure" and "service-worker-able" should be two
> different sets (though, in theory, service worker is a direct subset of
secure).

Ah that makes perfect sense.

[annekao, 2015-07-24 21:36:24]

kinuko@, please take a look.

[kinuko, 2015-07-26 14:09:36]

lgtm

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
File content/common/origin_util.cc (right):

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
content/common/origin_util.cc:17: class CustomSchemeAndOriginSet {
nit: 'custom scheme and origin set' doesn't really tell what it is,
'SchemeAndOriginWhitelist' or something might be better?

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
content/common/origin_util.cc:65: url.GetOrigin()))
nit: when condition line spans multiple lines using { } braces is more common
even if the body is 1 line

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
content/common/origin_util.cc:77: return true;
ditto

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
File content/public/common/origin_util.h (right):

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
content/public/common/origin_util.h:26: void CONTENT_EXPORT
ResetCustomSchemesAndOriginsForTesting();
ditto for naming

[annekao, 2015-07-27 17:02:13]

Fixed CL description and nits

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
File content/common/origin_util.cc (right):

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
content/common/origin_util.cc:17: class CustomSchemeAndOriginSet {
On 2015/07/26 14:09:35, kinuko wrote:
> nit: 'custom scheme and origin set' doesn't really tell what it is,
> 'SchemeAndOriginWhitelist' or something might be better?

Done.

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
content/common/origin_util.cc:65: url.GetOrigin()))
On 2015/07/26 14:09:35, kinuko wrote:
> nit: when condition line spans multiple lines using { } braces is more common
> even if the body is 1 line

Done.

https://codereview.chromium.org/1192013003/diff/280001/content/common/origin_...
content/common/origin_util.cc:77: return true;
On 2015/07/26 14:09:35, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
File content/public/common/origin_util.h (right):

https://codereview.chromium.org/1192013003/diff/280001/content/public/common/...
content/public/common/origin_util.h:26: void CONTENT_EXPORT
ResetCustomSchemesAndOriginsForTesting();
On 2015/07/26 14:09:35, kinuko wrote:
> ditto for naming

Done.

[annekao, 2015-07-27 19:24:05]

The ServiceWorkerRequestHandler unit test failed due to the change from
SchemeIsHTTPOrHTTPS to OriginCanAccessServiceWorkers.  Since the new function
checks for IsOriginSecure, some of the tests failed.  The tests were updated for
the new expected results.

falken@, will this cause any issues?

[falken, 2015-07-28 01:18:16]

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

[falken, 2015-07-28 01:18:17]

To be honest, I'm pretty confused by the original tests, I'd like some comments.
horo do you have any insight (see below)?

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler_unittest.cc
(right):

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:120:
TEST_F(ServiceWorkerRequestHandlerTest, InitializeHandler) {
Can you add comments like "Cannot initialize a handler for non-secure origins".
(and maybe group HTTP and FTP one together).

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:129:
RESOURCE_TYPE_MAIN_FRAME));
I believe this check is now redundant with the GET one for non-secure origins,
we can just remove it. The OPTIONS tests were added in
https://codereview.chromium.org/716083003 (a temporary workaround) and later
changed in https://codereview.chromium.org/716083003. We can keep the OPTIONS
one for secure origins, with a comment "OPTIONS is also supported" perhaps link
to crbug.com/434660 (unfortunately, crbug is not loading for me now so I can't
verify it'd be useful)

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:136:
EXPECT_STREQ("", provider_host_->document_url().spec().c_str());
I guess a comment for this block "Cannot initialize a handler for non-secure
origins" is also good. I'm totally confused by the original expectation, the
InitializeHandlerCheck fails but the provider host doc URL changes?? I'm not
sure what this is testing: +horo.

[falken, 2015-07-28 11:43:35]

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler_unittest.cc
(right):

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:136:
EXPECT_STREQ("", provider_host_->document_url().spec().c_str());
On 2015/07/28 01:18:17, falken wrote:
> I guess a comment for this block "Cannot initialize a handler for non-secure
> origins" is also good. I'm totally confused by the original expectation, the
> InitializeHandlerCheck fails but the provider host doc URL changes?? I'm not
> sure what this is testing: +horo.

OK I took another look and understand this now,
ServiceWorkerRequestHandler::InitializeHandler will (on ToT code) reach the
skip_service_worker step and set the document URL without creating a handler. So
I think all that's needed here is a comment like "Cannot initialize a handler
for non-secure origins."

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:144:
"http://host/scope/doc", "GET", true, RESOURCE_TYPE_SUB_FRAME));
Probably here too.

[annekao, 2015-07-28 16:37:21]

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler_unittest.cc
(right):

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:120:
TEST_F(ServiceWorkerRequestHandlerTest, InitializeHandler) {
On 2015/07/28 01:18:17, falken wrote:
> Can you add comments like "Cannot initialize a handler for non-secure
origins".
> (and maybe group HTTP and FTP one together).

Done.

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:129:
RESOURCE_TYPE_MAIN_FRAME));
On 2015/07/28 01:18:17, falken wrote:
> I believe this check is now redundant with the GET one for non-secure origins,
> we can just remove it. The OPTIONS tests were added in
> https://codereview.chromium.org/716083003 (a temporary workaround) and later
> changed in https://codereview.chromium.org/716083003. We can keep the OPTIONS
> one for secure origins, with a comment "OPTIONS is also supported" perhaps
link
> to crbug.com/434660 (unfortunately, crbug is not loading for me now so I can't
> verify it'd be useful)

Done.

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:136:
EXPECT_STREQ("", provider_host_->document_url().spec().c_str());
On 2015/07/28 11:43:35, falken wrote:
> On 2015/07/28 01:18:17, falken wrote:
> > I guess a comment for this block "Cannot initialize a handler for non-secure
> > origins" is also good. I'm totally confused by the original expectation, the
> > InitializeHandlerCheck fails but the provider host doc URL changes?? I'm not
> > sure what this is testing: +horo.
> 
> OK I took another look and understand this now,
> ServiceWorkerRequestHandler::InitializeHandler will (on ToT code) reach the
> skip_service_worker step and set the document URL without creating a handler.
So
> I think all that's needed here is a comment like "Cannot initialize a handler
> for non-secure origins."

Done.

https://codereview.chromium.org/1192013003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler_unittest.cc:144:
"http://host/scope/doc", "GET", true, RESOURCE_TYPE_SUB_FRAME));
On 2015/07/28 11:43:35, falken wrote:
> Probably here too.

Done.

[annekao, 2015-07-28 19:11:01]

The CQ bit was checked by annekao@google.com

[annekao, 2015-07-28 19:11:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, falken@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1192013003/#ps340001
(title: "Added comments to test")

[commit-bot: I haz the power, 2015-07-28 19:11:11]

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

[commit-bot: I haz the power, 2015-07-28 19:21:24]

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

[commit-bot: I haz the power, 2015-07-28 19:21:24]

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

[annekao, 2015-07-28 19:54:54]

annekao@google.com changed reviewers:
+ thestig@chromium.org

[annekao, 2015-07-28 19:57:18]

thestig@, please take a look at the changes in the chrome/common files.  I added
an additional function AddServiceWorkerSchemes which will add the
chrome-extension scheme.  Also, secure_origin_whitelist_unittest was updated to
match the new function name in origin_util.

[Lei Zhang, 2015-07-28 21:11:02]

chrome/common lgtm

[annekao, 2015-07-28 21:32:18]

The CQ bit was checked by annekao@google.com

[commit-bot: I haz the power, 2015-07-28 21:32:30]

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

[commit-bot: I haz the power, 2015-07-28 23:04:15]

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

[commit-bot: I haz the power, 2015-07-28 23:04:16]

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

[annekao, 2015-07-29 17:01:01]

The CQ bit was checked by annekao@google.com

[commit-bot: I haz the power, 2015-07-29 17:01:35]

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

[commit-bot: I haz the power, 2015-07-29 17:09:23]

Committed patchset #11 (id:340001)

[commit-bot: I haz the power, 2015-07-29 17:10:00]

Patchset 11 (id:??) landed as
https://crrev.com/1db36fd5516b2b067c05d31a2736e6dfd90fa215
Cr-Commit-Position: refs/heads/master@{#340908}