Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953943003:
base::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster (Closed)
Descriptionbase::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster
Previously, almost all usages of OffsetAdjuster applied a post-processing
step to clamp the resulting offsets to a "sane" range.
This refactor moves the clamping into the base function, but as an
optional parameter.
This also reduces the API surface by removing the LimitOffset class.
BUG=732582
Review-Url: https://codereview.chromium.org/2953943003
Cr-Commit-Position: refs/heads/master@{#486205}
Committed: https://chromium.googlesource.com/chromium/src/+/7a4241c698a71a5cc10ec6135060f4d99af2b96c
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #Patch Set 4 : fix an issue #Patch Set 5 : fix #
Total comments: 6
Patch Set 6 : merge #Patch Set 7 : fix #Patch Set 8 : fix #
Total comments: 2
Patch Set 9 : fix test #
Messages
Total messages: 46 (30 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 ========== DO NOT SUBMIT: refactor BUG= ========== to ========== Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. This also enables cleaning up some duplicate logic between url_formatter and omnibox components. BUG=732582 ==========
tommycli@chromium.org changed reviewers: + jdonnelly@chromium.org, pkasting@chromium.org
pkasting: PTAL, this is a potential refactor we could do based on our previous discussion about the offset adjustments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
This looks like a nice improvement to me. https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (left): https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:507: base::string16 AutocompleteMatch::FormatUrlForSuggestionDisplayWithOffsets( I think we should keep this function. If it makes sense to adjust the offsets on behalf of the caller in FormatUrlForSuggestionDisplay, it makes as much sense to do so here. https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils.cc (right): https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils.cc:108: // FormatURLWithOffsets call above. Fix this comment to refer to FormatUrlForSuggestionDisplayWithOffsets instead. https://codereview.chromium.org/2953943003/diff/80001/components/url_formatte... File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2953943003/diff/80001/components/url_formatte... components/url_formatter/url_formatter.cc:380: base::string16 FormatUrlWithOffsets( I don't think this function is used anywhere now. Can we get rid of it?
On 2017/06/27 15:28:46, Justin Donnelly wrote: > This looks like a nice improvement to me. > > https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... > File components/omnibox/browser/autocomplete_match.cc (left): > > https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... > components/omnibox/browser/autocomplete_match.cc:507: base::string16 > AutocompleteMatch::FormatUrlForSuggestionDisplayWithOffsets( > I think we should keep this function. If it makes sense to adjust the offsets on > behalf of the caller in FormatUrlForSuggestionDisplay, it makes as much sense to > do so here. > > https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... > File components/omnibox/browser/titled_url_match_utils.cc (right): > > https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... > components/omnibox/browser/titled_url_match_utils.cc:108: // > FormatURLWithOffsets call above. > Fix this comment to refer to FormatUrlForSuggestionDisplayWithOffsets instead. > > https://codereview.chromium.org/2953943003/diff/80001/components/url_formatte... > File components/url_formatter/url_formatter.cc (right): > > https://codereview.chromium.org/2953943003/diff/80001/components/url_formatte... > components/url_formatter/url_formatter.cc:380: base::string16 > FormatUrlWithOffsets( > I don't think this function is used anywhere now. Can we get rid of it? Hey Justin -- thanks for the review. I spoke with Peter yesterday and it's pretty much inevitable that will have to add flags directly into FormatURL (which I think you suggested) for property path ellipsizing. Therefore we are going to: 1) First consolidate all these manipulations into the base FormatURL method under experimental flags. 2) Rejigger this above refactor a bit to account for the above. Thanks, Tommy
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 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 ========== Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. This also enables cleaning up some duplicate logic between url_formatter and omnibox components. BUG=732582 ========== to ========== base::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. BUG=732582 ==========
Description was changed from ========== base::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. BUG=732582 ========== to ========== base::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. This also reduces the API surface by removing the LimitOffset class. BUG=732582 ==========
pkasting: PTAL. This is ready for review now.
jdonnelly: I didn't forget your comments. I think they have been overcome by other events now though, so I'm marking them acknowledged by obsoleted. https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_match.cc (left): https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_match.cc:507: base::string16 AutocompleteMatch::FormatUrlForSuggestionDisplayWithOffsets( On 2017/06/27 15:28:45, Justin Donnelly wrote: > I think we should keep this function. If it makes sense to adjust the offsets on > behalf of the caller in FormatUrlForSuggestionDisplay, it makes as much sense to > do so here. Acknowledged. https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils.cc (right): https://codereview.chromium.org/2953943003/diff/80001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils.cc:108: // FormatURLWithOffsets call above. On 2017/06/27 15:28:46, Justin Donnelly wrote: > Fix this comment to refer to FormatUrlForSuggestionDisplayWithOffsets instead. Acknowledged. https://codereview.chromium.org/2953943003/diff/80001/components/url_formatte... File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2953943003/diff/80001/components/url_formatte... components/url_formatter/url_formatter.cc:380: base::string16 FormatUrlWithOffsets( On 2017/06/27 15:28:46, Justin Donnelly wrote: > I don't think this function is used anywhere now. Can we get rid of it? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2953943003/diff/140001/base/strings/utf_offse... File base/strings/utf_offset_string_conversions_unittest.cc (right): https://codereview.chromium.org/2953943003/diff/140001/base/strings/utf_offse... base/strings/utf_offset_string_conversions_unittest.cc:80: OffsetAdjuster::Adjustments no_adjustments; Nit: const? constexpr? Name kNoAdjustments? Maybe just inline below?
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...
thanks! https://codereview.chromium.org/2953943003/diff/140001/base/strings/utf_offse... File base/strings/utf_offset_string_conversions_unittest.cc (right): https://codereview.chromium.org/2953943003/diff/140001/base/strings/utf_offse... base/strings/utf_offset_string_conversions_unittest.cc:80: OffsetAdjuster::Adjustments no_adjustments; On 2017/07/08 02:23:20, Peter Kasting wrote: > Nit: const? constexpr? Name kNoAdjustments? Maybe just inline below? Done. I did const kNoAdjustments.
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2953943003/#ps160001 (title: "fix test")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tommycli@chromium.org changed reviewers: + brettw@chromium.org
brettw: PTAL owners stamp needed. last days of rietveld
lgtm
On 2017/07/12 19:50:13, brettw wrote: > lgtm thank you, submitting
The CQ bit was checked by tommycli@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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tommycli@chromium.org
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": 160001, "attempt_start_ts": 1499894473276050,
"parent_rev": "44240fb7a36a682f6ef289d8af3e2cf103c1aaba", "commit_rev":
"7a4241c698a71a5cc10ec6135060f4d99af2b96c"}
Message was sent while issue was closed.
Description was changed from ========== base::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. This also reduces the API surface by removing the LimitOffset class. BUG=732582 ========== to ========== base::OffsetAdjuster: Refactor offset limiting logic into the base::OffsetAdjuster Previously, almost all usages of OffsetAdjuster applied a post-processing step to clamp the resulting offsets to a "sane" range. This refactor moves the clamping into the base function, but as an optional parameter. This also reduces the API surface by removing the LimitOffset class. BUG=732582 Review-Url: https://codereview.chromium.org/2953943003 Cr-Commit-Position: refs/heads/master@{#486205} Committed: https://chromium.googlesource.com/chromium/src/+/7a4241c698a71a5cc10ec6135060... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7a4241c698a71a5cc10ec6135060... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
