Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817403002:
[Omnibox] Elide omnibox text (Closed)
Description[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When display text attribute is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests --gtest_filter=RenderText*Test.LinesInvalidationOnElideBehaviorChange
Review-Url: https://codereview.chromium.org/2817403002
Cr-Commit-Position: refs/heads/master@{#465378}
Committed: https://chromium.googlesource.com/chromium/src/+/74e44d1e750a7780d268d7713c0e07a6b349e795
Patch Set 1 : [Omnibox] Elide omnibox text #
Total comments: 10
Patch Set 2 : Address pkasting's review comment #
Total comments: 2
Patch Set 3 : Reposition elide changing code #
Total comments: 2
Patch Set 4 : Move line clearing to OnDisplayTextAttributeChanged #
Total comments: 6
Patch Set 5 : Address @msw comment #Patch Set 6 : Add line invalidation test on OSX #
Total comments: 5
Patch Set 7 : Fix nit #Patch Set 8 : Add comment #
Messages
Total messages: 35 (20 generated)
simonhong@chromium.org changed reviewers: + msw@chromium.org, pkasting@chromium.org
PTAL. omnibox: @pkasting render_text: @msw
The CQ bit was checked by simonhong@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: The author simonhong@chromium.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by simonhong@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/2817403002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:141: Nit: Blank line here unnecessary https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:754: GetRenderText()->SetElideBehavior(gfx::NO_ELIDE); Nit: Both here and in OnBlur() I'd attempt to call this after calling the superclass handler, so the pattern is "run superclass handler, then do all local handling" https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:770: } Do we need to schedule a paint here or is changing the elide behavior guaranteed to do that? https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:398: OmniboxView* view = NULL; NitL If you need to init this at all, use nullptr https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:406: const gfx::Rect omnibox_bounds = BrowserView::GetBrowserViewForBrowser( Nit: Rather than click-focusing you could probably replace these several statements with chrome::FocusLocationBar(browser());.
https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:141: On 2017/04/17 19:03:52, Peter Kasting wrote: > Nit: Blank line here unnecessary Done. https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:754: GetRenderText()->SetElideBehavior(gfx::NO_ELIDE); On 2017/04/17 19:03:52, Peter Kasting wrote: > Nit: Both here and in OnBlur() I'd attempt to call this after calling the > superclass handler, so the pattern is "run superclass handler, then do all local > handling" Done. https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:770: } On 2017/04/17 19:03:53, Peter Kasting wrote: > Do we need to schedule a paint here or is changing the elide behavior guaranteed > to do that? It's guaranteed. When getting text or properties from RenderText, RenderText ensure reflecting new elide behavior to its text. https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:398: OmniboxView* view = NULL; On 2017/04/17 19:03:54, Peter Kasting wrote: > NitL If you need to init this at all, use nullptr Done. https://codereview.chromium.org/2817403002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:406: const gfx::Rect omnibox_bounds = BrowserView::GetBrowserViewForBrowser( On 2017/04/17 19:03:55, Peter Kasting wrote: > Nit: Rather than click-focusing you could probably replace these several > statements with chrome::FocusLocationBar(browser());. Done.
LGTM https://codereview.chromium.org/2817403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2817403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:754: GetRenderText()->SetElideBehavior(gfx::NO_ELIDE); Nit: Now that I think about it I might move these down even further, just to avoid implying that they have to happen before we talk to the model... maybe to just above the conditional Layout() call, in their own blocks? (2 places)
Thanks for review @pkasting. https://codereview.chromium.org/2817403002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2817403002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:754: GetRenderText()->SetElideBehavior(gfx::NO_ELIDE); On 2017/04/17 20:00:03, Peter Kasting wrote: > Nit: Now that I think about it I might move these down even further, just to > avoid implying that they have to happen before we talk to the model... maybe to > just above the conditional Layout() call, in their own blocks? (2 places) Done.
https://codereview.chromium.org/2817403002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2817403002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:566: lines_.clear(); Hmm, I'm a little surprised this isn't already handled [indirectly] via OnDisplayTextAttributeChanged(). (presumably, when display text properties change, we can't trust the result of the previous line breaking pass). Can you check if removing this |lines_.clear();| statement and calling UpdateDisplayText (perhaps indirectly, via EnsureLayout()) is sufficient? I'm guessing that you don't need any RenderTextChanges, and the user is just somehow skipping a layout pass after changing the elide behavior. Otherwise, it might make sense to do |lines_.clear()| in OnDisplayTextAttributeChanged. (Theoretically we can avoid clearing the lines if we are switching between no eliding and fade eliding, but that's not a big deal.)
Description was changed from
==========
[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When elide behavior is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests
--gtest_filter=RenderTextTest.LinesInvalidationOnElideBehaviorChange
==========
to
==========
[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When display text attribute is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests
--gtest_filter=RenderTextTest.LinesInvalidationOnElideBehaviorChange
==========
https://codereview.chromium.org/2817403002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2817403002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:566: lines_.clear(); On 2017/04/17 22:39:10, msw wrote: > Hmm, I'm a little surprised this isn't already handled [indirectly] via > OnDisplayTextAttributeChanged(). (presumably, when display text properties > change, we can't trust the result of the previous line breaking pass). Can you > check if removing this |lines_.clear();| statement and calling UpdateDisplayText > (perhaps indirectly, via EnsureLayout()) is sufficient? I'm guessing that you > don't need any RenderTextChanges, and the user is just somehow skipping a layout > pass after changing the elide behavior. UpdateDisplayText() didn't handled it properly. In RenderTextHarfBuzz::EnsureLayoutRunList(), UpdateDisplayText() is called. Then, update_display_text_ and update_display_run_list_ are set to false. After that, we lost the chance of clearing previous |lines_|. > > Otherwise, it might make sense to do |lines_.clear()| in > OnDisplayTextAttributeChanged. (Theoretically we can avoid clearing the lines if > we are switching between no eliding and fade eliding, but that's not a big > deal.) Moved.
https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1654: set_lines(&empty_lines); Instead of adding a similar operation in RenderTextHarfBuzz::OnDisplayTextAttributeChanged, can you move this set_lines() call to the top of the |if (update_display_text_)| block below? That should clear |lines_| anytime the layout or display text needs an update. https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:4421: test_api()->set_lines(&lines); nit: avoid adding RenderTextTestApi::set_lines and instead call test_api()->EnsureLayout() to ensure we generate a valid Line struct; you might need to call render_text->SetText() with some non-empty string first. https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:4426: // lines are cleared when elide behavior changes. nit: "Lines"
@msw, Line invalidation test on OSX is added, too. Thanks for review! https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1654: set_lines(&empty_lines); On 2017/04/17 23:51:44, msw wrote: > Instead of adding a similar operation in > RenderTextHarfBuzz::OnDisplayTextAttributeChanged, can you move this set_lines() > call to the top of the |if (update_display_text_)| block below? That should > clear |lines_| anytime the layout or display text needs an update. Yes, it's much better place. Done. https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:4421: test_api()->set_lines(&lines); On 2017/04/17 23:51:44, msw wrote: > nit: avoid adding RenderTextTestApi::set_lines and instead call > test_api()->EnsureLayout() to ensure we generate a valid Line struct; you might > need to call render_text->SetText() with some non-empty string first. Done. https://codereview.chromium.org/2817403002/diff/80001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:4426: // lines are cleared when elide behavior changes. On 2017/04/17 23:51:44, msw wrote: > nit: "Lines" Done.
Description was changed from
==========
[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When display text attribute is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests
--gtest_filter=RenderTextTest.LinesInvalidationOnElideBehaviorChange
==========
to
==========
[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When display text attribute is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests
--gtest_filter=RenderText*Test.LinesInvalidationOnElideBehaviorChange
==========
The CQ bit was checked by simonhong@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.
lgtm with nits https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:174: struct Line { nit: revert this spacing change. https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:537: void EnsureLayoutRunList() { GetRenderTextHarfBuzz()->EnsureLayoutRunList(); } nit: remove this, just call |test_api()->EnsureLayout()|, like many other places. (The harfbuzz-specific internal EnsureLayoutRunList is the first thing that RenderTextHarfBuzz does as part of the more public api, EnsureLayout)
https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:174: struct Line { On 2017/04/18 17:23:44, msw wrote: > nit: revert this spacing change. Done. https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:537: void EnsureLayoutRunList() { GetRenderTextHarfBuzz()->EnsureLayoutRunList(); } On 2017/04/18 17:23:44, msw wrote: > nit: remove this, just call |test_api()->EnsureLayout()|, like many other > places. (The harfbuzz-specific internal EnsureLayoutRunList is the first thing > that RenderTextHarfBuzz does as part of the more public api, EnsureLayout) For emptiness test, this is added. When EnsureLayout() used, new |lines_| are constructed. Should I find other test way for removing this?
lgtm with a new test function comment. https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:537: void EnsureLayoutRunList() { GetRenderTextHarfBuzz()->EnsureLayoutRunList(); } On 2017/04/18 19:56:02, simonhong wrote: > On 2017/04/18 17:23:44, msw wrote: > > nit: remove this, just call |test_api()->EnsureLayout()|, like many other > > places. (The harfbuzz-specific internal EnsureLayoutRunList is the first thing > > that RenderTextHarfBuzz does as part of the more public api, EnsureLayout) > > For emptiness test, this is added. When EnsureLayout() used, new |lines_| are > constructed. > Should I find other test way for removing this? Oh, hmm... I suppose this is okay, but it would also probably be okay to just skip adding these tests... Add a comment that says something like "Do not use this function to ensure layout. This is only used to run a subset of the EnsureLayout functionality and check intermediate state."
On 2017/04/18 20:13:03, msw wrote: > lgtm with a new test function comment. > > https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... > File ui/gfx/render_text_unittest.cc (right): > > https://codereview.chromium.org/2817403002/diff/120001/ui/gfx/render_text_uni... > ui/gfx/render_text_unittest.cc:537: void EnsureLayoutRunList() { > GetRenderTextHarfBuzz()->EnsureLayoutRunList(); } > On 2017/04/18 19:56:02, simonhong wrote: > > On 2017/04/18 17:23:44, msw wrote: > > > nit: remove this, just call |test_api()->EnsureLayout()|, like many other > > > places. (The harfbuzz-specific internal EnsureLayoutRunList is the first > thing > > > that RenderTextHarfBuzz does as part of the more public api, EnsureLayout) > > > > For emptiness test, this is added. When EnsureLayout() used, new |lines_| are > > constructed. > > Should I find other test way for removing this? > > Oh, hmm... I suppose this is okay, but it would also probably be okay to just > skip adding these tests... Add a comment that says something like "Do not use > this function to ensure layout. This is only used to run a subset of the > EnsureLayout functionality and check intermediate state." Suggested comment is added. Thanks!
The CQ bit was checked by simonhong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2817403002/#ps160001 (title: "Add comment")
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": 1492546741154330,
"parent_rev": "7f9189cb86fdc65ca77995d28ee9604a3c5a8312", "commit_rev":
"74e44d1e750a7780d268d7713c0e07a6b349e795"}
Message was sent while issue was closed.
Description was changed from
==========
[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When display text attribute is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests
--gtest_filter=RenderText*Test.LinesInvalidationOnElideBehaviorChange
==========
to
==========
[Omnibox] Elide omnibox text
Elide omnibox text when omnibox doesn't get focused.
This cl has two part of changes.
One part is OmniboxViewViews change.
In this, elide behavior is changed based on focus state.
The other is RenderText change.
When display text attribute is changed, RenderText::lines_ should be cleared.
BUG=698598
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.TextElideStatus,
gfx_unittests
--gtest_filter=RenderText*Test.LinesInvalidationOnElideBehaviorChange
Review-Url: https://codereview.chromium.org/2817403002
Cr-Commit-Position: refs/heads/master@{#465378}
Committed:
https://chromium.googlesource.com/chromium/src/+/74e44d1e750a7780d268d7713c0e...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/74e44d1e750a7780d268d7713c0e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
