Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527983003:
Omnibox: Fix unnecessary inside border (Closed)
|
Created:
4 years ago by Marc Treib Modified:
4 years ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox: Fix unnecessary inside border
This was broken in https://codereview.chromium.org/2510373003
BUG=668594
Committed: https://crrev.com/4117edd7e0555700b4856e281a98a4a19be0280a
Cr-Commit-Position: refs/heads/master@{#435086}
Patch Set 1 #
Total comments: 5
Patch Set 2 : nit #Patch Set 3 : move to OmniboxViewViews #
Messages
Total messages: 21 (12 generated)
The CQ bit was checked by treib@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...
treib@chromium.org changed reviewers: + pkasting@chromium.org
PTAL! https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:251: omnibox_view_->SetBorder(views::CreateEmptyBorder(0, 0, 0, 0)); Turns out that SetBorder call did something after all :) (let this be a lesson to myself to always test stuff before landing, even if "trivial"...) The SetBorder call used to be in Layout(), now it's in Init(), which should be fine because the values now never change during the lifetime of the view.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:251: omnibox_view_->SetBorder(views::CreateEmptyBorder(0, 0, 0, 0)); On 2016/11/25 10:25:41, Marc Treib wrote: > Turns out that SetBorder call did something after all :) (let this be a lesson > to myself to always test stuff before landing, even if "trivial"...) So what does this do? It's not at all apparent to me why we need to set this. ("Because it looks wrong if we don't" isn't what I mean; I mean, why are we getting some other kind of border otherwise?) Nit: CreateEmptyBorder(gfx::Insets()) does slightly less work.
https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:251: omnibox_view_->SetBorder(views::CreateEmptyBorder(0, 0, 0, 0)); On 2016/11/26 06:15:35, Peter Kasting wrote: > On 2016/11/25 10:25:41, Marc Treib wrote: > > Turns out that SetBorder call did something after all :) (let this be a > lesson > > to myself to always test stuff before landing, even if "trivial"...) > > So what does this do? It's not at all apparent to me why we need to set this. > ("Because it looks wrong if we don't" isn't what I mean; I mean, why are we > getting some other kind of border otherwise?) views::Textfield has a FocusableBorder by default: https://cs.chromium.org/chromium/src/ui/views/controls/textfield/textfield.cc... > Nit: CreateEmptyBorder(gfx::Insets()) does slightly less work. Done. Arguably, this should be done by OmniboxViewViews itself; I see no reason why its user should have to set it. Same for the SetFocusBehavior call above (though this seems to be the default anyway...) WDYT?
https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:251: omnibox_view_->SetBorder(views::CreateEmptyBorder(0, 0, 0, 0)); On 2016/11/28 10:46:48, Marc Treib wrote: > On 2016/11/26 06:15:35, Peter Kasting wrote: > > On 2016/11/25 10:25:41, Marc Treib wrote: > > > Turns out that SetBorder call did something after all :) (let this be a > > lesson > > > to myself to always test stuff before landing, even if "trivial"...) > > > > So what does this do? It's not at all apparent to me why we need to set this. > > > ("Because it looks wrong if we don't" isn't what I mean; I mean, why are we > > getting some other kind of border otherwise?) > > views::Textfield has a FocusableBorder by default: > https://cs.chromium.org/chromium/src/ui/views/controls/textfield/textfield.cc... Interesting. So I guess the comment on all this should be something like "cancel the default FocusableBorder on Textfield, since the LocationBarView will indicate the focus state." > > Nit: CreateEmptyBorder(gfx::Insets()) does slightly less work. > > Done. I just found this that seems even better?: SetBorder(views::NullBorder()); > Arguably, this should be done by OmniboxViewViews itself; I see no reason why > its user should have to set it. Same for the SetFocusBehavior call above (though > this seems to be the default anyway...) > WDYT? I agree. OmniboxViewViews should take care of both of these as necessary, and the SetFocusBehavior() call isn't even necessary, so I'd just remove that, and simply move the border setting into OmniboxViewViews().
https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2527983003/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:251: omnibox_view_->SetBorder(views::CreateEmptyBorder(0, 0, 0, 0)); On 2016/11/28 20:00:13, Peter Kasting wrote: > On 2016/11/28 10:46:48, Marc Treib wrote: > > On 2016/11/26 06:15:35, Peter Kasting wrote: > > > On 2016/11/25 10:25:41, Marc Treib wrote: > > > > Turns out that SetBorder call did something after all :) (let this be a > > > lesson > > > > to myself to always test stuff before landing, even if "trivial"...) > > > > > > So what does this do? It's not at all apparent to me why we need to set > this. > > > > > ("Because it looks wrong if we don't" isn't what I mean; I mean, why are we > > > getting some other kind of border otherwise?) > > > > views::Textfield has a FocusableBorder by default: > > > https://cs.chromium.org/chromium/src/ui/views/controls/textfield/textfield.cc... > > Interesting. So I guess the comment on all this should be something like > "cancel the default FocusableBorder on Textfield, since the LocationBarView will > indicate the focus state." Done. > > > Nit: CreateEmptyBorder(gfx::Insets()) does slightly less work. > > > > Done. > > I just found this that seems even better?: > > SetBorder(views::NullBorder()); Done. > > Arguably, this should be done by OmniboxViewViews itself; I see no reason why > > its user should have to set it. Same for the SetFocusBehavior call above > (though > > this seems to be the default anyway...) > > WDYT? > > I agree. OmniboxViewViews should take care of both of these as necessary, and > the SetFocusBehavior() call isn't even necessary, so I'd just remove that, and > simply move the border setting into OmniboxViewViews(). Done.
The CQ bit was checked by treib@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
The CQ bit was checked by treib@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": 40001, "attempt_start_ts": 1480452986803500,
"parent_rev": "92234d2b3727b7a2bb63ec28cf8fff707f97cf95", "commit_rev":
"70b35f578f7aee9dfedb7853a00891c98f38673b"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Omnibox: Fix unnecessary inside border This was broken in https://codereview.chromium.org/2510373003 BUG=668594 ========== to ========== Omnibox: Fix unnecessary inside border This was broken in https://codereview.chromium.org/2510373003 BUG=668594 Committed: https://crrev.com/4117edd7e0555700b4856e281a98a4a19be0280a Cr-Commit-Position: refs/heads/master@{#435086} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4117edd7e0555700b4856e281a98a4a19be0280a Cr-Commit-Position: refs/heads/master@{#435086} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
