Use light images in colored app frames, when the frame color is dark.

Use light images in colored app frames, when the frame color is dark.

This makes it so the buttons can be seen against the dark background.

TBR=oshima
BUG=339558
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269577

[benwells, 2014-05-09 06:49:24]

This patch also moves some images from win to common, but I'll land that bit
separately hence it isn't in this description.

[Matt Giuca, 2014-05-09 08:33:11]

lgtm with a few minor issues.

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
File apps/ui/views/app_window_frame_view.cc (right):

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.cc:76: AddChildView(close_button_);
// STATE_NORMAL images are set in SetButtonImagesForFrame, not here.

(or similar)

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.cc:369: ?
color_utils::GetLuminanceForColor(active_frame_color_)
Optional: Can I suggest a small refactor: add a method GetFrameColor that
returns widget_->IsActive() ? active_frame_color_ : innactive_frame_color_, and
use it here?

If not: Can you simplify this a bit:
unsigned char frame_luma = color_utils::GetLuminanceForColor(
    widget_->IsActive() ? active_frame_color_ :
    inactive_frame_color_);
(+ClangFormat)

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.cc:377: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
const

(I looked up GetSharedInstance and I'm shocked that it returns a non-const
reference... but you shouldn't need non-const here.)

[benwells, 2014-05-09 09:06:04]

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
File apps/ui/views/app_window_frame_view.cc (right):

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.cc:76: AddChildView(close_button_);
On 2014/05/09 08:33:11, Matt Giuca wrote:
> // STATE_NORMAL images are set in SetButtonImagesForFrame, not here.
> 
> (or similar)

Done.

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.cc:369: ?
color_utils::GetLuminanceForColor(active_frame_color_)
On 2014/05/09 08:33:11, Matt Giuca wrote:
> Optional: Can I suggest a small refactor: add a method GetFrameColor that
> returns widget_->IsActive() ? active_frame_color_ : innactive_frame_color_,
and
> use it here?
> 
> If not: Can you simplify this a bit:
> unsigned char frame_luma = color_utils::GetLuminanceForColor(
>     widget_->IsActive() ? active_frame_color_ :
>     inactive_frame_color_);
> (+ClangFormat)

Done.

https://codereview.chromium.org/274013002/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.cc:377: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
On 2014/05/09 08:33:11, Matt Giuca wrote:
> const
> 
> (I looked up GetSharedInstance and I'm shocked that it returns a non-const
> reference... but you shouldn't need non-const here.)

Yeah resource bundle isn't very consty. Needs to be non-const.

[benwells, 2014-05-09 11:56:03]

+tbr oshima

This contains the binary files / resource changes from the other patch you
reviewed.

Apparently the commit queue can handle binary files now so I am going to use the
cq for this.

[benwells, 2014-05-09 11:59:00]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-09 12:00:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/274013002/60001

[commit-bot: I haz the power, 2014-05-09 13:40:49]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 13:46:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 13:46:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[tapted, 2014-05-09 14:59:48]

On 2014/05/09 13:46:14, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

CQ balked on the binaries after all :/. I think it doesn't like ones that appear
as 'A+' in Rietveld. 

** Presubmit ERRORS **
Corrupt PNG in file
chrome/app/theme/default_100_percent/common/app_window_close.png. Note that
binaries are not correctly uploaded to the code review tool and must be directly
submitted using the dcommit command.

Corrupt PNG in file
chrome/app/theme/default_100_percent/common/app_window_close_active.png. Note
that binaries are not correctly uploaded to the code review tool and must be
directly submitted using the dcommit command.

Corrupt PNG in file chro

[benwells, 2014-05-10 01:28:43]

On 2014/05/09 14:59:48, tapted wrote:
> On 2014/05/09 13:46:14, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
> 
> CQ balked on the binaries after all :/. I think it doesn't like ones that
appear
> as 'A+' in Rietveld. 
> 
> ** Presubmit ERRORS **
> Corrupt PNG in file
> chrome/app/theme/default_100_percent/common/app_window_close.png. Note that
> binaries are not correctly uploaded to the code review tool and must be
directly
> submitted using the dcommit command.
> 
> Corrupt PNG in file
> chrome/app/theme/default_100_percent/common/app_window_close_active.png. Note
> that binaries are not correctly uploaded to the code review tool and must be
> directly submitted using the dcommit command.
> 
> Corrupt PNG in file chro

Yeah gonna dcommit the other patch then rebase and land this.

[benwells, 2014-05-10 02:08:56]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-10 02:11:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/274013002/80001

[commit-bot: I haz the power, 2014-05-10 05:42:40]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-10 05:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 05:45:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[benwells, 2014-05-10 08:30:28]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-10 08:31:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/274013002/80001

[commit-bot: I haz the power, 2014-05-10 12:16:24]

Change committed as 269577