Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(926)

Issue 1272113002: Allow url::SchemeHostPort to hold non-file scheme without port (Closed)

Created:
2 years, 9 months ago by tyoshino (SeeGerritForStatus)
Modified:
2 years, 9 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Allow url::SchemeHostPort to hold non-file scheme without port WebSockets use url::Origin to pass origin info between renderer and browser. Currently, it cannot hold an origin with non-file scheme and no port. Chrome extensions have been using such origins, so we need to keep the channel to convey origin info work for such origins. BUG=516971 R=sleevi,brettw Committed: https://crrev.com/1ac9ec7bccd1b5178b18338b10149f36292f5fb6 Cr-Commit-Position: refs/heads/master@{#343895} Committed: https://crrev.com/11a7c9fe93850341043844ab48bf379c485d05b1 Cr-Commit-Position: refs/heads/master@{#344181}

Patch Set 1 #

Total comments: 9

Patch Set 2 : Addressed #4 #

Patch Set 3 : #

Patch Set 4 : Updated based on rsleevi's suggestion #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : Add unittests for the newly added url_util functions #

Patch Set 8 : Rebase #

Patch Set 9 : Update mojo code which uses the AddStandardScheme func. Fix URL_EXPORT usage #

Patch Set 10 : Fixing copy assignment #

Patch Set 11 : Fixing copy assignment 2 #

Total comments: 8

Patch Set 12 : Addressed #19. Fixed build. #

Patch Set 13 : Rebase #

Patch Set 14 : Fix include #

Patch Set 15 : Fix iOS build #

Patch Set 16 : Make SchemeWithType POD type #

Patch Set 17 : Fix build break #

Patch Set 18 : Reland #

Patch Set 19 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+309 lines, -122 lines) Patch
M chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc View 1 2 3 17 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/chrome_content_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 17 2 chunks +3 lines, -1 line 0 comments Download
M chrome/common/chrome_content_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +21 lines, -9 lines 0 comments Download
M chrome/common/chrome_content_client_ios.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -1 line 0 comments Download
M chrome/common/chrome_content_client_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 17 2 chunks +18 lines, -0 lines 0 comments Download
M components/test/run_all_unittests.cc View 1 2 3 17 1 chunk +4 lines, -4 lines 0 comments Download
M content/browser/site_instance_impl_unittest.cc View 1 2 3 17 1 chunk +2 lines, -2 lines 0 comments Download
M content/common/url_schemes.cc View 1 2 3 17 2 chunks +7 lines, -7 lines 0 comments Download
M content/public/common/content_client.h View 1 2 3 17 2 chunks +2 lines, -1 line 0 comments Download
M extensions/shell/common/shell_content_client.h View 1 2 3 17 2 chunks +2 lines, -1 line 0 comments Download
M extensions/shell/common/shell_content_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 17 1 chunk +11 lines, -3 lines 0 comments Download
M extensions/test/extensions_unittests_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 17 2 chunks +12 lines, -3 lines 0 comments Download
M google_apis/drive/drive_api_url_generator_unittest.cc View 1 2 3 17 1 chunk +1 line, -1 line 0 comments Download
M ios/chrome/test/ios_chrome_unit_test_suite.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 17 1 chunk +2 lines, -1 line 0 comments Download
M mojo/runner/url_resolver.cc View 1 2 3 4 5 6 7 8 17 1 chunk +1 line, -1 line 0 comments Download
M url/scheme_host_port.h View 1 2 3 4 5 17 1 chunk +9 lines, -10 lines 0 comments Download
M url/scheme_host_port.cc View 1 2 3 4 5 6 7 8 9 10 11 17 4 chunks +103 lines, -50 lines 0 comments Download
M url/url_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 17 3 chunks +30 lines, -3 lines 0 comments Download
M url/url_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 17 11 chunks +45 lines, -22 lines 0 comments Download
M url/url_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 17 2 chunks +33 lines, -1 line 0 comments Download

Messages

