Vectorize MD otr frame icon

Vectorize MD otr frame icon

BUG=501377
Committed: https://crrev.com/05d72ceba126bdd58e1bd87e464ae4d362fac288
Cr-Commit-Position: refs/heads/master@{#360499}

[Evan Stade, 2015-11-17 02:41:11]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2015-11-17 02:41:32]

estade@chromium.org changed reviewers:
+ oshima@chromium.org, pkasting@chromium.org

[Evan Stade, 2015-11-17 02:41:32]

+oshima for c/b/theme
+pkasting for the rest

[Evan Stade, 2015-11-17 02:41:52]

On 2015/11/17 02:41:32, Evan Stade wrote:
> +oshima for c/b/theme
> +pkasting for the rest

c/a*/theme of course

[commit-bot: I haz the power, 2015-11-17 02:42:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452923002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452923002/1

[oshima, 2015-11-17 02:42:59]

lgtm

[commit-bot: I haz the power, 2015-11-17 03:15:04]

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

[commit-bot: I haz the power, 2015-11-17 03:15:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2015-11-17 21:07:59]

LGTM

https://codereview.chromium.org/1452923002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:689: SK_ColorWHITE);
Does this work on Win 10 where the frame color is always white?  Seems like the
incognito icon will be invisible there if it's also white.

[Evan Stade, 2015-11-17 21:34:23]

https://codereview.chromium.org/1452923002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:689: SK_ColorWHITE);
On 2015/11/17 21:07:58, Peter Kasting wrote:
> Does this work on Win 10 where the frame color is always white?  Seems like
the
> incognito icon will be invisible there if it's also white.

The mocks Sebastien provided show a black frame for win 10 OTR (although yea,
it's not implemented yet). Maybe you know something I don't wrt whether a black
frame for OTR makes sense on win10?

[Peter Kasting, 2015-11-17 21:48:31]

https://codereview.chromium.org/1452923002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:689: SK_ColorWHITE);
On 2015/11/17 21:34:23, Evan Stade wrote:
> On 2015/11/17 21:07:58, Peter Kasting wrote:
> > Does this work on Win 10 where the frame color is always white?  Seems like
> the
> > incognito icon will be invisible there if it's also white.
> 
> The mocks Sebastien provided show a black frame for win 10 OTR (although yea,
> it's not implemented yet). Maybe you know something I don't wrt whether a
black
> frame for OTR makes sense on win10?

It's currently not technically possible to implement a black frame on Win 10.

We're hoping this will change down the road, but for now, assume the window
frame on Win 10 is always white, even in incognito.

[Evan Stade, 2015-11-17 22:18:05]

https://codereview.chromium.org/1452923002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:691: #endif
PTAL^

[Peter Kasting, 2015-11-17 22:25:48]

https://codereview.chromium.org/1452923002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:691: #endif
On 2015/11/17 22:18:05, Evan Stade wrote:
> PTAL^

Seems fine.  I might add a comment like "It's not currently possible to change
the frame color on Windows 10, so assuming the frame is white, we can't use a
white icon."

I wonder if there's an API that gives you the frame color?  In the newest WIn 10
version users can configure their system to use a different accent color for
frames, so ideally we'd detect that.  That's not necessary for this patch in any
case, though.

[Evan Stade, 2015-11-17 22:33:18]

https://codereview.chromium.org/1452923002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:691: #endif
On 2015/11/17 22:25:48, Peter Kasting wrote:
> On 2015/11/17 22:18:05, Evan Stade wrote:
> > PTAL^
> 
> Seems fine.  I might add a comment like "It's not currently possible to change
> the frame color on Windows 10, so assuming the frame is white, we can't use a
> white icon."
> 
> I wonder if there's an API that gives you the frame color?  In the newest WIn
10
> version users can configure their system to use a different accent color for
> frames, so ideally we'd detect that.  That's not necessary for this patch in
any
> case, though.

added comment

[Evan Stade, 2015-11-17 22:33:24]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-17 22:33:24]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1452923002/#ps40001
(title: "add comment")

[commit-bot: I haz the power, 2015-11-17 22:35:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452923002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452923002/40001

[commit-bot: I haz the power, 2015-11-17 22:59:28]

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

[commit-bot: I haz the power, 2015-11-17 22:59:29]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Evan Stade, 2015-11-18 00:50:30]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-18 00:51:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452923002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452923002/80001

[commit-bot: I haz the power, 2015-11-18 01:37:49]

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

[commit-bot: I haz the power, 2015-11-18 01:37:50]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2015-11-18 23:06:56]

https://codereview.chromium.org/1452923002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:699: return
*frame_->GetThemeProvider()->GetImageSkiaNamed(IDR_OTR_ICON);
slight change here from previous patchsets, please take a quick glance

[Peter Kasting, 2015-11-18 23:21:56]

https://codereview.chromium.org/1452923002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:699: return
*frame_->GetThemeProvider()->GetImageSkiaNamed(IDR_OTR_ICON);
On 2015/11/18 23:06:56, Evan Stade wrote:
> slight change here from previous patchsets, please take a quick glance

Wouldn't this have been a bug with the existing code?

[Evan Stade, 2015-11-18 23:49:12]

https://codereview.chromium.org/1452923002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1452923002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:699: return
*frame_->GetThemeProvider()->GetImageSkiaNamed(IDR_OTR_ICON);
On 2015/11/18 23:21:56, Peter Kasting wrote:
> On 2015/11/18 23:06:56, Evan Stade wrote:
> > slight change here from previous patchsets, please take a quick glance
> 
> Wouldn't this have been a bug with the existing code?

BrowserView::GetOTRAvatarIcon is now called in more places (i.e. in
AvatarMenuButton::GetAvatarImages)

[Peter Kasting, 2015-11-19 00:01:49]

LGTM

[Evan Stade, 2015-11-19 00:15:31]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-19 00:15:31]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1452923002/#ps80001
(title: "dont crash in tests")

[commit-bot: I haz the power, 2015-11-19 00:16:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452923002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452923002/80001

[commit-bot: I haz the power, 2015-11-19 02:35:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-19 02:37:08]

Patchset 5 (id:??) landed as
https://crrev.com/05d72ceba126bdd58e1bd87e464ae4d362fac288
Cr-Commit-Position: refs/heads/master@{#360499}