Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2855793003:
Fix RTL URL rendering in Omnibox (domain off screen on long URL). (Closed)
|
Created:
3 years, 7 months ago by Matt Giuca Modified:
3 years, 7 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, jdonnelly+watch_chromium.org, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix RTL URL rendering in Omnibox (domain off screen on long URL).
BUG=656417
Review-Url: https://codereview.chromium.org/2855793003
Cr-Commit-Position: refs/heads/master@{#473830}
Committed: https://chromium.googlesource.com/chromium/src/+/c603f3c7e6a01188ba81015e5354e4797a017f45
Patch Set 1 #Patch Set 2 : Added TODO: Tests failing. #Patch Set 3 : Massive simplification; I think this is all we actually need. #Patch Set 4 : Clean up and write new test. #Patch Set 5 : Clarify comment. #
Total comments: 1
Patch Set 6 : Update tests to new behaviour. Had to remove BeginningShownAfterBlur entirely. #
Total comments: 9
Patch Set 7 : Respond to review. #
Total comments: 2
Patch Set 8 : Fix comment nit. #
Messages
Total messages: 32 (22 generated)
The CQ bit was checked by mgiuca@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: 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...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by mgiuca@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...
mgiuca@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2855793003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:787: model()->popup_model()->IsOpen() && text() != model()->PermanentText()) { I had to add this, and the below, null check, because the test would crash otherwise.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (left): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1868: IN_PROC_BROWSER_TEST_F(OmniboxViewTest, BeginningShownAfterBlur) { Sad... I had to delete this entire test. I think it's correct to do so. What this test is testing (as far as I can tell, the only thing it really tests) is that when you blur the Omnibox, it "shows the beginning" by setting the selection range to [0, 0]. The bug that I am fixing here is that selecting [0, 0] doesn't necessarily show the beginning, if your URL has RTL characters in it. So this test is meaningless, it just tests that the old code does what it does. The new code clears scrolling, and that's Views-specific. There's no way to test that from here, because it relies on inspecting RenderText. So the new test in omnibox_view_views_unittest.cc (OnBlur) tests that for Views specifically.
The CQ bit was checked by mgiuca@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.
https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:787: model()->popup_model()->IsOpen() && text() != model()->PermanentText()) { (1) How come you added a popup_model() null-check? Is that necessary for the test? If so, I would explicitly say in a comment here that the popup model can be null in tests (since it should never be in production). (2) Omnibox code doesn't use {} in cases like this (one-line conditional bodies, multi-line conditional); was this git cl format? https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:809: if (location_bar_view_) { Again, if this null-check is needed for tests, say that; otherwise don't add it. https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (left): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:278: // releasing. We should focus the omnibox but not select all of its text. It seems like we should still ensure we don't focus-all when there's a drag-after-click.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Re-running the tests; it might fail but if not, ptal. https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:787: model()->popup_model()->IsOpen() && text() != model()->PermanentText()) { On 2017/05/18 18:27:18, Peter Kasting wrote: > (1) How come you added a popup_model() null-check? Is that necessary for the > test? If so, I would explicitly say in a comment here that the popup model can > be null in tests (since it should never be in production). Yes, it null-crashed in the test. Added a comment. > (2) Omnibox code doesn't use {} in cases like this (one-line conditional bodies, > multi-line conditional); was this git cl format? No, it was "matt cl format" :) Somewhere I picked up a rule that you need braces if either the conditional or the body was multi-line. Style guide doesn't say this. I think it's a good rule anyway but reverting. https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:809: if (location_bar_view_) { On 2017/05/18 18:27:18, Peter Kasting wrote: > Again, if this null-check is needed for tests, say that; otherwise don't add it. Done. https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (left): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:278: // releasing. We should focus the omnibox but not select all of its text. On 2017/05/18 18:27:19, Peter Kasting wrote: > It seems like we should still ensure we don't focus-all when there's a > drag-after-click. True, this should still be false. (I kind of guessed since ALL of these checks are failing even on master on my local PC. Let's run it again on the trybots.)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM if passes https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:787: model()->popup_model()->IsOpen() && text() != model()->PermanentText()) { On 2017/05/19 05:55:32, Matt Giuca wrote: > Somewhere I picked up a rule that you need braces if either the conditional or > the body was multi-line. Style guide doesn't say this. I think it's a good rule > anyway but reverting. Scott likes that rule. I used to really hate it, now I'm more ambivalent. In this case I wouldn't have minded it, it's more a "consistency with other omnibox code" thing than the change actually being bad. https://codereview.chromium.org/2855793003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:785: // information saved above. Note: |popup_model()| can be null in tests. Nit: || not needed around function calls (the () already set them off), just variable names
https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:787: model()->popup_model()->IsOpen() && text() != model()->PermanentText()) { On 2017/05/19 06:11:18, Peter Kasting wrote: > On 2017/05/19 05:55:32, Matt Giuca wrote: > > Somewhere I picked up a rule that you need braces if either the conditional or > > the body was multi-line. Style guide doesn't say this. I think it's a good > rule > > anyway but reverting. > > Scott likes that rule. I used to really hate it, now I'm more ambivalent. In > this case I wouldn't have minded it, it's more a "consistency with other omnibox > code" thing than the change actually being bad. Acknowledged. https://codereview.chromium.org/2855793003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2855793003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:785: // information saved above. Note: |popup_model()| can be null in tests. On 2017/05/19 06:11:18, Peter Kasting wrote: > Nit: || not needed around function calls (the () already set them off), just > variable names Done.
The CQ bit was checked by mgiuca@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/2855793003/#ps160001 (title: "Fix comment nit.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mgiuca@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": 1495522438484720,
"parent_rev": "01912990baad76adb75b28ccc11c74691c9b34d3", "commit_rev":
"c603f3c7e6a01188ba81015e5354e4797a017f45"}
Message was sent while issue was closed.
Description was changed from ========== Fix RTL URL rendering in Omnibox (domain off screen on long URL). BUG=656417 ========== to ========== Fix RTL URL rendering in Omnibox (domain off screen on long URL). BUG=656417 Review-Url: https://codereview.chromium.org/2855793003 Cr-Commit-Position: refs/heads/master@{#473830} Committed: https://chromium.googlesource.com/chromium/src/+/c603f3c7e6a01188ba81015e5354... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c603f3c7e6a01188ba81015e5354... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
