Adjust location of text in omnibox edit control for Metro.

Adjust location of text in omnibox edit control for Metro.

BUG=126132
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137616

[Jói, 2012-04-19 15:25:41]

This just moves the cursor to a more appropriate position.  Next on my list is
to look at making the omnibox dropdown a more suitable size on Metro.

Cheers,
Jói

[sky, 2012-04-19 15:56:18]

I'm not sure if there are other subtleties if you tweak this. Peter knows better
than I. I've added him as a reviewer.

[Peter Kasting, 2012-04-19 17:39:55]

http://codereview.chromium.org/9985012/diff/1/chrome/browser/ui/views/omnibox...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/9985012/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:515: text_baseline = 21;
If I understand correctly, this is because in metro mode you're changing the
height of the whole omnibox to be bigger, right?

If so, I don't think this is the right way to do this change.  We should be able
to ask some other object how tall the omnibox is and then calculate the desired
baseline as either a fraction of that height or an offset from the top or bottom
(probably the former).  Maybe we can do this by checking our own height, or else
we could look at the height of |parent_view|.

This will also help if you decide to make metro-mode popup windows have a
different omnibox than non-popups, just as we currently do for non-metro-mode.

[sail, 2012-04-19 17:49:16]

How come ASH doesn't have this problem? Is there anyway to share code with them?

[Jói, 2012-04-23 14:26:54]

Peter, thanks for the review.

I wanted to point out that the previous code had hard-coded offsets
for popup vs. non-popup.  I agree with you that this is non-ideal and
I think your suggestion is quite reasonable as a better fix.  I'm just
wondering since this was the way it was already done whether a
hard-coded offset for Metro might be OK as a quick fix.  As I
understand it, there is other work going on to make layouts scale more
smoothly on HiDPI screens, and I wonder if a whole-sale re-thinking of
how we do layout might not be better than a few one-off fixes here and
there, such as your very reasonable suggestion.

Re pop-ups on Metro, I actually wonder how they are supposed to behave
at all, given that the Metro paradigm is full-screen rather than
windowing.  Will raise this with the UX folks.

Cheers,
Jói



On Thu, Apr 19, 2012 at 5:49 PM,  <sail@chromium.org> wrote:
> How come ASH doesn't have this problem? Is there anyway to share code with
> them?
>
> http://codereview.chromium.org/9985012/

[Peter Kasting, 2012-04-23 18:06:00]

On 2012/04/23 14:26:54, Jói wrote:
> I'm just
> wondering since this was the way it was already done whether a
> hard-coded offset for Metro might be OK as a quick fix.

My appetite for quick fixes always depends on how urgent they are and how likely
we'll do the right thing later :)

Part of my concern with specifying a hard-coded offset here is concern that we
may need different offsets for different zoom levels, different fonts (? not
sure how Metro looks, really), etc.  So I'm worried about the complexity of this
increasing, and the fix I suggested seemed offhand like it would be pretty easy
to implement.  I could be wrong of course.

> As I
> understand it, there is other work going on to make layouts scale more
> smoothly on HiDPI screens, and I wonder if a whole-sale re-thinking of
> how we do layout might not be better than a few one-off fixes here and
> there, such as your very reasonable suggestion.

sail can perhaps speak to this more than I can.

[sail, 2012-04-23 18:16:10]

> > As I
> > understand it, there is other work going on to make layouts scale more
> > smoothly on HiDPI screens, and I wonder if a whole-sale re-thinking of
> > how we do layout might not be better than a few one-off fixes here and
> > there, such as your very reasonable suggestion.
> 
> sail can perhaps speak to this more than I can.

oshima is working on adding HiDPI support to the views framework. Unfortunately
this won't help you for two reasons:
  - This CL is not about HiDPI. In metro mode you're using a large toolbar size
in 1x scale. This means that none of oshima's work will help.
  - The omnibox text field is using a native control (I think?). The HiDPI views
support won't help in the cases where we call native APIs directly.

I believe that in these cases we'll have to do one offs for each of the layouts
(desktop, ash, metro).

[Jói, 2012-04-23 18:22:05]

Thanks Peter.

