Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2914163004:
Omnibox UI Experiments: Make suggestions dropdown match omnibox width. (Closed)
|
Created:
3 years, 6 months ago by tommycli Modified:
3 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, asvitkine+watch_chromium.org, jdonnelly+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox UI Experiments: Make suggestions dropdown match omnibox width.
Adds flag and Views implementation. Cocoa implementation to follow.
BUG=728844
Review-Url: https://codereview.chromium.org/2914163004
Cr-Commit-Position: refs/heads/master@{#478322}
Committed: https://chromium.googlesource.com/chromium/src/+/0695a7245850b35c774a67028549af90941f481a
Patch Set 1 #Patch Set 2 : fix #
Total comments: 6
Patch Set 3 : Update flag #Patch Set 4 : fix #
Total comments: 2
Patch Set 5 : Merge remote-tracking branch 'refs/remotes/origin/master' into 024-omnibox-width-views #
Messages
Total messages: 39 (28 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...
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 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...
tommycli@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: PTAL. Screenshots in bug. There are portions of this implementation (vertical shadows) that will need a followup, but I think since this is behind a flag, this is ready for review. The vertical shadow may need Skia changes anyways, so I'd prefer to keep that separate. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2985: {"omnibox-ui-suggestions-dropdown-width", Nit: The name of the flag/associated variables sound like they'll take a width arg: --omnibox-blah-width=357 You might consider something like "omnibox-ui-narrow-dropdown" instead, which doesn't sound like it requires an argument (to me). https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:3134: "Makes the suggestions dropdown width match the Omnibox width."; Nit: Don't capitalize Omnibox https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:460: bool match_omnibox_width = base::FeatureList::IsEnabled( It seems like the implementation in this file leaves the actual dropdown width wide, but then draws the shadow and children more narrowly. I would expect a simpler and clearer change, in the end, if instead the dropdown itself was sized and positioned matching the omnibox, rather than just trying to adjust where we paint. Otherwise you're going to have other potential issues with damage rects, event handling, etc. (and even if not, the intent just isn't as clear).
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...
pkasting: Thanks! https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2985: {"omnibox-ui-suggestions-dropdown-width", On 2017/06/05 18:25:44, Peter Kasting wrote: > Nit: The name of the flag/associated variables sound like they'll take a width > arg: > > --omnibox-blah-width=357 > > You might consider something like "omnibox-ui-narrow-dropdown" instead, which > doesn't sound like it requires an argument (to me). Done. https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:3134: "Makes the suggestions dropdown width match the Omnibox width."; On 2017/06/05 18:25:45, Peter Kasting wrote: > Nit: Don't capitalize Omnibox Done. https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/2914163004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:460: bool match_omnibox_width = base::FeatureList::IsEnabled( On 2017/06/05 18:25:45, Peter Kasting wrote: > It seems like the implementation in this file leaves the actual dropdown width > wide, but then draws the shadow and children more narrowly. > > I would expect a simpler and clearer change, in the end, if instead the dropdown > itself was sized and positioned matching the omnibox, rather than just trying to > adjust where we paint. Otherwise you're going to have other potential issues > with damage rects, event handling, etc. (and even if not, the intent just isn't > as clear). Done. Thank you for this feedback! This made things a lot simpler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
pkasting: ping! Thanks - also added some more UI details in the bug. We have a pretty well defined path forward in terms of finalizing the narrow-mode appearance in Harmony-style.
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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: CQ cannot get SignCLA result. Please try later.
On 2017/06/07 17:06:11, tommycli wrote: > pkasting: ping! > > Thanks - also added some more UI details in the bug. We have a pretty well > defined path forward in terms of finalizing the narrow-mode appearance in > Harmony-style. So to be clear, this CL and https://codereview.chromium.org/2923073004/ are designed to work in tandem with each other?
On 2017/06/08 05:34:24, Peter Kasting (busy Wed-Thu) wrote: > On 2017/06/07 17:06:11, tommycli wrote: > > pkasting: ping! > > > > Thanks - also added some more UI details in the bug. We have a pretty well > > defined path forward in terms of finalizing the narrow-mode appearance in > > Harmony-style. > > So to be clear, this CL and https://codereview.chromium.org/2923073004/ are > designed to work in tandem with each other? Yes! After this CL goes in, the https://codereview.chromium.org/2923073004/ patch is a follow-up that adds the shadow. This is a minimal patch that adds the flag and shrinks the Omnibox. The followup adds the styling.
LGTM https://codereview.chromium.org/2914163004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2914163004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:383: *right_margin = narrow_popup ? 0 : *popup_width - bounds().right(); Nit: Reading this, it felt a bit cryptic and error-prone. This felt a bit clearer, and with fewer conditionals: // The popup contents are always sized matching the location bar size. const int popup_contents_left = x(); const int popup_contents_right = bounds().right(); // The popup itself may either be the same width as the contents, or as wide // as the toolbar. const bool narrow_popup = ...; const int popup_left = narrow_popup ? popup_contents_left : 0; const int popup_right = narrow_popup ? popup_contents_right : parent()->width(); *top_left_screen_coord = gfx::Point(popup_left, ...); ... *popup_width = popup_right - popup_left; *left_margin = popup_contents_left - popup_left; *right_margin = popup_right - popup_contents_right;
On 2017/06/09 02:17:53, Peter Kasting (busy Wed-Thu) wrote: > LGTM > > https://codereview.chromium.org/2914163004/diff/60001/chrome/browser/ui/views... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/2914163004/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/location_bar/location_bar_view.cc:383: *right_margin = > narrow_popup ? 0 : *popup_width - bounds().right(); > Nit: Reading this, it felt a bit cryptic and error-prone. This felt a bit > clearer, and with fewer conditionals: > > // The popup contents are always sized matching the location bar size. > const int popup_contents_left = x(); > const int popup_contents_right = bounds().right(); > > // The popup itself may either be the same width as the contents, or as wide > // as the toolbar. > const bool narrow_popup = ...; > const int popup_left = narrow_popup ? popup_contents_left : 0; > const int popup_right = > narrow_popup ? popup_contents_right : parent()->width(); > > *top_left_screen_coord = gfx::Point(popup_left, ...); > ... > > *popup_width = popup_right - popup_left; > *left_margin = popup_contents_left - popup_left; > *right_margin = popup_right - popup_contents_right; Thanks. Last patchset contains a merge which may confuse the interpatch-diff, but I basically took your suggestions verbatim, and those are the only changes. Tommy
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/2914163004/#ps80001 (title: "Merge remote-tracking branch 'refs/remotes/origin/master' into 024-omnibox-width-views")
https://codereview.chromium.org/2914163004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2914163004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:383: *right_margin = narrow_popup ? 0 : *popup_width - bounds().right(); On 2017/06/09 02:17:53, Peter Kasting (busy Wed-Thu) wrote: > Nit: Reading this, it felt a bit cryptic and error-prone. This felt a bit > clearer, and with fewer conditionals: > > // The popup contents are always sized matching the location bar size. > const int popup_contents_left = x(); > const int popup_contents_right = bounds().right(); > > // The popup itself may either be the same width as the contents, or as wide > // as the toolbar. > const bool narrow_popup = ...; > const int popup_left = narrow_popup ? popup_contents_left : 0; > const int popup_right = > narrow_popup ? popup_contents_right : parent()->width(); > > *top_left_screen_coord = gfx::Point(popup_left, ...); > ... > > *popup_width = popup_right - popup_left; > *left_margin = popup_contents_left - popup_left; > *right_margin = popup_right - popup_contents_right; Done.
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": 80001, "attempt_start_ts": 1497025606726310,
"parent_rev": "81faa4757dd1858df5de62a3cc52eecc3d41c519", "commit_rev":
"75d76e18a3edf613343571435bd26e418ade4c38"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497025606726310,
"parent_rev": "8058d1d4dfbea1ace4e9403fdff4ddeff49b8d41", "commit_rev":
"0695a7245850b35c774a67028549af90941f481a"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox UI Experiments: Make suggestions dropdown match omnibox width. Adds flag and Views implementation. Cocoa implementation to follow. BUG=728844 ========== to ========== Omnibox UI Experiments: Make suggestions dropdown match omnibox width. Adds flag and Views implementation. Cocoa implementation to follow. BUG=728844 Review-Url: https://codereview.chromium.org/2914163004 Cr-Commit-Position: refs/heads/master@{#478322} Committed: https://chromium.googlesource.com/chromium/src/+/0695a7245850b35c774a67028549... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0695a7245850b35c774a67028549... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
