Set toolbar padding to something appropriate for Metro icons.

Set toolbar padding to something appropriate for Metro icons.

Prepare to handle different sizes of Metro icons, and move now-common
code to ui/base/scale.h and .cc

BUG=114311
TEST=When launched from Metro, Chrome uses larger icons in the tab strip and they fit in a reasonably balanced way.


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

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

Sail, would you take an initial look and then I'll ask sky@ for the owners
approval?

Cheers,
Jói

[sail, 2012-04-18 16:44:14]

LGTM!

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.cc
File ui/base/scale.cc (right):

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.cc#newcode19
ui/base/scale.cc:19: bool IsInMetroMode() {
I've added a new metro.h file in this CL:
http://codereview.chromium.org/10082020/diff/7004/ui/base/win/metro.h

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.h
File ui/base/scale.h (right):

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.h#newcode25
ui/base/scale.h:25: // Returns the display scale in use for themes.
can you expand this comment a bit more? What's the difference between a theme
scale and a UI scale. For example, when would I use theme and when would I use
UI?

[Jói, 2012-04-18 17:06:31]

Thanks for the quick review!  Responses below, new patch set has been uploaded.

Cheers,
Jói

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.cc
File ui/base/scale.cc (right):

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.cc#newcode19
ui/base/scale.cc:19: bool IsInMetroMode() {
On 2012/04/18 16:44:14, sail wrote:
> I've added a new metro.h file in this CL:
> http://codereview.chromium.org/10082020/diff/7004/ui/base/win/metro.h

Updated my patch of your change and switched to using that file.

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.h
File ui/base/scale.h (right):

http://codereview.chromium.org/10115029/diff/2001/ui/base/scale.h#newcode25
ui/base/scale.h:25: // Returns the display scale in use for themes.
On 2012/04/18 16:44:14, sail wrote:
> can you expand this comment a bit more? What's the difference between a theme
> scale and a UI scale. For example, when would I use theme and when would I use
> UI?

I've added a TODO on the second function that expands on this a bit.  Basically
I was making sure to leave the existing functionality, which was that for the UI
resources, we don't currently care whether Metro is enabled, but we do care
whether high-DPI is enabled.  I was thinking we would probably want to unify all
of this stuff as much as possible.

[Jói, 2012-04-18 20:15:02]

Hi Sailesh,

I simplified the interface in scale.h based on our discussion, and also improved
the positioning of the new tab button in Metro mode.

Given that this depends on your changes landing, I will probably switch to
looking at the omnibox on Metro in the meantime.  However, let me know if you
think this is ready for sky@ to take a look for OWNERS approval.

Cheers,
Jói

[sail, 2012-04-18 20:17:26]

Second LGTM!

http://codereview.chromium.org/10115029/diff/3003/ui/base/scale.h
File ui/base/scale.h (right):

http://codereview.chromium.org/10115029/diff/3003/ui/base/scale.h#newcode17
ui/base/scale.h:17: #if defined(OS_WIN)
Would it be better to leave out this #ifdef

[Jói, 2012-04-18 20:31:14]

sky: sail@ has reviewed, please take a look for OWNERS approval and any other
feedback you might have.

Cheers,
Jói

http://codereview.chromium.org/10115029/diff/3003/ui/base/scale.h
File ui/base/scale.h (right):

http://codereview.chromium.org/10115029/diff/3003/ui/base/scale.h#newcode17
ui/base/scale.h:17: #if defined(OS_WIN)
On 2012/04/18 20:17:26, sail wrote:
> Would it be better to leave out this #ifdef

Done.

[sky, 2012-04-18 21:50:32]

Oshima recently added scale factor to Monitor. What is the long term plan for
unifying this scale the scale in monitor?

http://codereview.chromium.org/10115029/diff/2009/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/10115029/diff/2009/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_strip.cc:60: const int GetNewTabButtonHOffset()
{
The code that makes use of these values is called A LOT. Should we add fields
and cache them since they'll never change? Adding a static int to each function
would work too.

Also, can you move the functions below the other static variables.

http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.cc
File ui/base/scale.cc (right):

http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.cc#newcode20
ui/base/scale.cc:20: void GetScaleImplWin(bool* use_metro, bool* use_hidpi) {
nit: Since it seems that metro and hidpi are mututally exclusive, it seems like
an enum would be better. It could even return the value then.

http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.h
File ui/base/scale.h (right):

http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.h#newcode14
ui/base/scale.h:14: DS_ASH,
These names are completely non-obvious. Please add a description.

Also, DS doesn't mean much, how about SCALE? I'm not opposed to DISPLAY_SCALE
either, but it's a bit long.

[Jói, 2012-04-19 14:12:03]

Thanks for the review.

Monitor seems ASH-specific at the moment, and its concept of scale is
related to the physical DPI of the screen, whereas the DisplayScale
enum is intended as logical display scales, e.g. to choose the set of
resources and pixel offsets to use on a system, based on some
reasoning about its physical display characteristics.  I added a
comment about that in scale.h, and commented the constants as
requested.  In terms of future integration, the Monitor class' notion
of scale would likely be used in scale.h's GetDisplayScale function
if/when it comes time to think about multiple different DisplayScales
for ASH, the same way ui::GetDPIScale() is currently used in the
Windows implementation.

I've addressed your other comments as well in the latest patchset.  I
added a static int to each of the GetXyz functions to cache the
result.  I inlined the GetScaleImplWin function, it was introduced in
an older changeset but no longer needed to be a separate function.  I
went with the SCALE_ naming for the DisplaySale enum.

Cheers,
Jói


On Wed, Apr 18, 2012 at 9:50 PM,  <sky@chromium.org> wrote:
> Oshima recently added scale factor to Monitor. What is the long term plan
> for
> unifying this scale the scale in monitor?
>
>
>
http://codereview.chromium.org/10115029/diff/2009/chrome/browser/ui/views/tab...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>
>
http://codereview.chromium.org/10115029/diff/2009/chrome/browser/ui/views/tab...
> chrome/browser/ui/views/tabs/tab_strip.cc:60: const int
> GetNewTabButtonHOffset() {
> The code that makes use of these values is called A LOT. Should we add
> fields and cache them since they'll never change? Adding a static int to
> each function would work too.
>
> Also, can you move the functions below the other static variables.
>
> http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.cc
> File ui/base/scale.cc (right):
>
> http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.cc#newcode20
> ui/base/scale.cc:20: void GetScaleImplWin(bool* use_metro, bool*
> use_hidpi) {
> nit: Since it seems that metro and hidpi are mututally exclusive, it
> seems like an enum would be better. It could even return the value then.
>
> http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.h
> File ui/base/scale.h (right):
>
> http://codereview.chromium.org/10115029/diff/2009/ui/base/scale.h#newcode14
> ui/base/scale.h:14: DS_ASH,
> These names are completely non-obvious. Please add a description.
>
> Also, DS doesn't mean much, how about SCALE? I'm not opposed to
> DISPLAY_SCALE either, but it's a bit long.
>
> http://codereview.chromium.org/10115029/

[sky, 2012-04-19 15:37:05]

LGTM

[Jói, 2012-04-23 14:20:52]

Given the discussion on a separate thread, I renamed the DisplayScale
enum and GetDisplayScale function to DisplayLayout/GetDisplayLayout,
and simplified the enum so it has just three values, LAYOUT_ASH,
LAYOUT_DESKTOP and LAYOUT_METRO.  Since the last patchset, I've also
added updated icons from Sebastien.

I plan to land as-is once sail@'s predecessor change has landed.

Cheers,
Jói



On Thu, Apr 19, 2012 at 3:37 PM,  <sky@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/10115029/

[sky, 2012-04-23 14:43:13]

LGTM

[sail, 2012-04-23 15:47:25]

LGTM!

http://codereview.chromium.org/10115029/diff/12001/ui/base/layout.h
File ui/base/layout.h (right):

http://codereview.chromium.org/10115029/diff/12001/ui/base/layout.h#newcode24
ui/base/layout.h:24: // Returns the display layout that should be used.
could you also give an example of where this would be used. (for example, to
tweak hardcoded padding that's layout specific)

[Jói, 2012-04-23 15:54:24]

Done, changed comment to:

// Returns the display layout that should be used.  This could be used
// e.g. to tweak hard-coded padding that's layout specific, or choose
// the .pak file of theme resources to load.
UI_EXPORT const DisplayLayout GetDisplayLayout();

Cheers,
Jói


On Mon, Apr 23, 2012 at 3:47 PM,  <sail@chromium.org> wrote:
> LGTM!
>
>
> http://codereview.chromium.org/10115029/diff/12001/ui/base/layout.h
> File ui/base/layout.h (right):
>
> http://codereview.chromium.org/10115029/diff/12001/ui/base/layout.h#newcode24
> ui/base/layout.h:24: // Returns the display layout that should be used.
> could you also give an example of where this would be used. (for
> example, to tweak hardcoded padding that's layout specific)
>
> http://codereview.chromium.org/10115029/

[commit-bot: I haz the power, 2012-04-26 11:21:24]

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

[commit-bot: I haz the power, 2012-04-26 11:46:04]

Try job failure for 10115029-36001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-26 11:59:56]

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

[commit-bot: I haz the power, 2012-04-26 13:38:14]

Change committed as 134098