Cleanup of static lists of schemes & origins that are created at startup.

Cleanup of static lists of schemes & origins that are created at startup.

This in preparation for using this method to grab other schemes that we need to now share across the browser and renderer for PlzNavigate. This change does:
-combines AddAdditionalSchemes/AddSecureSchemesAndOrigins/AddServiceWorkerSchemes
-standardizes on std::string

BUG=627502
Review-Url: https://codereview.chromium.org/2622693002
Cr-Commit-Position: refs/heads/master@{#442776}
Committed: https://chromium.googlesource.com/chromium/src/+/e0dcd98dbb8363f64857345c317da57cde0588b3

[jam, 2017-01-09 19:29:27]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 19:30:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 19:42:43]

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

[commit-bot: I haz the power, 2017-01-09 19:42:43]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-01-09 20:07:23]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 20:08:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-09 20:29:35]

Description was changed from

==========
Cleanup of static lists of schemes & origins that are created at startup.

BUG=627502
==========

to

==========
Cleanup of static lists of schemes & origins that are created at startup.

This in preparation for using this method to grab other schemes that we need to
now share across the browser and renderer for PlzNavigate. This change does:
-combines
AddAdditionalSchemes/AddSecureSchemesAndOrigins/AddServiceWorkerSchemes
-standardizes on std::string

BUG=627502
==========

[jam, 2017-01-09 20:29:35]

jam@chromium.org changed reviewers:
+ brettw@chromium.org

[commit-bot: I haz the power, 2017-01-09 20:30:13]

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

[commit-bot: I haz the power, 2017-01-09 20:30:14]

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

[jam, 2017-01-09 20:33:34]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 20:34:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-09 20:39:03]

Patchset #4 (id:60001) has been deleted

[jam, 2017-01-09 20:39:04]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 20:39:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-09 21:16:48]

Patchset #3 (id:40001) has been deleted

[jam, 2017-01-09 21:16:55]

Patchset #2 (id:20001) has been deleted

[jam, 2017-01-09 21:17:01]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-09 21:25:33]

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

[commit-bot: I haz the power, 2017-01-09 21:25:34]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2017-01-09 21:49:11]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 21:49:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 00:47:59]

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

[commit-bot: I haz the power, 2017-01-10 00:48:00]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-10 00:49:20]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 00:50:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 01:04:18]

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

[commit-bot: I haz the power, 2017-01-10 01:04:19]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-01-10 01:09:33]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 01:10:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-10 01:11:52]

used a struct per our chat, ptal

[commit-bot: I haz the power, 2017-01-10 02:00:19]

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

[commit-bot: I haz the power, 2017-01-10 02:00:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jam, 2017-01-10 04:27:28]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 04:28:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 04:50:07]

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

[commit-bot: I haz the power, 2017-01-10 04:50:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-01-10 05:03:56]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-01-10 05:04:03]

Patchset #5 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-01-10 05:04:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-10 05:35:13]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-01-10 05:35:38]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-01-10 05:35:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 06:28:03]

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

[commit-bot: I haz the power, 2017-01-10 06:28:04]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2017-01-10 06:33:22]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 06:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-10 06:33:46]

Patchset #5 (id:200001) has been deleted

[jam, 2017-01-10 06:34:00]

Patchset #4 (id:140001) has been deleted

[jam, 2017-01-10 06:34:48]

Patchset #3 (id:120001) has been deleted

[jam, 2017-01-10 06:35:02]

Patchset #1 (id:80001) has been deleted

[jam, 2017-01-10 06:39:04]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 06:39:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-10 06:40:19]

Patchset #2 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-01-10 08:19:45]

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

[commit-bot: I haz the power, 2017-01-10 08:19:46]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-01-10 18:58:17]

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
File chrome/common/secure_origin_whitelist.h (right):

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist.h:18: void
GetSecureOriginWhitelist(std::vector<std::string>* origins);
Why did the type change to string? The results look like they're just given to
GURL constructor. If we're passing URLs around, I think they should be GURL
objects. (vector change seems good)

Optional: if we are changing this a return value is cleaner. Then in the caller
you can even do:
  for (auto& origin : GetSecureOriginWhitelist()) ...

