Share schemes needed for mixed content checking between the browser and renderer.

Share schemes needed for mixed content checking between the browser and renderer.

At process startup, content/chrome/android_webview add their schemes to the default set in url/. Then Blink and content/browser can grab the list from there for their usage. This enforces that the same scheme list is used for browser side navigation mixed content checks as when they're running in Blink.

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

[jam, 2017-01-11 22:59:21]

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

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

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-11 23:12:10]

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

[commit-bot: I haz the power, 2017-01-11 23:12:34]

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-11 23:13:28]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-11 23:42:04]

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

[commit-bot: I haz the power, 2017-01-11 23:42:05]

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

[jam, 2017-01-11 23:51:49]

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

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

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-12 00:21:35]

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

[commit-bot: I haz the power, 2017-01-12 00:21:35]

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

[jam, 2017-01-12 00:40:12]

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

[commit-bot: I haz the power, 2017-01-12 00:41:32]

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-12 01:16:07]

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

[commit-bot: I haz the power, 2017-01-12 01:16:42]

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-12 01:28:33]

Patchset #3 (id:60001) has been deleted

[jam, 2017-01-12 01:28:44]

Patchset #2 (id:40001) has been deleted

[jam, 2017-01-12 01:28:50]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-12 02:03:17]

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

[commit-bot: I haz the power, 2017-01-12 02:03:17]

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-12 04:31:11]

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

[commit-bot: I haz the power, 2017-01-12 04:31:42]

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-12 05:04:49]

Description was changed from

==========
Share schemes needed for mixed content checking between the browser and
renderer.

At process startup, content/chrome/android_webview add their schemes to the
default set in url/. Then Blink and content/browser can grab the list from there
for their usage. This enforces that the same scheme list is used for browser
side navigation mixed content checks.

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

to

==========
Share schemes needed for mixed content checking between the browser and
renderer.

At process startup, content/chrome/android_webview add their schemes to the
default set in url/. Then Blink and content/browser can grab the list from there
for their usage. This enforces that the same scheme list is used for browser
side navigation mixed content checks as when they're running in Blink.

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

[jam, 2017-01-12 05:04:49]

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

[jam, 2017-01-12 05:18:17]

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

[commit-bot: I haz the power, 2017-01-12 05:18:29]

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-12 06:54:08]

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

[commit-bot: I haz the power, 2017-01-12 06:54:08]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-12 17:15:04]

Patchset #1 (id:80001) has been deleted

[jam, 2017-01-12 17:15:14]

Patchset #1 (id:100001) has been deleted

[Mike West, 2017-01-13 11:58:38]

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

[Mike West, 2017-01-13 11:58:38]

mkwst@chromium.org changed required reviewers:
+ brettw@chromium.org

[Mike West, 2017-01-13 11:58:38]

The //url and //third_party/WebKit changes LGTM from a technical perspective.

I'm adding Brett, as I'm on the fence about whether or not these new registries
belong in //url from a layering perspective. On the one hand, things like
"secure" seem fairly cleanly bound to URLs and scheme. Things like "CORS", on
the other hand, seem less URL-related, and more application-related.

Regardless of where they live, please add tests for the new setters and getters.

https://codereview.chromium.org/2623353002/diff/120001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2623353002/diff/120001/url/url_util.h#newcode80
url/url_util.h:80: URL_EXPORT const std::vector<std::string>&
GetSecureSchemes();
Please add tests for these new methods.

https://codereview.chromium.org/2623353002/diff/120001/url/url_util.h#newcode97
url/url_util.h:97: URL_EXPORT const std::vector<std::string>&
GetCORSEnabledSchemes();
Hrm. We have enough of these scheme registries at this point that it may make
sense to split them out from `url_util.h`. Brett, WDYT?

[jam, 2017-01-13 16:14:30]

Thanks, some questions

On 2017/01/13 11:58:38, Mike West (sloooooow) wrote:
> The //url and //third_party/WebKit changes LGTM from a technical perspective.
> 
> I'm adding Brett, as I'm on the fence about whether or not these new
registries
> belong in //url from a layering perspective. On the one hand, things like
> "secure" seem fairly cleanly bound to URLs and scheme. Things like "CORS", on
> the other hand, seem less URL-related, and more application-related.

Would you have a suggestion for a better place to store this?

> 
> Regardless of where they live, please add tests for the new setters and
getters.

Do we really need to test adding/getting a string from a vector? If there was
some logic there that could be broken, sure, but this seems like trivial code?

> 
> https://codereview.chromium.org/2623353002/diff/120001/url/url_util.h
> File url/url_util.h (right):
> 
>
https://codereview.chromium.org/2623353002/diff/120001/url/url_util.h#newcode80
> url/url_util.h:80: URL_EXPORT const std::vector<std::string>&
> GetSecureSchemes();
> Please add tests for these new methods.
> 
>
https://codereview.chromium.org/2623353002/diff/120001/url/url_util.h#newcode97
> url/url_util.h:97: URL_EXPORT const std::vector<std::string>&
> GetCORSEnabledSchemes();
> Hrm. We have enough of these scheme registries at this point that it may make
> sense to split them out from `url_util.h`. Brett, WDYT?

[jam, 2017-01-16 21:54:52]

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

[commit-bot: I haz the power, 2017-01-16 21:55:05]

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-16 22:20:59]

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

[commit-bot: I haz the power, 2017-01-16 22:21:00]

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

[jam, 2017-01-16 22:33:57]

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

[jam, 2017-01-16 22:34:02]

Patchset #2 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-01-16 22:34:10]

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-17 00:13:59]

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

[commit-bot: I haz the power, 2017-01-17 00:14:00]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-01-18 23:06:48]

I think this is reasonable to live in URL. We already moved some of the same
origin stuff to URL and it makes sense to accumulate such stuff here, even if
it's not strictly required for the existing code in the directory.

[brettw, 2017-01-18 23:06:53]

lgtm

[jam, 2017-01-18 23:09:56]

The CQ bit was checked by jam@chromium.org

[jam, 2017-01-18 23:09:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2623353002/#ps160001
(title: "remove unused public methods")

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

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-19 01:49:06]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484780996139040,
"parent_rev": "712079835ab9fd11466c0471d6232983bd52da38", "commit_rev":
"0901535a93c8bdf5d9a67edc8085beb2786888b3"}

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

Description was changed from

==========
Share schemes needed for mixed content checking between the browser and
renderer.

At process startup, content/chrome/android_webview add their schemes to the
default set in url/. Then Blink and content/browser can grab the list from there
for their usage. This enforces that the same scheme list is used for browser
side navigation mixed content checks as when they're running in Blink.

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

to

==========
Share schemes needed for mixed content checking between the browser and
renderer.

At process startup, content/chrome/android_webview add their schemes to the
default set in url/. Then Blink and content/browser can grab the list from there
for their usage. This enforces that the same scheme list is used for browser
side navigation mixed content checks as when they're running in Blink.

BUG=627502

Review-Url: https://codereview.chromium.org/2623353002
Cr-Commit-Position: refs/heads/master@{#444597}
Committed:
https://chromium.googlesource.com/chromium/src/+/0901535a93c8bdf5d9a67edc8085...
==========

[commit-bot: I haz the power, 2017-01-19 01:49:40]

Committed patchset #2 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0901535a93c8bdf5d9a67edc8085...