Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2923073004:
Omnibox UI Experiments: Simple border for narrow mode (Closed)
|
Created:
3 years, 6 months ago by tommycli Modified:
3 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, jdonnelly+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox UI Experiments: Simple border for narrow mode
Makes the narrow Omnibox follow the same paint pattern as other Chrome
bubbles -- until UI provides customizations.
BUG=728844
Review-Url: https://codereview.chromium.org/2923073004
Cr-Commit-Position: refs/heads/master@{#478637}
Committed: https://chromium.googlesource.com/chromium/src/+/a0770bb9b89c76b8923872054c3195b9a0aaa572
Patch Set 1 #Patch Set 2 : Use BubbleBorder #Patch Set 3 : format #Patch Set 4 : fix #Patch Set 5 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into 025-add-omnibox-border #
Total comments: 10
Patch Set 6 : reformat #Messages
Total messages: 23 (15 generated)
Description was changed from ========== DO NOT SUBMIT: Omnibox UI Experiments: Simple border for narrow mode BUG=728844 ========== to ========== Omnibox UI Experiments: Simple border for narrow mode Makes the narrow Omnibox follow the same paint pattern as other Chrome bubbles -- until UI provides customizations. BUG=728844 ==========
tommycli@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: PTAL, i now think this CL is a reasonable first-start for the rounded corners and shadow look for the narrow Omnibox. Screenshots in bug -- we will keep iterating as Max generates final specs.
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...
On 2017/06/07 22:05:17, tommycli wrote: > pkasting: PTAL, i now think this CL is a reasonable first-start for the rounded > corners and shadow look for the narrow Omnibox. > > Screenshots in bug -- we will keep iterating as Max generates final specs. pkasting: PTAL - the before-patch just went in. Screenshots of this one are in c#16
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...
https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:94: !base::FeatureList::IsEnabled(omnibox::kUIExperimentNarrowDropdown)) { Nit: Pull out to a temp https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:236: if (!base::FeatureList::IsEnabled(omnibox::kUIExperimentNarrowDropdown)) { Nit: Pull out to a temp https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:254: GetNativeTheme() When can this fail? https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:262: // Reposition the popup to factor in for the shadow size. Nit: "Reposition" -> "Outdent"; Remove "for" https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:488: OnPaintBorder(canvas); Just call View::OnPaint() instead of manually calling these.
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
pkasting: Thanks! https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:94: !base::FeatureList::IsEnabled(omnibox::kUIExperimentNarrowDropdown)) { On 2017/06/09 19:49:30, Peter Kasting wrote: > Nit: Pull out to a temp Done. https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:236: if (!base::FeatureList::IsEnabled(omnibox::kUIExperimentNarrowDropdown)) { On 2017/06/09 19:49:30, Peter Kasting wrote: > Nit: Pull out to a temp Done. https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:254: GetNativeTheme() On 2017/06/09 19:49:30, Peter Kasting wrote: > When can this fail? Done. Digging into it, I don't see a failure state... and in fact most of the callers use it as if it can never fail. https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:262: // Reposition the popup to factor in for the shadow size. On 2017/06/09 19:49:30, Peter Kasting wrote: > Nit: "Reposition" -> "Outdent"; Remove "for" Done. https://codereview.chromium.org/2923073004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:488: OnPaintBorder(canvas); On 2017/06/09 19:49:30, Peter Kasting wrote: > Just call View::OnPaint() instead of manually calling these. Done.
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
On 2017/06/10 00:51:01, Peter Kasting wrote: > LGTM peter: thanks!
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": 100001, "attempt_start_ts": 1497278979367110,
"parent_rev": "345081ea475b0c90cbac6314af5532bd666f36b7", "commit_rev":
"a0770bb9b89c76b8923872054c3195b9a0aaa572"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox UI Experiments: Simple border for narrow mode Makes the narrow Omnibox follow the same paint pattern as other Chrome bubbles -- until UI provides customizations. BUG=728844 ========== to ========== Omnibox UI Experiments: Simple border for narrow mode Makes the narrow Omnibox follow the same paint pattern as other Chrome bubbles -- until UI provides customizations. BUG=728844 Review-Url: https://codereview.chromium.org/2923073004 Cr-Commit-Position: refs/heads/master@{#478637} Committed: https://chromium.googlesource.com/chromium/src/+/a0770bb9b89c76b8923872054c31... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a0770bb9b89c76b8923872054c31... |