https://codereview.chromium.org/2622693002/diff/240001/content/common/url_sch...
File content/common/url_schemes.h (right):

https://codereview.chromium.org/2622693002/diff/240001/content/common/url_sch...
content/common/url_schemes.h:29: // getters can be invoked on any thread.
Can you comment here when the return values will be null? If you don't think
that's an interesting case, then they should probably return const refs instead.

[jam, 2017-01-10 19:16:32]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-01-10 19:16:33]

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
File chrome/common/secure_origin_whitelist.h (right):

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist.h:18: void
GetSecureOriginWhitelist(std::vector<std::string>* origins);
On 2017/01/10 18:58:17, brettw (ping after 24h) wrote:
> Why did the type change to string? The results look like they're just given to
> GURL constructor. If we're passing URLs around, I think they should be GURL
> objects. (vector change seems good)

This is for consistency with all the other parameters to the ContentClient
method which are strings. Since the source data for all these parameters are
string constants, it didn't seem to matter whether the caller wraps it with a
GURL or the receiver does it in this one case.

> 
> Optional: if we are changing this a return value is cleaner. Then in the
caller
> you can even do:
>   for (auto& origin : GetSecureOriginWhitelist()) ...

changed to return value and the method below while i was at it

https://codereview.chromium.org/2622693002/diff/240001/content/common/url_sch...
File content/common/url_schemes.h (right):

https://codereview.chromium.org/2622693002/diff/240001/content/common/url_sch...
content/common/url_schemes.h:29: // getters can be invoked on any thread.
On 2017/01/10 18:58:17, brettw (ping after 24h) wrote:
> Can you comment here when the return values will be null? If you don't think
> that's an interesting case, then they should probably return const refs
instead.

changed to const ref

[jam, 2017-01-10 19:29:10]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 19:29:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-01-10 20:19:45]

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
File chrome/common/secure_origin_whitelist.h (right):

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist.h:18: void
GetSecureOriginWhitelist(std::vector<std::string>* origins);
But the other things in the list are schemes which aren't URLs. In fact, when I
saw the Schemes object and that one of them was listed as "origins", I
questioned what exactly that meant. Is it canonical? Does it have a slash after
the host name? What can you compare it to? 

Sending as a GURL resolves all of these questions with no documentation.

[jam, 2017-01-10 21:01:37]

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
File chrome/common/secure_origin_whitelist.h (right):

https://codereview.chromium.org/2622693002/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist.h:18: void
GetSecureOriginWhitelist(std::vector<std::string>* origins);
On 2017/01/10 20:19:45, brettw (ping after 24h) wrote:
> But the other things in the list are schemes which aren't URLs. In fact, when
I
> saw the Schemes object and that one of them was listed as "origins", I
> questioned what exactly that meant. Is it canonical? Does it have a slash
after
> the host name? What can you compare it to? 
> 
> Sending as a GURL resolves all of these questions with no documentation.

ok, done

[jam, 2017-01-10 21:01:47]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 21:02:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-01-10 21:35:23]

lgtm

[jam, 2017-01-10 21:38:01]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-01-10 21:38:05]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-01-10 21:38:09]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-10 21:38:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 23:24:17]

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

[commit-bot: I haz the power, 2017-01-10 23:24:19]

Failed to apply patch for
content/browser/service_worker/service_worker_provider_host_unittest.cc:
While running git apply --index -p1;
  error: patch failed:
content/browser/service_worker/service_worker_provider_host_unittest.cc:172
  error:
content/browser/service_worker/service_worker_provider_host_unittest.cc: patch
does not apply

Patch:      
content/browser/service_worker/service_worker_provider_host_unittest.cc
Index: content/browser/service_worker/service_worker_provider_host_unittest.cc
diff --git
a/content/browser/service_worker/service_worker_provider_host_unittest.cc
b/content/browser/service_worker/service_worker_provider_host_unittest.cc
index
184d661b6554db35c038e4c430e6c6a402483897..99e9e7996960ff013bea6d747e8c49503c95b39d
100644
--- a/content/browser/service_worker/service_worker_provider_host_unittest.cc
+++ b/content/browser/service_worker/service_worker_provider_host_unittest.cc
@@ -14,6 +14,7 @@
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_test_utils.h"
 #include "content/browser/service_worker/service_worker_version.h"