Sailesh explained things pretty well.  Just one more thing to add, is
that we'll be dealing with Metro's 140% and 180% scale factors (a.k.a.
HiDPI with fixed scale) soon after we get the 100% case working, so
this would provide an opportunity in the very near future to create a
more correct solution (here and elsewhere).

Cheers,
Jói


On Mon, Apr 23, 2012 at 6:16 PM,  <sail@chromium.org> wrote:
>> > As I
>> > understand it, there is other work going on to make layouts scale more
>> > smoothly on HiDPI screens, and I wonder if a whole-sale re-thinking of
>> > how we do layout might not be better than a few one-off fixes here and
>> > there, such as your very reasonable suggestion.
>
>
>> sail can perhaps speak to this more than I can.
>
>
> oshima is working on adding HiDPI support to the views framework.
> Unfortunately
> this won't help you for two reasons:
>  - This CL is not about HiDPI. In metro mode you're using a large toolbar
> size
> in 1x scale. This means that none of oshima's work will help.
>  - The omnibox text field is using a native control (I think?). The HiDPI
> views
> support won't help in the cases where we call native APIs directly.
>
> I believe that in these cases we'll have to do one offs for each of the
> layouts
> (desktop, ash, metro).
>
> http://codereview.chromium.org/9985012/

[Peter Kasting, 2012-04-23 18:24:25]

On 2012/04/23 18:22:05, Jói wrote:
> Thanks Peter.
> 
> Sailesh explained things pretty well.  Just one more thing to add, is
> that we'll be dealing with Metro's 140% and 180% scale factors (a.k.a.
> HiDPI with fixed scale) soon after we get the 100% case working, so
> this would provide an opportunity in the very near future to create a
> more correct solution (here and elsewhere).

If you're convinced you'll replace this code, and you just need something
working for the moment, LGTM if it helps you progress faster.  I just don't want
this to languish or get Yet More Cases added later when we're trying to get 140%
and 180% to work.

I'd still probably feel better doing the right fix now (or at least
investigating to see if there are any gotchas).

[Jói, 2012-04-23 18:29:18]

I'll take a look to see if the right fix can be done now.  I just
don't think I'll be able to make it 100% right for all cases until I
have the actual bits for the 140% and 180% Metro layouts - but that's
fine.

Cheers,
Jói


On Mon, Apr 23, 2012 at 6:24 PM,  <pkasting@chromium.org> wrote:
> On 2012/04/23 18:22:05, Jói wrote:
>>
>> Thanks Peter.
>
>
>> Sailesh explained things pretty well.  Just one more thing to add, is
>> that we'll be dealing with Metro's 140% and 180% scale factors (a.k.a.
>> HiDPI with fixed scale) soon after we get the 100% case working, so
>> this would provide an opportunity in the very near future to create a
>> more correct solution (here and elsewhere).
>
>
> If you're convinced you'll replace this code, and you just need something
> working for the moment, LGTM if it helps you progress faster.  I just don't
> want
> this to languish or get Yet More Cases added later when we're trying to get
> 140%
> and 180% to work.
>
> I'd still probably feel better doing the right fix now (or at least
> investigating to see if there are any gotchas).
>
> http://codereview.chromium.org/9985012/

[Jói, 2012-05-16 21:25:36]

Peter: Please take another look.

I finally got around to this fix again, and made it work for all cases without
any hardcoded pixel offsets by calculating the appropriate baseline offset based
on the font and view sizes.

Cheers,
Jói

[Peter Kasting, 2012-05-16 21:34:34]

Once you fix the issue below, it will be easy for me to review the (smaller)
diff against the existing code.

http://codereview.chromium.org/9985012/diff/10002/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/9985012/diff/10002/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1894: // font_y_adjustment_.
Instead of doing this, pass in the expected height to the constructor. 
LocationBarView::Init() calculates this value as |height|.  This is simpler and
safer than waiting for OnWindowPosChanging().

[Jói, 2012-05-16 22:49:13]

Please take another look.

