Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2940973002:
Omnibox UI Experiments: Implement scheme-trimming for suggested URLs. (Closed)
DescriptionOmnibox UI Experiments: Implement scheme-trimming for suggested URLs.
Enabled by feature flag only. Only trims out HTTPs, since we already
trim HTTP, and other schemes do not seem appropriate to trim out.
BUG=732582
TEST=unit and manual
Review-Url: https://codereview.chromium.org/2940973002
Cr-Commit-Position: refs/heads/master@{#481687}
Committed: https://chromium.googlesource.com/chromium/src/+/98195f33ec0f9d33f0099846674c2ebc1f5a7898
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 12
Patch Set 4 : fix #Patch Set 5 : fix #
Total comments: 6
Patch Set 6 : fix #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Omnibox UI Experiments: Implement scheme-trimming for suggested URLs. Enabled by feature flag only. BUG=732582 TEST=unit and manual ========== to ========== Omnibox UI Experiments: Implement scheme-trimming for suggested URLs. Enabled by feature flag only. Only trims out HTTPs, since we already trim HTTP, and other schemes do not seem appropriate to trim out. BUG=732582 TEST=unit and manual ==========
tommycli@chromium.org changed reviewers: + jdonnelly@chromium.org
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jdonnelly: PTAL, thanks!
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Did you consider putting a new option (kFormatUrlOmitHTTPS) in url_formatter? It'd introduce a bit of a conceptual complication (you wouldn't include it in kFormatUrlOmitAll) and you'd have to put a big warning about security surfaces. But this logic is so closely related to the http stripping there it feels like it belongs there. Maybe a TODO to do this if the experimental option is made permanent? https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:517: // Also trim HTTPS if this experiment is enabled. Can you add a note that it's intentional that this won't work for view-source: URLs? https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:520: constexpr char kHTTPS[] = "https://"; std::string(url::kHttpsScheme) + url::kStandardSchemeSeparator instead? https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:525: adjustments->insert(adjustments->begin(), Is this right? Inserting at the beginning instead of appending? If so and we put an adjustment at the beginning, does that require adjusting the subsequent adjustments? Ok, looking at FormatUrlWithAdjustments, I guess not: https://cs.chromium.org/chromium/src/components/url_formatter/url_formatter.c... https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:15: #include "base/strings/utf_offset_string_conversions.h" Can you forward declare base::OffsetAdjuster::Adjustments instead of including this? https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match_unittest.cc (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_unittest.cc:153: FormatUrlTestData omit_scheme_cases[] = { We could use one or more cases here that result in adjustments being performed by url_formatter (e.g. username/pw) to make sure that your logic to insert an additional adjustment is working correctly. https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils.cc (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils.cc:90: AutocompleteMatch::FormatUrlForSuggestionDisplayWithAdjustments( If it's worth having a convenience version of this function that accepts a single offset, it's probably worth having one that takes a vector of offsets, too.
https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:517: // Also trim HTTPS if this experiment is enabled. On 2017/06/20 17:31:01, Justin Donnelly wrote: > Can you add a note that it's intentional that this won't work for view-source: > URLs? Done. https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:520: constexpr char kHTTPS[] = "https://"; On 2017/06/20 17:31:01, Justin Donnelly wrote: > std::string(url::kHttpsScheme) + url::kStandardSchemeSeparator instead? Done. https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:525: adjustments->insert(adjustments->begin(), On 2017/06/20 17:31:01, Justin Donnelly wrote: > Is this right? Inserting at the beginning instead of appending? If so and we put > an adjustment at the beginning, does that require adjusting the subsequent > adjustments? > > Ok, looking at FormatUrlWithAdjustments, I guess not: > https://cs.chromium.org/chromium/src/components/url_formatter/url_formatter.c... Acknowledged. https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.h (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.h:15: #include "base/strings/utf_offset_string_conversions.h" On 2017/06/20 17:31:01, Justin Donnelly wrote: > Can you forward declare base::OffsetAdjuster::Adjustments instead of including > this? Unfortunately it's not possible to forward declare a nested type. Also Adjustments is a typedef so it also can't be forward declared. https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match_unittest.cc (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_match_unittest.cc:153: FormatUrlTestData omit_scheme_cases[] = { On 2017/06/20 17:31:01, Justin Donnelly wrote: > We could use one or more cases here that result in adjustments being performed > by url_formatter (e.g. username/pw) to make sure that your logic to insert an > additional adjustment is working correctly. Done. https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils.cc (right): https://codereview.chromium.org/2940973002/diff/40001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils.cc:90: AutocompleteMatch::FormatUrlForSuggestionDisplayWithAdjustments( On 2017/06/20 17:31:02, Justin Donnelly wrote: > If it's worth having a convenience version of this function that accepts a > single offset, it's probably worth having one that takes a vector of offsets, > too. Done. Hmm... well after this there's now some non-trivial code duplication between the the convenience methods for Offsets in autocomplete_match.cc and url_formatter.cc. Take a look. Folding the logic into the URLFormatter method is starting to look more appealing, but still not very appealing given all the downsides we discussed earlier today. Let me know your thoughts. -- if we should send this in for now or rethink things now.
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Code lgtm but yeah, this is really duplicating a substantial amount of code in url_formatter. Can you ask Peter what he thinks about introducing kFormatUrlOmitHTTPS? The good news is that I think whether you add that to url_formatter or stick with this, the call sites have been made clearer. In other words, even if you add kFormatUrlOmitHTTPS and take most of the logic out of autocomplete_match, introducing FormatUrlForSuggestionDisplay as a place for the logic of which elements we want to omit is a win.
tommycli@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: PTAL, We were hoping to get a second opinion here. https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:528: base::OffsetAdjuster::Adjustments* adjustments) { These three methods duplicate a lot of offset adjustment code found in url_formatter. The alternative Justin and I were discussing was to fold this logic into url_formatter::FormatUrlWithAdjustments. The downside of that is 1) We would have to extend the bitmask flags used in FormatUrlWithAdjustments, and that would change the meaning of kFormatUrlOmitAll. 2) These elisions are experimental and pretty application specific. But the upside is that then we wouldn't need these three methods that are kind of duplicated. We wanted to know your thoughts...
https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:528: base::OffsetAdjuster::Adjustments* adjustments) { On 2017/06/21 18:53:44, tommycli wrote: > These three methods duplicate a lot of offset adjustment code found in > url_formatter. > > The alternative Justin and I were discussing was to fold this logic into > url_formatter::FormatUrlWithAdjustments. > > The downside of that is 1) We would have to extend the bitmask flags used in > FormatUrlWithAdjustments, and that would change the meaning of > kFormatUrlOmitAll. 2) These elisions are experimental and pretty application > specific. > > But the upside is that then we wouldn't need these three methods that are kind > of duplicated. I'm a little leery of adding a new flag type and this functionality, which as you mention does seem pretty specific. However, it seems like this code really boils down to: * We want to FormatUrlWithAdjustments(), * And then add one more adjustment * And some callers want to convert to offsets And half of what you're adding here is about reimplementing the third bullet. I wonder if you could refactor the code in url_formatter to expose some of these helpers more. Or possibly some refactoring should happen elsewhere, e.g. AdjustOffsets() should take an optional third arg that's a limit to clamp everything to. (But maybe not, it looks like right now LimitOffsets is only used by two main places, and one clamps before adjusting while the other clamps after. I don't know why someone would need to clamp before, shouldn't the caller have done that? But whatever.) Basically, it seems like there's an opportunity to shrink down to "only add the actual HTTPS-stripping here", at which point it probably makes sense to leave this here until you know for sure this is going in for good, at which point maybe you do pull it into url_formatter. https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:545: base::CompareCase::SENSITIVE)) { Can we just check if url.SchemeIs(url::kHttpsScheme)? I'd think that'd be simpler and maybe cheaper.
pkasting: Thanks! I'm going to CQ dry run it and send it in for commit later this afternoon. So feel free to object if this isn't commit worthy yet. https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:528: base::OffsetAdjuster::Adjustments* adjustments) { On 2017/06/21 23:17:27, Peter Kasting wrote: > On 2017/06/21 18:53:44, tommycli wrote: > > These three methods duplicate a lot of offset adjustment code found in > > url_formatter. > > > > The alternative Justin and I were discussing was to fold this logic into > > url_formatter::FormatUrlWithAdjustments. > > > > The downside of that is 1) We would have to extend the bitmask flags used in > > FormatUrlWithAdjustments, and that would change the meaning of > > kFormatUrlOmitAll. 2) These elisions are experimental and pretty application > > specific. > > > > But the upside is that then we wouldn't need these three methods that are kind > > of duplicated. > > I'm a little leery of adding a new flag type and this functionality, which as > you mention does seem pretty specific. > > However, it seems like this code really boils down to: > * We want to FormatUrlWithAdjustments(), > * And then add one more adjustment > * And some callers want to convert to offsets > > And half of what you're adding here is about reimplementing the third bullet. > > I wonder if you could refactor the code in url_formatter to expose some of these > helpers more. Or possibly some refactoring should happen elsewhere, e.g. > AdjustOffsets() should take an optional third arg that's a limit to clamp > everything to. (But maybe not, it looks like right now LimitOffsets is only > used by two main places, and one clamps before adjusting while the other clamps > after. I don't know why someone would need to clamp before, shouldn't the > caller have done that? But whatever.) > > Basically, it seems like there's an opportunity to shrink down to "only add the > actual HTTPS-stripping here", at which point it probably makes sense to leave > this here until you know for sure this is going in for good, at which point > maybe you do pull it into url_formatter. Okay cool -- In that case, it seems like the best thing to do is to send this in pretty much as is, and look for an opportunity for refactoring in a followup -- or after we find out if this is going to be enabled by default. https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:545: base::CompareCase::SENSITIVE)) { On 2017/06/21 23:17:27, Peter Kasting wrote: > Can we just check if url.SchemeIs(url::kHttpsScheme)? I'd think that'd be > simpler and maybe cheaper. Done.
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (right): https://codereview.chromium.org/2940973002/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:528: base::OffsetAdjuster::Adjustments* adjustments) { On 2017/06/22 19:18:43, tommycli wrote: > On 2017/06/21 23:17:27, Peter Kasting wrote: > > On 2017/06/21 18:53:44, tommycli wrote: > > > These three methods duplicate a lot of offset adjustment code found in > > > url_formatter. > > > > > > The alternative Justin and I were discussing was to fold this logic into > > > url_formatter::FormatUrlWithAdjustments. > > > > > > The downside of that is 1) We would have to extend the bitmask flags used in > > > FormatUrlWithAdjustments, and that would change the meaning of > > > kFormatUrlOmitAll. 2) These elisions are experimental and pretty application > > > specific. > > > > > > But the upside is that then we wouldn't need these three methods that are > kind > > > of duplicated. > > > > I'm a little leery of adding a new flag type and this functionality, which as > > you mention does seem pretty specific. > > > > However, it seems like this code really boils down to: > > * We want to FormatUrlWithAdjustments(), > > * And then add one more adjustment > > * And some callers want to convert to offsets > > > > And half of what you're adding here is about reimplementing the third bullet. > > > > I wonder if you could refactor the code in url_formatter to expose some of > these > > helpers more. Or possibly some refactoring should happen elsewhere, e.g. > > AdjustOffsets() should take an optional third arg that's a limit to clamp > > everything to. (But maybe not, it looks like right now LimitOffsets is only > > used by two main places, and one clamps before adjusting while the other > clamps > > after. I don't know why someone would need to clamp before, shouldn't the > > caller have done that? But whatever.) > > > > Basically, it seems like there's an opportunity to shrink down to "only add > the > > actual HTTPS-stripping here", at which point it probably makes sense to leave > > this here until you know for sure this is going in for good, at which point > > maybe you do pull it into url_formatter. > > Okay cool -- In that case, it seems like the best thing to do is to send this in > pretty much as is, and look for an opportunity for refactoring in a followup -- > or after we find out if this is going to be enabled by default. FWIW, Justin actually liked that these convenience methods encapsulate the generation of the correct FormatUrlTypes into a single boolean (omit_scheme)...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdonnelly@chromium.org Link to the patchset: https://codereview.chromium.org/2940973002/#ps100001 (title: "fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498169294396180,
"parent_rev": "7e9eee3af0638c3f9990b962c41f3e9b7a83bf84", "commit_rev":
"98195f33ec0f9d33f0099846674c2ebc1f5a7898"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox UI Experiments: Implement scheme-trimming for suggested URLs. Enabled by feature flag only. Only trims out HTTPs, since we already trim HTTP, and other schemes do not seem appropriate to trim out. BUG=732582 TEST=unit and manual ========== to ========== Omnibox UI Experiments: Implement scheme-trimming for suggested URLs. Enabled by feature flag only. Only trims out HTTPs, since we already trim HTTP, and other schemes do not seem appropriate to trim out. BUG=732582 TEST=unit and manual Review-Url: https://codereview.chromium.org/2940973002 Cr-Commit-Position: refs/heads/master@{#481687} Committed: https://chromium.googlesource.com/chromium/src/+/98195f33ec0f9d33f0099846674c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/98195f33ec0f9d33f0099846674c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
