Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1133943002:
Create net::FormatOriginForDisplay helper function. (Closed)
DescriptionCreate net::FormatOriginForDisplay helper function.
net::FormatOriginForDisplay provides a canonical, concise, human-friendly
display for origins. It supports callers in any higher-layer module.
Based heavily on earlier work by dewittj and felt. Thanks!
BUG=402698
TBR=thakis,brettw
Committed: https://crrev.com/f39e5aff5aae8451a7ef190a2fc822bf80c4e30d
Cr-Commit-Position: refs/heads/master@{#330254}
Patch Set 1 #Patch Set 2 : Add tests, make them pass, note TODOs for the future. #Patch Set 3 : Soothe cranky Windows compiler. #
Total comments: 16
Patch Set 4 : Fixes, tests, and put the big TODO in the right place. #
Total comments: 9
Patch Set 5 : Simplify the implementation for FormatOriginForDisplay, disentangling it form FormatUrl, thus fixin… #Patch Set 6 : Call examples for FormatOriginForDisplay in the comments. #Patch Set 7 : Remove the now-unused kFormatUrlOmit{Scheme,Port}. #Patch Set 8 : Crucial punctuation in comment. #
Total comments: 8
Messages
Total messages: 33 (4 generated)
palmer@chromium.org changed reviewers: + asanka@chromium.org, dewittj@chromium.org, felt@chromium.org
This picks up previous work by dewittj: https://codereview.chromium.org/493253003/ and felt: https://codereview.chromium.org/532523002/ Note that it is not yet ready to land: I need to write some filesystem: tests, and the file:///... test with omit_scheme fails, because for some reason file: does not get stripped off. Weird? Anyway, PTAL and I'll iron those 2 things out.
> Note that it is not yet ready to land: I need to write some filesystem: tests, > and the file:///... test with omit_scheme fails, because for some reason file: > does not get stripped off. Weird? > > Anyway, PTAL and I'll iron those 2 things out. These things are handled. Please take a look. Thanks!
https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:755: new_parsed->port.reset(); The else clause now includes the 'parsed.port.is_nonempty() && (format_types & kFormatUrlOmitPort)' condition where we remove the port. We need to push an adjustment reflect the omission. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:844: base::UTF8ToUTF16(url.path()); It is (or should be) possible for url.host() to be non-empty for a file:// URL. E.g. for file://example.com/path . The special case file:/// is referring to the empty origin, which is considered to be an alias for the local system. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:850: // TODO(palmer): Determine whether GURL::IsStandard should return false for This is a good point. You should file a bug for it and add a reference here, though technically this comment should go in gurl.h. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:856: FormatOriginForDisplay(*inner_url, languages, omit_scheme); Perhaps add a comment that this method deals correctly with filesystem URLs whose authority is a file: URL. "filesystem:" + FormatOriginForDisplay(url.GetOrigin(), ...) would drop the path, which is significant for a file: URL. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:868: if (display_origin.EffectiveIntPort() == default_port) Nit: use IntPort(). https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:872: UnescapeRule::SPACES, NULL, NULL, NULL); NULL -> nullptr https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu_u... File net/base/net_util_icu_unittest.cc (right): https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu_u... net/base/net_util_icu_unittest.cc:1194: L"filesystem:http://www.google.com"}, Nit: filesystem URIs are of the form filesystem:<origin>/<filessytem type>/<path> https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu_u... net/base/net_util_icu_unittest.cc:1200: }; Do you want to test how the code behaves with invalid URLs?
https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:755: new_parsed->port.reset(); On 2015/05/11 23:39:44, asanka wrote: > The else clause now includes the 'parsed.port.is_nonempty() && (format_types & > kFormatUrlOmitPort)' condition where we remove the port. We need to push an > adjustment reflect the omission. Done. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:844: base::UTF8ToUTF16(url.path()); For file: URLs, the path is the origin, so that's what we display. In file: URLs, the hostname is extraneous for origin determination purposes. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:850: // TODO(palmer): Determine whether GURL::IsStandard should return false for On 2015/05/11 23:39:44, asanka wrote: > This is a good point. You should file a bug for it and add a reference here, > though technically this comment should go in gurl.h. Done. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:856: FormatOriginForDisplay(*inner_url, languages, omit_scheme); > Perhaps add a comment that this method deals correctly with filesystem URLs > whose authority is a file: URL. > > "filesystem:" + FormatOriginForDisplay(url.GetOrigin(), ...) would drop the > path, which is significant for a file: URL. Yeah, wow. Either GURL::inner_url is broken, or my expectations were wrong. I have changed the code to cause it to return what I expect, and added tests. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:868: if (display_origin.EffectiveIntPort() == default_port) On 2015/05/11 23:39:44, asanka wrote: > Nit: use IntPort(). Done. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu.c... net/base/net_util_icu.cc:872: UnescapeRule::SPACES, NULL, NULL, NULL); On 2015/05/11 23:39:44, asanka wrote: > NULL -> nullptr Done. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu_u... File net/base/net_util_icu_unittest.cc (right): https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu_u... net/base/net_util_icu_unittest.cc:1194: L"filesystem:http://www.google.com"}, On 2015/05/11 23:39:44, asanka wrote: > Nit: filesystem URIs are of the form filesystem:<origin>/<filessytem > type>/<path> Done. https://codereview.chromium.org/1133943002/diff/40001/net/base/net_util_icu_u... net/base/net_util_icu_unittest.cc:1200: }; > Do you want to test how the code behaves with invalid URLs? I added some, but I am not sure if it really helps. In most/all of the contexts this function is likely to get called in, a page has loaded (implying the URL was valid).
https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util.h#new... net/base/net_util.h:361: // in contexts where the origin is always secure by necessity). Should the flag actually be omit_scheme_if_secure and then only omit the scheme if the origin is secure? https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.c... net/base/net_util_icu.cc:579: const FormatUrlType kFormatUrlOmitScheme = 1 << 3; nit: spaces -- should this line up with the others? https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.c... net/base/net_util_icu.cc:855: return base::ASCIIToUTF16("filesystem:") + will this sometimes yield filesystem:url instead of filesystem://url? (or is it supposed to be filesystem:url? i've never seen a filesystem scheme)
https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.c... net/base/net_util_icu.cc:756: parsed.port.begin, parsed.port.len, 0)); We are also dropping the colon.
https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.c... net/base/net_util_icu.cc:855: return base::ASCIIToUTF16("filesystem:") + > will this sometimes yield filesystem:url instead of filesystem://url? (or is it > supposed to be filesystem:url? i've never seen a filesystem scheme) It yields filesystem:url, as it should. (See tests.)
https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util.h#new... net/base/net_util.h:361: // in contexts where the origin is always secure by necessity). > Should the flag actually be omit_scheme_if_secure and then only omit the scheme > if the origin is secure? I'd like to leave that to the caller to decide, e.g. foo = FormatOriginForDisplay(url, ..., url.SchemeIsCryptographic()); https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.c... net/base/net_util_icu.cc:579: const FormatUrlType kFormatUrlOmitScheme = 1 << 3; > nit: spaces -- should this line up with the others? This is what "git cl format" gives me. (Sigh.) I'll handle it at the end of the process. https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util_icu.c... net/base/net_util_icu.cc:756: parsed.port.begin, parsed.port.len, 0)); > We are also dropping the colon. I'm going to change this code; it's triggering a DCHECK in this file when a particular Mac-only test is run. I'm going to take a new approach today.
https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/1133943002/diff/60001/net/base/net_util.h#new... net/base/net_util.h:361: // in contexts where the origin is always secure by necessity). On 2015/05/14 18:18:01, palmer wrote: > > Should the flag actually be omit_scheme_if_secure and then only omit the > scheme > > if the origin is secure? > > I'd like to leave that to the caller to decide, e.g. > > foo = FormatOriginForDisplay(url, ..., url.SchemeIsCryptographic()); I'm guessing that callers will be using this because they are not sure how to display URLs correctly. I find that leaving off the scheme all the time is a common mistake. But perhaps your comment is sufficient. Maybe mention IsOriginSecure in the comment, as such: Callers should only set |omit_scheme| to true when it is safe to do so (e.g. FormatOriginForDisplay(origin, langs, IsOriginSecure(origin))
> > I'd like to leave that to the caller to decide, e.g. > > > > foo = FormatOriginForDisplay(url, ..., url.SchemeIsCryptographic()); > > I'm guessing that callers will be using this because they are not sure how to > display URLs correctly. I find that leaving off the scheme all the time is a > common mistake. But perhaps your comment is sufficient. Maybe mention > IsOriginSecure in the comment, as such: > > Callers should only set |omit_scheme| to true when it is safe to do so (e.g. > FormatOriginForDisplay(origin, langs, IsOriginSecure(origin)) I have added 2 example calls. See what you think!
TBRing thakis (base/OWNERS) and brettw (url/OWNERS) because I'm only changing comments in those directories. asanka: net/base LGTY? felt: LGTY overall?
I'm wondering about RTL languages. Do you need to test cases to make sure the right thing happens with RTL langs?
> I'm wondering about RTL languages. Do you need to test cases to make sure the > right thing happens with RTL langs? I'm not sure. I don't *think* this CL makes that situation any worse, and that situation is my next project.
On 2015/05/15 21:01:16, palmer wrote: > > I'm wondering about RTL languages. Do you need to test cases to make sure the > > right thing happens with RTL langs? > > I'm not sure. I don't *think* this CL makes that situation any worse, and that > situation is my next project. k cool -- after tackling the RTL in the omnibox, maybe revisit this to make sure your fix there is also applied here?
On 2015/05/15 21:10:10, felt wrote: > On 2015/05/15 21:01:16, palmer wrote: > > > I'm wondering about RTL languages. Do you need to test cases to make sure > the > > > right thing happens with RTL langs? > > > > I'm not sure. I don't *think* this CL makes that situation any worse, and that > > situation is my next project. > > k cool -- after tackling the RTL in the omnibox, maybe revisit this to make sure > your fix there is also applied here? lgtm other than the RTL TODO
lgtm
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133943002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Message was sent while issue was closed.
I previously objected to putting this in //net, which shouldn't have to worry itself about UI elements. Could you explain why it's better suited here rather than an appropriate user-facing layer in Chrome?
Message was sent while issue was closed.
https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:835: if (!url.IsStandard()) BUG: You should be checking how invalid GURLs provided here work (w/r/t is_valid() https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:840: return (omit_scheme ? base::ASCIIToUTF16("") Performance: You should just be using base::string16() here, rather than forcing such a conversion https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:849: FormatOriginForDisplay(inner_url, languages, omit_scheme); BUG: This is not the right way to create sub-GURLs. You should use the ::Replacements to format the inner_url, getting the appropriate scheme & such.
Message was sent while issue was closed.
rsleevi@chromium.org changed reviewers: + brettw@chromium.org, thakis@chromium.org
Message was sent while issue was closed.
Actually adding Nico and Brett to this review, since you TBR'd them to commit this.
Message was sent while issue was closed.
> I previously objected to putting this in //net, which shouldn't have to worry > itself about UI elements. > > Could you explain why it's better suited here rather than an appropriate > user-facing layer in Chrome? Several reasons: * Other net/ code might want to call it (for logging, perhaps)? * Anyone can call it when it's in net, and many call sites all over the place in many modules might potentially to need to call it. * It's right next to its friends FormatUrl*. It fits in nicely here, without having to cross any layers and without being complicated. It fits naturally in net. * It does not make use of any higher-layer concepts — and should not. The function would not get new access to any relevant information if it were put in a higher layer module. * You seemed to mainly object to the comment stating that many callers are likely to be in higher-level modules; you didn't seem to be objecting to any implementation details arising as a result of layering concerns. But there is no guarantee that the only callers will be in higher-layer modules. * It's not hurting anything by being here.
Message was sent while issue was closed.
On 2015/05/16 01:31:24, palmer wrote: > * Other net/ code might want to call it (for logging, perhaps)? net/ code shouldn't concern itself with display. If it is, it's a bug in net/ > * Anyone can call it when it's in net, and many call sites all over the place in > many modules might potentially to need to call it. Similar to base/, this isn't an acceptable criteria. Putting something in a higher layer simply because it "might" be used by more classes is not accepted, and even when there *are* multiple users, it's a high bar. > * It's right next to its friends FormatUrl*. It fits in nicely here, without > having to cross any layers and without being complicated. It fits naturally in > net. I disagree here. It's a presentational concern, and //net has hithertofor tried to avoid user interface considerations. > * It does not make use of any higher-layer concepts — and should not. The > function would not get new access to any relevant information if it were put in > a higher layer module. It's a usability consideration. If we decide it's not as usable, we shouldn't have to change this. Note that quite a bit of our UI for URLs (such as the Omnibox) lives in higher layers. > * You seemed to mainly object to the comment stating that many callers are > likely to be in higher-level modules; you didn't seem to be objecting to any > implementation details arising as a result of layering concerns. But there is no > guarantee that the only callers will be in higher-layer modules. I'm objecting on the fundamental grounds that its only purpose is to serve UI needs, and that's not in and of itself a justificaiton. > * It's not hurting anything by being here. Sorry, that's the worst argument for code health and makes me quite unhappy :( The question about what constitutes a URL for display is absolutely a UX consideration. The fact that you omit the path, for example, is indica enough that it's not acceptable for all display purposes, but only some. I know the motivation is "For things in which we have to present a security origin to the user", but that's also not net's role. We had all these discussions on the previous CL, so it's unclear whether it was intentional that you avoided adding me as a reviewer to this new CL (especially with my request on the bug to be added, precisely so we could have these discussions)
Message was sent while issue was closed.
https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:835: if (!url.IsStandard()) > BUG: You should be checking how invalid GURLs provided here work (w/r/t > is_valid() Can you formulate a unit test that triggers a failure in this code? Note that the unit tests include several wacky URLs. https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:840: return (omit_scheme ? base::ASCIIToUTF16("") > Performance: You should just be using base::string16() here, rather than forcing > such a conversion OK. https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:849: FormatOriginForDisplay(inner_url, languages, omit_scheme); > BUG: This is not the right way to create sub-GURLs. You should use the > ::Replacements to format the inner_url, getting the appropriate scheme & such. Look at previous patchsets, in which we tried that approach. It created new intertwinglings between this function and FormatUrl, which caused code complications and bugs. Can you formulate a unit test that triggers a failure in this code?
Message was sent while issue was closed.
> Actually adding Nico and Brett to this review, since you TBR'd them to commit > this. I figured this was reasonable since they are the OWNERS of files I only added comments to (as stated in a previous message).
Message was sent while issue was closed.
https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.cc File net/base/net_util_icu.cc (right): https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:835: if (!url.IsStandard()) On 2015/05/16 01:37:18, palmer wrote: > > BUG: You should be checking how invalid GURLs provided here work (w/r/t > > is_valid() > > Can you formulate a unit test that triggers a failure in this code? Note that > the unit tests include several wacky URLs. Sure, easy - FormatOriginForDisplay(GURL(), ...) https://codereview.chromium.org/1133943002/diff/140001/net/base/net_util_icu.... net/base/net_util_icu.cc:849: FormatOriginForDisplay(inner_url, languages, omit_scheme); On 2015/05/16 01:37:18, palmer wrote: > > BUG: This is not the right way to create sub-GURLs. You should use the > > ::Replacements to format the inner_url, getting the appropriate scheme & such. > > Look at previous patchsets, in which we tried that approach. It created new > intertwinglings between this function and FormatUrl, which caused code > complications and bugs. I did before making that comment, I didn't see a single patch where you tried to use url::Replacements > Can you formulate a unit test that triggers a failure in this code? I know this was well intentioned push-back, but as the CL submitter, it's your responsibility to adhere to the API contract. It's not my role to provide you counter-tests. It's my role to tell you "Hey, you're not adhering to the supported API, and you're making it our problem to hopefully find and fix before it becomes an issue". That's a hidden technical debt time-bomb.
Message was sent while issue was closed.
On 2015/05/16 01:37:51, palmer wrote: > > Actually adding Nico and Brett to this review, since you TBR'd them to commit > > this. > > I figured this was reasonable since they are the OWNERS of files I only added > comments to (as stated in a previous message). I think the base change is ok to tbr, but tbr'ing means 1.) Adding to review 2.) Sending mail with "tbr thakis,brettw for base/owners" (or equivalent). TBR means "to be reviewed, post-commit, by", not "not reviewed by" – and if I don't get an email about a TBR, I won't know about it and can't review it.
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1131813004/ by palmer@chromium.org. The reason for reverting is: This CL needs more work, and potentially to be moved to a new module..
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f39e5aff5aae8451a7ef190a2fc822bf80c4e30d Cr-Commit-Position: refs/heads/master@{#330254} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