On Wed, May 16, 2012 at 2:34 PM,  <pkasting@chromium.org> wrote:
> Once you fix the issue below, it will be easy for me to review the (smaller)
> diff against the existing code.
>
>
>
http://codereview.chromium.org/9985012/diff/10002/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):
>
>
http://codereview.chromium.org/9985012/diff/10002/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1894: //
> font_y_adjustment_.
> Instead of doing this, pass in the expected height to the constructor.
> LocationBarView::Init() calculates this value as |height|.  This is
> simpler and safer than waiting for OnWindowPosChanging().
>
> http://codereview.chromium.org/9985012/

[Peter Kasting, 2012-05-16 23:19:50]

I just realized we can do even better than passing the height in, in the sense
of minimizing the number of constructor args (and number of files touched): we
can get this height off the LocationBarView itself because we have a pointer to
it.

We can either do the calculation ourselves -- since both GetPreferredSize() and
kVerticalEdgeThickness are public -- or we can add an accessor on
LocationBarView to do this for us, and make the two places in LocationBarView
that calculate that call this instead.

LGTM with that change.

http://codereview.chromium.org/9985012/diff/14004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/9985012/diff/14004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:509: // centered on the
available height of the view.
You did check that this produces identical results to the current code on
non-Metro, right?

http://codereview.chromium.org/9985012/diff/14004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:511: (height - cap_height) /
2 + cap_height - font_.GetBaseline();
Nit: Simpler:

  font_y_adjustment_ = ((height - cap_height) / 2) - tm.tmInternalLeading;

[Jói, 2012-05-16 23:58:50]

Thanks Peter.

I don't think it should be a goal to minimize the number of files
touched by a change, but I completely agree that an accessor on
LocationBarView to retrieve the height makes sense.  Will switch to
that and use your suggestion for a simpler calculation before
committing.

> You did check that this produces identical results to the current code
> on non-Metro, right?

Yep, see screenshots I just emailed you for the three cases (desktop,
pop-up, Metro).

Cheers,
Jói


On Wed, May 16, 2012 at 4:19 PM,  <pkasting@chromium.org> wrote:
> I just realized we can do even better than passing the height in, in the
> sense
> of minimizing the number of constructor args (and number of files touched):
> we
> can get this height off the LocationBarView itself because we have a pointer
> to
> it.
>
> We can either do the calculation ourselves -- since both GetPreferredSize()
> and
> kVerticalEdgeThickness are public -- or we can add an accessor on
> LocationBarView to do this for us, and make the two places in
> LocationBarView
> that calculate that call this instead.
>
> LGTM with that change.
>
>
>
http://codereview.chromium.org/9985012/diff/14004/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):
>
>
http://codereview.chromium.org/9985012/diff/14004/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_view_win.cc:509: // centered on
> the available height of the view.
> You did check that this produces identical results to the current code
> on non-Metro, right?
>
>
http://codereview.chromium.org/9985012/diff/14004/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_view_win.cc:511: (height -
> cap_height) / 2 + cap_height - font_.GetBaseline();
> Nit: Simpler:
>
>  font_y_adjustment_ = ((height - cap_height) / 2) -
> tm.tmInternalLeading;
>
> http://codereview.chromium.org/9985012/

[Jói, 2012-05-17 00:29:21]

Thanks again for the review.  I've uploaded a version with the changes you
suggested, I agree it's a lot more beautiful this way.

I'm flipping the CQ bit now, feel free to unset it if you see anything that
needs fixing.

Cheers,
Jói

[commit-bot: I haz the power, 2012-05-17 00:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9985012/18003

[Peter Kasting, 2012-05-17 01:09:06]

https://chromiumcodereview.appspot.com/9985012/diff/18003/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/9985012/diff/18003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:522: int
location_height = std::max(height() - (kVerticalEdgeThickness * 2), 0);
Nit: Should this call GetHeight() too, maybe if you added a bool about whether
to use the preferred or the current height?

[commit-bot: I haz the power, 2012-05-17 02:21:51]

Change committed as 137616

[Jói, 2012-05-17 05:25:23]

Whoops, just saw this. Will address your suggestion in a separate change.
On May 16, 2012 7:21 PM, <commit-bot@chromium.org> wrote:

> Change committed as 137616
>
>
https://chromiumcodereview.**appspot.com/9985012/<https://chromiumcodereview....
>