Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1262413005:
Fix Spacing in OmniboxResultView (Closed)
|
Created:
5 years, 4 months ago by jonross Modified:
5 years, 4 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Spacing in OmniboxResultView
OmniboxResultView's Widget was returning the default ThemeProvider. With the
layout parameters now themeable, this was leading to incorrect values being used
for layout.
OmniboxResultView has been updated to refer to LocationBarView's ThemeProvider
in order to get the appropriate values.
A TODO was added to review the dependency on LocaitonBarView. This will be
addressed when the omnibox suggestion popup is updated for Material Design.
TEST=Manual testing on device, ran interactive_ui_tests
BUG=515378
Committed: https://crrev.com/5764e3e320c872bfbb1b5dbaf212de1458e5d42f
Cr-Commit-Position: refs/heads/master@{#341921}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #Patch Set 3 : #Messages
Total messages: 31 (18 generated)
jonross@chromium.org changed reviewers: + pkasting@chromium.org
Could you provide the owners review to this change? Thanks, Jon
LGTM https://codereview.chromium.org/1262413005/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1262413005/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:359: location_bar_view_->GetThemeProvider()->GetDisplayProperty( Might want some sort of a "See TODO in Layout()" comment here.
The CQ bit was checked by jonross@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/1262413005/#ps20001 (title: " ")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1262413005 Patch 1). Please resolve the dependency and try again.
https://codereview.chromium.org/1262413005/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1262413005/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:359: location_bar_view_->GetThemeProvider()->GetDisplayProperty( On 2015/07/31 20:37:59, Peter Kasting wrote: > Might want some sort of a "See TODO in Layout()" comment here. Done.
The CQ bit was checked by jonross@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1262413005 Patch 1). Please resolve the dependency and try again.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by jonross@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/1262413005/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1262413005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1262413005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1262413005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1262413005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by jonross@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/1262413005/#ps80001 (title: "Rebase")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1262413005 Patch 1). Please resolve the dependency and try again.
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1262413005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1262413005/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5764e3e320c872bfbb1b5dbaf212de1458e5d42f Cr-Commit-Position: refs/heads/master@{#341921} |