Total messages: 62 (22 generated)
tyoshino (SeeGerritForStatus)
Hi Ryan, It's WIP (needs tests). But first I'd like you to review this briefly ...
2 years, 9 months ago (2015-08-06 03:24:28 UTC) #2
Ryan Sleevi
Adding Mike as well. This Not LGTM. A big part of the goal here was ...
2 years, 9 months ago (2015-08-07 01:35:36 UTC) #4
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#newcode38 url/scheme_host_port.cc:38: } On 2015/08/07 01:35:36, Ryan Sleevi wrote: > We ...
2 years, 9 months ago (2015-08-07 02:47:54 UTC) #5
tyoshino (SeeGerritForStatus)
On 2015/08/07 01:35:36, Ryan Sleevi wrote: > Adding Mike as well. > > This Not ...
2 years, 9 months ago (2015-08-07 02:53:38 UTC) #6
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#newcode38 url/scheme_host_port.cc:38: } On 2015/08/07 02:47:54, tyoshino wrote: > On 2015/08/07 ...
2 years, 9 months ago (2015-08-07 06:10:23 UTC) #7
tyoshino (SeeGerritForStatus)
On 2015/08/07 02:53:38, tyoshino wrote: > On 2015/08/07 01:35:36, Ryan Sleevi wrote: > > Adding ...
2 years, 9 months ago (2015-08-07 07:50:08 UTC) #8
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#newcode38 url/scheme_host_port.cc:38: } On 2015/08/07 06:10:23, tyoshino wrote: > On 2015/08/07 ...
2 years, 9 months ago (2015-08-07 07:57:39 UTC) #9
Ryan Sleevi
On 2015/08/07 07:50:08, tyoshino (off on Aug 10) wrote: > On 2015/08/07 02:53:38, tyoshino wrote: ...
2 years, 9 months ago (2015-08-07 20:28:21 UTC) #10
tyoshino (SeeGerritForStatus)
On 2015/08/07 20:28:21, Ryan Sleevi wrote: > On 2015/08/07 07:50:08, tyoshino (off on Aug 10) ...
2 years, 9 months ago (2015-08-11 11:19:20 UTC) #11
Ryan Sleevi
On 2015/08/11 11:19:20, tyoshino wrote: > Sorry for the unclear text. By "extended", I didn't ...
2 years, 9 months ago (2015-08-11 22:44:52 UTC) #12
tyoshino (SeeGerritForStatus)
On 2015/08/11 22:44:52, Ryan Sleevi wrote: > On 2015/08/11 11:19:20, tyoshino wrote: > > Sorry ...
2 years, 9 months ago (2015-08-12 03:40:22 UTC) #13
tyoshino (SeeGerritForStatus)
Done. PTAL
2 years, 9 months ago (2015-08-12 09:54:58 UTC) #14
tyoshino (SeeGerritForStatus)
Added unittests for the newly introduced url_util functions.
2 years, 9 months ago (2015-08-13 15:00:02 UTC) #15
tyoshino (SeeGerritForStatus)
I'll fix the build failures tomorrow. At least chrome compiles now.
2 years, 9 months ago (2015-08-13 18:09:44 UTC) #17
Ryan Sleevi
LGTM; you'll want Brett as url/ OWNERs to review. https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.cc#newcode25 url/scheme_host_port.cc:25: ...
2 years, 9 months ago (2015-08-13 22:32:13 UTC) #19
tyoshino (SeeGerritForStatus)
Thanks! https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.cc#newcode25 url/scheme_host_port.cc:25: const Component raw_host_component(0, static_cast<int>(host.length())); On 2015/08/13 22:32:12, Ryan ...
2 years, 9 months ago (2015-08-14 06:29:01 UTC) #20
tyoshino (SeeGerritForStatus)
Brett, please take a look. This fix is for a stable blocker bug.
2 years, 9 months ago (2015-08-14 06:30:01 UTC) #21
tyoshino (SeeGerritForStatus)
Now the builds are green.
2 years, 9 months ago (2015-08-14 13:10:35 UTC) #22
brettw
LGTM, I didn't look at all of the details.
2 years, 9 months ago (2015-08-14 18:05:02 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/240001
2 years, 9 months ago (2015-08-14 21:43:32 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/56961) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, ...
2 years, 9 months ago (2015-08-14 21:45:10 UTC) #28
Mike West
On 2015/08/14 at 21:45:10, commit-bot wrote: > Try jobs failed on following builders: > ios_dbg_simulator_ninja ...
2 years, 9 months ago (2015-08-17 22:31:12 UTC) #29
tyoshino (SeeGerritForStatus)
On 2015/08/17 22:31:12, Mike West (traveling) wrote: > On 2015/08/14 at 21:45:10, commit-bot wrote: > ...
2 years, 9 months ago (2015-08-18 04:32:57 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/260001
2 years, 9 months ago (2015-08-18 04:34:02 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_dbg_recipe/builds/108174)
2 years, 9 months ago (2015-08-18 04:54:37 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/280001
2 years, 9 months ago (2015-08-18 05:27:50 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/57741)
2 years, 9 months ago (2015-08-18 05:51:18 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/300001
2 years, 9 months ago (2015-08-18 06:17:05 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/40798)
2 years, 9 months ago (2015-08-18 07:06:40 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/300001
2 years, 9 months ago (2015-08-18 07:28:21 UTC) #47
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/40819)
2 years, 9 months ago (2015-08-18 08:31:33 UTC) #49
tyoshino (SeeGerritForStatus)
I accidentally used a global array of non-POD type objects (SchemeWithType struct with constructor). Fixing ...
2 years, 9 months ago (2015-08-18 11:17:28 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/320001
2 years, 9 months ago (2015-08-18 11:29:37 UTC) #53
commit-bot: I haz the power
Committed patchset #16 (id:320001)
2 years, 9 months ago (2015-08-18 12:50:53 UTC) #54
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/1ac9ec7bccd1b5178b18338b10149f36292f5fb6 Cr-Commit-Position: refs/heads/master@{#343895}
2 years, 9 months ago (2015-08-18 12:51:26 UTC) #55
msramek
A revert of this CL (patchset #16 id:320001) has been created in https://codereview.chromium.org/1301563003/ by msramek@chromium.org. ...
2 years, 9 months ago (2015-08-18 13:03:45 UTC) #56
tyoshino (SeeGerritForStatus)
Prepared a patchset for relanding. Checking iOS Device ninja rel
2 years, 9 months ago (2015-08-19 04:05:33 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272113002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272113002/380001
2 years, 9 months ago (2015-08-19 06:45:19 UTC) #60
commit-bot: I haz the power
Committed patchset #19 (id:380001)
2 years, 9 months ago (2015-08-19 08:51:53 UTC) #61
commit-bot: I haz the power
2 years, 9 months ago (2015-08-19 08:52:29 UTC) #62
Message was sent while issue was closed.
Patchset 19 (id:??) landed as
https://crrev.com/11a7c9fe93850341043844ab48bf379c485d05b1
Cr-Commit-Position: refs/heads/master@{#344181}

Powered by Google App Engine
This is Rietveld 408576698