Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2098723003:
Add content ID scheme to //url/url_constants.* (Closed)
DescriptionAdd content ID scheme to //url/url_constants.*
This CL adds the content ID scheme to //url/url_constants.*. This scheme is
specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for
this CL is that an upcoming CL needs to reference the content ID scheme in
order to correctly handle navigations within MHTML documents when using
browser-side navigation.
BUG=623553
Committed: https://crrev.com/5ef36cb49aff9e2daeb0f449bfa2ac68c2b52222
Cr-Commit-Position: refs/heads/master@{#402185}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Response to review #
Dependent Patchsets: Messages
Total messages: 23 (9 generated)
blundell@chromium.org changed reviewers: + mkwst@chromium.org
Hi Mike, AFAICT, //url is the right layer at which to add these.
What bits and pieces of the codebase needs to know about these schemes? Should they be considered "standard" (e.g. are they hierarchical)?
On 2016/06/24 08:25:47, Mike West wrote: > What bits and pieces of the codebase needs to know about these schemes? Should > they be considered "standard" (e.g. are they hierarchical)? My understanding is that these schemes are standard for usage within MHTML, so I would consider them as standard as MHTML. //content is what will need to know about them concretely. They could go in //content/public/common/url_constants.*, but they feel like a bad fit to me there, as all of the existing ones there are specific to the content layer. Blink already knows about them: see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mhtml... for example. I don't know what hierarchical means in this context (I'm not a domain expert).
On 2016/06/24 at 08:32:28, blundell wrote: > My understanding is that these schemes are standard for usage within MHTML, so I would consider them as standard as MHTML. Sorry for the confusion, I meant "standard" in the sense of `GURL::IsStandard()`. It looks like these schemes' formats do not match the syntax in https://tools.ietf.org/html/rfc3986#section-3, which is what I meant to ask. Can you add a line to GURLTest::IsStandard to verify/document that these don't show up as standard URLs? > Blink already knows about them: see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mhtml... for example. It doesn't look like Blink or //content use `mid:` anywhere other than WebRTC, and the patch at https://codereview.chromium.org/2098543003/#ps1 doesn't seem to use them either. Could we just add `cid`, or do we need `mid` for something in the future?
Thanks!
Description was changed from ========== Add content ID and message ID schemes to //url/url_constants.* This CL adds the content ID and message ID schemes to //url/url_constants.*. These schemes are specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. ========== to ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. ==========
Description was changed from ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. ========== to ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. ==========
Description was changed from ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. ========== to ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. BUG=623553 ==========
LGTM when the bots are happy. Thanks! On Monday, 27 June 2016, <blundell@chromium.org> wrote: > Thanks! > > https://codereview.chromium.org/2098723003/ > -- -mike -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/27 14:25:56, Mike West wrote: > LGTM when the bots are happy. Thanks! > > On Monday, 27 June 2016, <mailto:blundell@chromium.org> wrote: > > > Thanks! > > > > https://codereview.chromium.org/2098723003/ > > > > > -- > -mike > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. (whoops, I had actually crafted a response specifically addressing your points and somehow ate that response instead of sending it...sorry about that).
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/06/27 14:29:25, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Mike, did you possibly give the LG via email or something? The tooling doesn't seem to be picking it up (including the Rietveld UI, which isn't green on your message).
Yeah, I keep forgetting that email doesn't work. LGTM. Really, I mean it.
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. BUG=623553 ========== to ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. BUG=623553 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. BUG=623553 ========== to ========== Add content ID scheme to //url/url_constants.* This CL adds the content ID scheme to //url/url_constants.*. This scheme is specified in https://tools.ietf.org/html/rfc2392. The concrete motivation for this CL is that an upcoming CL needs to reference the content ID scheme in order to correctly handle navigations within MHTML documents when using browser-side navigation. BUG=623553 Committed: https://crrev.com/5ef36cb49aff9e2daeb0f449bfa2ac68c2b52222 Cr-Commit-Position: refs/heads/master@{#402185} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5ef36cb49aff9e2daeb0f449bfa2ac68c2b52222 Cr-Commit-Position: refs/heads/master@{#402185} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
