[Material Design] Update Website Setting Icons in Omnibox on OSX

[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446, 596036
Committed: https://crrev.com/9e77ca29c851bee6722db299074b3dc0a3ba3952
Cr-Commit-Position: refs/heads/master@{#393316}

[spqchan, 2016-05-06 22:30:10]

Description was changed from

==========
[Materials] Update Website Setting Icons in Omnibox on OSX

BUG=604446
==========

to

==========
[Materials Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446
==========

[spqchan, 2016-05-06 22:33:44]

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

[spqchan, 2016-05-06 22:33:45]

PTAL

[shrike, 2016-05-06 22:59:08]

LGTM (and thank you for the code cleanup in location_bar_view_mac.mm)

https://codereview.chromium.org/1955083003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1955083003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:185: bool
did_icon_changed =
Should it be did_icon_change (without the trailing "d")?

[spqchan, 2016-05-06 23:20:25]

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

[spqchan, 2016-05-06 23:20:25]

+thakis for OWNERs

https://codereview.chromium.org/1955083003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1955083003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:185: bool
did_icon_changed =
On 2016/05/06 22:59:08, shrike wrote:
> Should it be did_icon_change (without the trailing "d")?

Done.

[spqchan, 2016-05-06 23:33:29]

Description was changed from

==========
[Materials Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446
==========

to

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446
==========

[shrike, 2016-05-10 15:45:41]

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (left):

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:192:
SetImage(content_setting_image_model_->raster_icon().ToNSImage());
One more thing: we have to keep around the old code, wrapped in a check for
whether or not we are running in Material Design. Look for the call to
ui::MaterialDesignController::IsModeMaterial() in star_decoration.mm, for
example.

[spqchan, 2016-05-10 16:22:32]

On 2016/05/10 15:45:41, shrike wrote:
>
https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm
(left):
> 
>
https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:192:
> SetImage(content_setting_image_model_->raster_icon().ToNSImage());
> One more thing: we have to keep around the old code, wrapped in a check for
> whether or not we are running in Material Design. Look for the call to
> ui::MaterialDesignController::IsModeMaterial() in star_decoration.mm, for
> example.

I actually refactored and moved some of the logic from here to
content_setting_image_model so that we can avoid redundant IsModeMaterial()
checks.
The check for Material Design is inside content_setting_image_model and the
object will return raster_image() if IsModeMaterial() is set to false.

[shrike, 2016-05-10 17:14:18]

On 2016/05/10 16:22:32, spqchan wrote:
> On 2016/05/10 15:45:41, shrike wrote:
> >
>
https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
> > File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm
> (left):
> > 
> >
>
https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
> > chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:192:
> > SetImage(content_setting_image_model_->raster_icon().ToNSImage());
> > One more thing: we have to keep around the old code, wrapped in a check for
> > whether or not we are running in Material Design. Look for the call to
> > ui::MaterialDesignController::IsModeMaterial() in star_decoration.mm, for
> > example.
> 
> I actually refactored and moved some of the logic from here to
> content_setting_image_model so that we can avoid redundant IsModeMaterial()
> checks.
> The check for Material Design is inside content_setting_image_model and the
> object will return raster_image() if IsModeMaterial() is set to false.

OK, got it.

[spqchan, 2016-05-10 17:15:21]

Description was changed from

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446
==========

to

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446, 596036
==========

[Nico, 2016-05-11 19:13:33]

lgtm

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:192: ?
SkColorSetA(SK_ColorWHITE, 0xCC)
we try to use named constants instead of putting magic numbers into the source,
see e.g.
https://code.google.com/p/chromium/codesearch#search/&q=%5Econst.*skcolor%20f...
https://code.google.com/p/chromium/codesearch#search/&q=%5Econst.*uint%20file...
(but there are several other examples)

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:192: // Returns
true if the location bar is dark.
comment just repeats the method name

[spqchan, 2016-05-12 17:40:05]

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:192: ?
SkColorSetA(SK_ColorWHITE, 0xCC)
On 2016/05/11 19:13:33, Nico wrote:
> we try to use named constants instead of putting magic numbers into the
source,
> see e.g.
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Econst.*skcolor%20f...
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Econst.*uint%20file...
> (but there are several other examples)

Done.

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/1955083003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:192: // Returns
true if the location bar is dark.
On 2016/05/11 19:13:33, Nico wrote:
> comment just repeats the method name

Done.

[spqchan, 2016-05-12 17:40:10]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-05-12 17:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from shrike@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1955083003/#ps80001
(title: "comment nitpick")

[commit-bot: I haz the power, 2016-05-12 17:40:42]

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

[commit-bot: I haz the power, 2016-05-12 17:53:00]

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

[commit-bot: I haz the power, 2016-05-12 17:53:01]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[spqchan, 2016-05-12 18:01:02]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-05-12 18:01:02]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
shrike@chromium.org
Link to the patchset: https://codereview.chromium.org/1955083003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-05-12 18:01:21]

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

[commit-bot: I haz the power, 2016-05-12 18:43:12]

Description was changed from

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446, 596036
==========

to

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446, 596036
==========

[commit-bot: I haz the power, 2016-05-12 18:43:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-12 18:44:44]

Description was changed from

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446, 596036
==========

to

==========
[Material Design] Update Website Setting Icons in Omnibox on OSX

Update the icons to use vector graphics. The website setting icons
include Javascript, cookies and images.

BUG=604446, 596036

Committed: https://crrev.com/9e77ca29c851bee6722db299074b3dc0a3ba3952
Cr-Commit-Position: refs/heads/master@{#393316}
==========

[commit-bot: I haz the power, 2016-05-12 18:44:45]

Patchset 6 (id:??) landed as
https://crrev.com/9e77ca29c851bee6722db299074b3dc0a3ba3952
Cr-Commit-Position: refs/heads/master@{#393316}