+#include "content/common/url_schemes.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/test/test_content_browser_client.h"
@@ -26,8 +27,8 @@ const char kServiceWorkerScheme[] =
"i-can-use-service-worker";
 
 class ServiceWorkerTestContentClient : public TestContentClient {
  public:
-  void AddServiceWorkerSchemes(std::set<std::string>* schemes) override {
-    schemes->insert(kServiceWorkerScheme);
+  void AddAdditionalSchemes(Schemes* schemes) override {
+    schemes->service_worker_schemes.push_back(kServiceWorkerScheme);
   }
 };
 
@@ -42,6 +43,7 @@ class ServiceWorkerProviderHostTest : public testing::Test {
   void SetUp() override {
     old_content_browser_client_ =
         SetBrowserClientForTesting(&test_content_browser_client_);
+    RefreshSecuritySchemesForTesting();
 
     helper_.reset(new EmbeddedWorkerTestHelper(base::FilePath()));
     context_ = helper_->context();
@@ -79,13 +81,15 @@ class ServiceWorkerProviderHostTest : public testing::Test {
     registration2_ = 0;
     helper_.reset();
     SetBrowserClientForTesting(old_content_browser_client_);
+    // Reset cached security schemes so we don't affect other tests.
+    RefreshSecuritySchemesForTesting();
   }
 
   bool PatternHasProcessToRun(const GURL& pattern) const {
     return context_->process_manager()->PatternHasProcessToRun(pattern);
   }
 
-  content::TestBrowserThreadBundle thread_bundle_;
+  TestBrowserThreadBundle thread_bundle_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
   ServiceWorkerContextCore* context_;
   scoped_refptr<ServiceWorkerRegistration> registration1_;
@@ -172,7 +176,6 @@ TEST_P(ServiceWorkerProviderHostTestP, MatchRegistration) {
 
 TEST_P(ServiceWorkerProviderHostTestP, ContextSecurity) {
   using FrameSecurityLevel = ServiceWorkerProviderHost::FrameSecurityLevel;
-  content::ResetSchemesAndOriginsWhitelistForTesting();
 
   // Insecure document URL.
   provider_host1_->SetDocumentUrl(GURL("http://host"));

[jam, 2017-01-10 23:53:13]

The CQ bit was checked by jam@chromium.org

[jam, 2017-01-10 23:53:14]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2622693002/#ps300001
(title: "merge")

[commit-bot: I haz the power, 2017-01-10 23:53:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 01:36:51]

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

[commit-bot: I haz the power, 2017-01-11 01:36:52]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2017-01-11 01:54:30]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-11 01:55:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 03:13:50]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1484099670050330,
"parent_rev": "cc1cb9fbd275d9b119ffdaf31d7bec56cb5b4d8d", "commit_rev":
"e0dcd98dbb8363f64857345c317da57cde0588b3"}

[commit-bot: I haz the power, 2017-01-11 03:14:20]

Description was changed from

==========
Cleanup of static lists of schemes & origins that are created at startup.

This in preparation for using this method to grab other schemes that we need to
now share across the browser and renderer for PlzNavigate. This change does:
-combines
AddAdditionalSchemes/AddSecureSchemesAndOrigins/AddServiceWorkerSchemes
-standardizes on std::string

BUG=627502
==========

to

==========
Cleanup of static lists of schemes & origins that are created at startup.

This in preparation for using this method to grab other schemes that we need to
now share across the browser and renderer for PlzNavigate. This change does:
-combines
AddAdditionalSchemes/AddSecureSchemesAndOrigins/AddServiceWorkerSchemes
-standardizes on std::string

BUG=627502

Review-Url: https://codereview.chromium.org/2622693002
Cr-Commit-Position: refs/heads/master@{#442776}
Committed:
https://chromium.googlesource.com/chromium/src/+/e0dcd98dbb8363f64857345c317d...
==========

[commit-bot: I haz the power, 2017-01-11 03:14:23]

Committed patchset #5 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/e0dcd98dbb8363f64857345c317d...