Issue metadata
Sign in to add a comment
|
CCT trusted publisher URL display escapes spaces |
||||||||||||||||||||||
Issue descriptionCustomTabToolbar.java uses different URL formatting for trusted publisher URLs than a normal URL. In particular, extractPublisherFromPublisherUrl uses formatUrlForDisplayOmitScheme which escapes spaces, whereas other omnibox-like surfaces are careful not to escape spaces; see https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc?type=cs&q=geturlfordisplay&sq=package:chromium&l=60. We should try to reuse that same formatting code (ToolbarModelImpl::GetFormattedURL) for the publisher URL if possible.
,
Apr 27 2018
See https://chromium-review.googlesource.com/c/chromium/src/+/1015229#message-730d1442fe3728ac22f8f5f27f76d2cbaa45eefc for some more discussion on this too. Since formatUrlForDisplay isn't used, it would be nice to be able to get rid of formatUrlForDisplayOmitScheme too (or at least rename it to something more clear that doesn't reference the dead formatUrlForDisplay function).
,
Apr 27 2018
Hm, what do you think about adding a method to ToolbarModel that formats an arbitrary URL? We could then pass in the publisher origin for CCTs so they'll always be formatted the same way.
,
Apr 27 2018
+pkasting for thoughts on c3
,
Apr 27 2018
I don't understand the problem space well enough. It's a bit unusual to want to expose the omnibox' formatting to other consumers because the omnibox has fairly custom needs, and it wouldn't normally be appropriate for URLs displayed in other surfaces to copy it; they should be making their own decisions. But "custom tab toolbar" sounds as if this is maybe a URL displayed where the omnibox would normally be in a custom tab toolbar, and thus kinda makes sense to format the same way? Not sure. Note that we'd really prefer SPACES to NORMAL if we know users are not copying this URL, as it's more readable.
,
Apr 27 2018
FWIW (and to reply to bauerb@), I don't have any strong opinions in this space. The Custom Tab toolbar is meant to display text in a similar way to the omnibox, but it is ready only, so we could argue that didn't need to be the case from the get go. I'm morbidly curious why the omnibox formatting isn't good for other consumers. Is that because it's used for editing? Does it strip parts of the URL out that wouldn't make sense (ports, username/password)? I guess I could read the code, but maybe there is a canonical doc that is better here. I don't know enough of the gotchas for displaying strings to know why something would be safe in one case but not in another. I know that unicode characters can masquerade as other characters (e.g. ı vs i). But, I would assume that would apply across security sensitive surfaces, and why we use punycode. Despite this rambling, I think formatting the publisher URL with the same formatting used for non-publisher URLs in the CCT toolbar makes sense. I don't have a strong opinion whether that should be the existing omnibox formatting or something else though.
,
Apr 27 2018
Part of it is that we are moving towards having "display" vs. "editing" URLs, and they're optimized for different cases, so you'd need to understand which one you want to be more like. And part of the reason we've made certain decisions is that we know if users interact with the omnibox they'll see both -- so e.g. we're comfortable stripping subdomains like "www" and "m" in part because we know if the user really wants to renavigate to that URL they'll end up interacting with the box and seeing the full URL. If a surface doesn't guarantee that, maybe not all those design decisions make sense. ...Or maybe they still do. This isn't a list of "reasons not to do this" so much as "not sure whether tying these together long-term is the right call, versus having an independent function that happens to behave similarly".
,
May 2 2018
Okay, so in this case we are talking about a display URL -- there is no editable version of it at all in Custom Tabs, and the URL that gets copied is a different one. Also, Custom Tabs passes in a plain origin URL (without path or query etc.), which is I think the only place where spaces might be unescaped anyway? So at least in the context where it is currently used, I think that method should be fine, correct?
,
May 2 2018
If you have only an origin, and it's already in a GURL so it's known to be canonical, then the main things you'd need to worry about during fixup are:
* Whether to show the scheme
* Whether to show "trivial subdomains" ("www.", "m.")
* Punycode -> Unicode/anti-spoofing
The first two of these are the interesting ones from my perspective, since the third should happen similarly in all contexts. In the omnibox, we've just moved to a display scheme that always elides both of these until you begin interacting, at which point we usually show them (except for "http://"). For the use case of custom tabs, it seems like we're less interested in providing a navigable hostname than an identifier of "where did this content come from", in which case the distinction between HTTP and HTTPS is unimportant, as are trivial subdomains.
,
May 4 2018
* We indicate the scheme via the security icon, so we always leave it out of the text. * We do trim trivial subdomains (a larger set, matching what the AMP viewer currently does: (www[0-9]*|web|ftp|wap|home|mobile|amp). * We convert Punycode to Unicode iff non-confusable -- see attached examples.
,
Sep 8
Bulk edit: Moving back into the untriaged pool, as I'm leaving the project.
,
Oct 16
,
Nov 12
Assigning to meacer@ for ownership or further triage per instructions at go/estark-on-leave. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by bauerb@chromium.org
, Apr 27 2018