[Material][Mac] Fix Default Favicon's Color

[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes. CloseButtonColor has been refactored.

BUG=621015, 625821
Committed: https://crrev.com/7d57b7e232022e031e8a0908215f9ad4eee61117
Cr-Commit-Position: refs/heads/master@{#405612}

[spqchan, 2016-07-06 20:27:31]

spqchan@chromium.org changed reviewers:
+ shrike@chromium.org

[spqchan, 2016-07-06 20:27:31]

PTAL

[shrike, 2016-07-07 23:20:28]

It seems like now the default favicon is always the default color (black or dark
gray) instead of the color of the close button's "x". I believe the coloring was
correct before, it just needed a refresh after the theme changed.

[spqchan, 2016-07-08 23:15:31]

Description was changed from

==========
[Material][Mac] Fix to update the favicon when the theme changes

BUG=621015
==========

to

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes.

BUG=621015, 625821
==========

[spqchan, 2016-07-08 23:32:10]

Description was changed from

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes.

BUG=621015, 625821
==========

to

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes. CloseButtonColor has been refactored.

BUG=621015, 625821
==========

[spqchan, 2016-07-08 23:33:21]

On 2016/07/07 23:20:28, shrike wrote:
> It seems like now the default favicon is always the default color (black or
dark
> gray) instead of the color of the close button's "x". I believe the coloring
was
> correct before, it just needed a refresh after the theme changed.

Yeah, the default icon was using the title color, not the "x" color. I made a
change so that it now uses the x one.
I also refactored a bit too.

[shrike, 2016-07-12 23:14:57]

lgtm

[spqchan, 2016-07-12 23:22:53]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2016-07-12 23:22:54]

thanks!

+rsesek for OWNERS

[Robert Sesek, 2016-07-12 23:43:56]

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_controller.h (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_controller.h:131: - (void)updateDefaultFavicon;
Does this need to be public?

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1585: [tabController
setShouldUseDefaultFavicon:NO];
Does the TabController really need to know about whether or not it should use
the default favicon? Can the TabStripController just setIconImage: correctly
below, and in themeDidChangeNotification:?

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_view.h (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_view.h:128: // the "x" and the default favicon.
What are your thoughts on the serial comma? (I like it :).

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_view.mm (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_view.mm:646: bool use_active_tab_text_color =
[self isActiveTab];
naming: camelCase in ObjC

[spqchan, 2016-07-13 17:16:14]

PTAL

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_controller.h (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_controller.h:131: - (void)updateDefaultFavicon;
On 2016/07/12 23:43:56, Robert Sesek wrote:
> Does this need to be public?

Done.

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1585: [tabController
setShouldUseDefaultFavicon:NO];
On 2016/07/12 23:43:56, Robert Sesek wrote:
> Does the TabController really need to know about whether or not it should use
> the default favicon? Can the TabStripController just setIconImage: correctly
> below, and in themeDidChangeNotification:?

The default favicon also needs to be updated at internalSetSelected:, which
isn't handled by TabStripController.

One alternative is to pass in the WebContents to the TabController and then set
a flag to "showFavicon".
The TabController will then call imageForColor from favicon_util_mac when the
image needs to be updated, regardless if it's the default favicon or not. WDYT?

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_view.h (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_view.h:128: // the "x" and the default favicon.
On 2016/07/12 23:43:56, Robert Sesek wrote:
> What are your thoughts on the serial comma? (I like it :).

I am confused if that's a smiley, or if you want me to use a colon :P

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_view.mm (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_view.mm:646: bool use_active_tab_text_color =
[self isActiveTab];
On 2016/07/12 23:43:56, Robert Sesek wrote:
> naming: camelCase in ObjC

Done.

[Robert Sesek, 2016-07-13 22:37:59]

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1585: [tabController
setShouldUseDefaultFavicon:NO];
On 2016/07/13 17:16:14, spqchan wrote:
> On 2016/07/12 23:43:56, Robert Sesek wrote:
> > Does the TabController really need to know about whether or not it should
use
> > the default favicon? Can the TabStripController just setIconImage: correctly
> > below, and in themeDidChangeNotification:?
> 
> The default favicon also needs to be updated at internalSetSelected:, which
> isn't handled by TabStripController.

Why can't the TabStripController updates the icon when it observes the selection
changing?

> One alternative is to pass in the WebContents to the TabController and then
set
> a flag to "showFavicon".
> The TabController will then call imageForColor from favicon_util_mac when the
> image needs to be updated, regardless if it's the default favicon or not.
WDYT?

No, the TabController shouldn't know about WebContents. Ideally it shouldn't
know about favicons, either.

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_view.h (right):

https://codereview.chromium.org/2126043002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_view.h:128: // the "x" and the default favicon.
On 2016/07/13 17:16:14, spqchan wrote:
> On 2016/07/12 23:43:56, Robert Sesek wrote:
> > What are your thoughts on the serial comma? (I like it :).
> 
> I am confused if that's a smiley, or if you want me to use a colon :P

It's a smiley. I would recommend the serial comma
(https://www.grammarly.com/blog/what-is-the-oxford-comma-and-why-do-people-car...).

[spqchan, 2016-07-14 20:04:25]

Good point, I made the changes accordingly. 

PTAL

[Robert Sesek, 2016-07-14 21:14:55]

LGTM

[spqchan, 2016-07-14 21:39:04]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-07-14 21:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from shrike@chromium.org
Link to the patchset: https://codereview.chromium.org/2126043002/#ps80001
(title: "Fix for rsesek 2")

[spqchan, 2016-07-14 21:39:17]

thanks!

[commit-bot: I haz the power, 2016-07-14 21:39:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 22:51:18]

Description was changed from

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes. CloseButtonColor has been refactored.

BUG=621015, 625821
==========

to

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes. CloseButtonColor has been refactored.

BUG=621015, 625821
==========

[commit-bot: I haz the power, 2016-07-14 22:51:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-14 22:52:55]

Description was changed from

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes. CloseButtonColor has been refactored.

BUG=621015, 625821
==========

to

==========
[Material][Mac] Fix Default Favicon's Color

Default favicon now has the same color as the close button. This CL
also makes sure that the favicon's color gets updated when the theme
changes. CloseButtonColor has been refactored.

BUG=621015, 625821

Committed: https://crrev.com/7d57b7e232022e031e8a0908215f9ad4eee61117
Cr-Commit-Position: refs/heads/master@{#405612}
==========

[commit-bot: I haz the power, 2016-07-14 22:52:56]

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