Make PermissionRequest::GetIconId return different types

Make PermissionRequest::GetIconId return different types
on desktop and Android so that the IDR_ and vector id
versions don't coexist on any single platform.

BUG=651270
Committed: https://crrev.com/20c051a9a415b64187d410d0ec207c0442d784f5
Cr-Commit-Position: refs/heads/master@{#425577}

[Evan Stade, 2016-10-04 23:43:00]

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

[commit-bot: I haz the power, 2016-10-04 23:43:31]

Dry run: 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-10-05 00:04:59]

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

[commit-bot: I haz the power, 2016-10-05 00:05:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2016-10-05 19:09:12]

Description was changed from

==========
Remove PermissionRequest::GetIconId in favor of vector version

BUG=651270
==========

to

==========
On desktop, remove PermissionRequest::GetIconId in favor of vector version

BUG=651270
==========

[Evan Stade, 2016-10-05 19:09:47]

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

[commit-bot: I haz the power, 2016-10-05 19:10:24]

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

[Evan Stade, 2016-10-05 19:15:27]

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

[commit-bot: I haz the power, 2016-10-05 19:16:02]

Dry run: 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-10-05 19:33:13]

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

[commit-bot: I haz the power, 2016-10-05 19:33:13]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2016-10-06 00:08:57]

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

[commit-bot: I haz the power, 2016-10-06 00:09:31]

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

[Evan Stade, 2016-10-06 00:09:44]

Description was changed from

==========
On desktop, remove PermissionRequest::GetIconId in favor of vector version

BUG=651270
==========

to

==========
Make PermissionRequest::GetIconId return different types
on desktop and Android so that the IDR_ and vector id
versions don't coexist on any single platform.

BUG=651270
==========

[Evan Stade, 2016-10-06 00:09:44]

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

[commit-bot: I haz the power, 2016-10-06 00:35:13]

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

[commit-bot: I haz the power, 2016-10-06 00:35:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Peter Kasting, 2016-10-06 07:10:30]

LGTM

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_capture_indicator.cc (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_capture_indicator.cc:359: //
TODO(estade): these should use vector icons.
Nit: crbug.com/505953?  Or is there another one?

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:283: return
gfx::VectorIconId::MICROPHONE;
Nit: Shorter:

  return IsAskingForVideo() ? gfx::VectorIconId::VIDEOCAM
                            : gfx::VectorIconId::MICROPHONE;

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request.h (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request.h:64: // On Android, icons are
represented with an IDR_ identifier.
Are we planning to convert Android to vector icons someday?  If so, is there a
bug on this, that we could link as a TODO here?

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_impl.cc (left):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_impl.cc:74: int
PermissionRequestImpl::GetIconId() const {
I guess if this used to be reachable, then the new code won't compile on
Android, because VectorIconId is an enum class and won't implicitly convert to
IconId (== int), right?  So if it compiles removing this is safe.

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:226:
gfx::VectorIconId vector_id = requests[index]->GetIconId();
Nit: Seems like this should technically be

    PermissionRequest::IconId icon_id = ...;  // Or just use auto

[Evan Stade, 2016-10-06 15:33:50]

estade@chromium.org changed reviewers:
+ felt@chromium.org, thakis@chromium.org

[Evan Stade, 2016-10-06 15:33:51]

OWNERs, please review:

felt:
  chrome/browser/permissions/

thakis:
  chrome/browser/custom_handlers/
  chrome/app/theme/
  chrome/browser/android/
  chrome/browser/chrome_quota_permission_context.cc
  chrome/browser/download/
  chrome/browser/media/webrtc/

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_capture_indicator.cc (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_capture_indicator.cc:359: //
TODO(estade): these should use vector icons.
On 2016/10/06 07:10:29, Peter Kasting (OOO Oct. 6-9) wrote:
> Nit: crbug.com/505953?  Or is there another one?

there is not another one filed. Dunno if you wanted me to link to that one, but
it doesn't seem all that useful (easy enough to find resources that still need
removing by looking in chrome/app/theme)

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:283: return
gfx::VectorIconId::MICROPHONE;
On 2016/10/06 07:10:29, Peter Kasting (OOO Oct. 6-9) wrote:
> Nit: Shorter:
> 
>   return IsAskingForVideo() ? gfx::VectorIconId::VIDEOCAM
>                             : gfx::VectorIconId::MICROPHONE;

Done.

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request.h (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request.h:64: // On Android, icons are
represented with an IDR_ identifier.
On 2016/10/06 07:10:30, Peter Kasting (OOO Oct. 6-9) wrote:
> Are we planning to convert Android to vector icons someday?  If so, is there a
> bug on this, that we could link as a TODO here?

I'm not aware of any such plans.

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_impl.cc (left):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_impl.cc:74: int
PermissionRequestImpl::GetIconId() const {
On 2016/10/06 07:10:30, Peter Kasting (OOO Oct. 6-9) wrote:
> I guess if this used to be reachable, then the new code won't compile on
> Android, because VectorIconId is an enum class and won't implicitly convert to
> IconId (== int), right?  So if it compiles removing this is safe.

I noticed this file shouldn't even be compiled on Android so I updated the
BUILD.gn

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2385063005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:226:
gfx::VectorIconId vector_id = requests[index]->GetIconId();
On 2016/10/06 07:10:30, Peter Kasting (OOO Oct. 6-9) wrote:
> Nit: Seems like this should technically be
> 
>     PermissionRequest::IconId icon_id = ...;  // Or just use auto

I dunno, because at some point you do have to assume they're the same (in order
to pass it to CreateVectorIcon)

[Evan Stade, 2016-10-06 15:34:21]

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

[commit-bot: I haz the power, 2016-10-06 15:34:43]

Dry run: 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-10-06 15:56:21]

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

[commit-bot: I haz the power, 2016-10-06 15:56:22]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2016-10-06 16:38:51]

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

[commit-bot: I haz the power, 2016-10-06 16:39:22]

Dry run: 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-10-06 17:04:32]

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

[commit-bot: I haz the power, 2016-10-06 17:04:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2016-10-10 18:38:47]

ping Nico, Adrienne

[Evan Stade, 2016-10-10 18:39:23]

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

[commit-bot: I haz the power, 2016-10-10 18:39:40]

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

[Nico, 2016-10-10 18:57:47]

What's the desired end game here? Will Android always use bitmaps and desktop
always vectors?

[commit-bot: I haz the power, 2016-10-10 19:45:14]

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

[commit-bot: I haz the power, 2016-10-10 19:45:15]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-10 21:05:05]

On 2016/10/10 18:57:47, Nico wrote:
> What's the desired end game here? Will Android always use bitmaps and desktop
> always vectors?

For now that is the plan.

[felt, 2016-10-10 21:35:56]

+dominickn and tsergeant on cc as FYI

[felt, 2016-10-10 21:39:44]

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:74: int
PermissionRequestImpl::GetIconId() const {
Where did this move to?

[Evan Stade, 2016-10-10 22:17:43]

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:74: int
PermissionRequestImpl::GetIconId() const {
On 2016/10/10 21:39:44, felt wrote:
> Where did this move to?

this file wasn't actually being used on android. The function was just removed.

It's only used on desktop:
https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_co...

[tsergeant, 2016-10-10 23:54:17]

Also ccing lshang@, who is working to bring the PermissionRequest classes to
Android.

[felt, 2016-10-11 00:30:13]

felt@chromium.org changed reviewers:
+ lshang@chromium.org

[felt, 2016-10-11 00:30:43]

lshang, can you please review?

[Evan Stade, 2016-10-11 00:32:46]

On 2016/10/11 00:30:43, felt wrote:
> lshang, can you please review?

Note that if we did in the future want to use PermissionRequestImpl on android,
it should be easy to restore the contents of the GetIconId function that I've
removed. I only removed it because it's not currently useful.

[felt, 2016-10-11 00:36:35]

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:40:
PermissionRequest::IconId PermissionRequestImpl::GetIconId() const {
I find this part a little confusing. Someone reading this code might think that
this function ought to work on Android, but obviously it won't. Since the IconId
abstracts that away. Thinking about what might make this more obvious --
perhaps, could you add an if def with a NOTREACHED() for Android?

[Evan Stade, 2016-10-11 00:45:17]

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:40:
PermissionRequest::IconId PermissionRequestImpl::GetIconId() const {
On 2016/10/11 00:36:35, felt wrote:
> I find this part a little confusing. Someone reading this code might think
that
> this function ought to work on Android, but obviously it won't. Since the
IconId
> abstracts that away. Thinking about what might make this more obvious --
> perhaps, could you add an if def with a NOTREACHED() for Android?

This entire file is not compiled on Android. It doesn't seem like the best way
to make that obvious is to add a runtime check. Do we do that elsewhere in
Chrome?

[Peter Kasting, 2016-10-11 00:48:51]

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:40:
PermissionRequest::IconId PermissionRequestImpl::GetIconId() const {
On 2016/10/11 00:45:17, Evan Stade wrote:
> On 2016/10/11 00:36:35, felt wrote:
> > I find this part a little confusing. Someone reading this code might think
> that
> > this function ought to work on Android, but obviously it won't. Since the
> IconId
> > abstracts that away. Thinking about what might make this more obvious --
> > perhaps, could you add an if def with a NOTREACHED() for Android?
> 
> This entire file is not compiled on Android. It doesn't seem like the best way
> to make that obvious is to add a runtime check. Do we do that elsewhere in
> Chrome?

I would be somewhat unhappy if we added #if PLATFORM_X code to a file not used
on platform X.  That seems misleading.

If it's really unclear how it is, I would consider adding a simple comment like
"No need for an Android version of this, since this file is not compiled on
Android."

[lshang, 2016-10-11 12:26:57]

https://codereview.chromium.org/2385063005/diff/100001/chrome/app/theme/theme...
File chrome/app/theme/theme_resources.grd (left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/app/theme/theme...
chrome/app/theme/theme_resources.grd:264: <structure type="chrome_scaled_image"
name="IDR_INFOBAR_MEDIA_STREAM_SCREEN"
file="common/infobar_media_stream_screen_share.png" />
Why removing these images?

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
File
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc
(left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc:48:
return gfx::VectorIconId::VECTOR_ICON_NONE;
Did you remove this because this file will never get compiled on MACOSX? I can
only see it's non-android from BUILD.gn file, but don't know if there's other
guarantee of it. Otherwise, it'll return  gfx::VectorIconId::PROTOCOL_HANDLER on
MACOSX, instead of gfx::VectorIconId::VECTOR_ICON_NONE after your change. Or is
this intended for some reason?

https://codereview.chromium.org/2385063005/diff/100001/ui/gfx/vector_icons/mi...
File ui/gfx/vector_icons/microphone.icon (right):

https://codereview.chromium.org/2385063005/diff/100001/ui/gfx/vector_icons/mi...
ui/gfx/vector_icons/microphone.icon:24: END
Why adding a new microphone icon here? What is used for microphone icon on
desktop before this change?

[Evan Stade, 2016-10-12 15:12:36]

https://codereview.chromium.org/2385063005/diff/100001/chrome/app/theme/theme...
File chrome/app/theme/theme_resources.grd (left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/app/theme/theme...
chrome/app/theme/theme_resources.grd:264: <structure type="chrome_scaled_image"
name="IDR_INFOBAR_MEDIA_STREAM_SCREEN"
file="common/infobar_media_stream_screen_share.png" />
On 2016/10/11 12:26:57, lshang wrote:
> Why removing these images?

they are no longer used. On desktop, we use vector icons. On android, we use
android-specific images (this was already true but it changed from a LINK to a
DECLARE in android_resources because the desktop image is gone now).

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
File
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc
(left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc:48:
return gfx::VectorIconId::VECTOR_ICON_NONE;
On 2016/10/11 12:26:57, lshang wrote:
> Did you remove this because this file will never get compiled on MACOSX? I can
> only see it's non-android from BUILD.gn file, but don't know if there's other
> guarantee of it. Otherwise, it'll return  gfx::VectorIconId::PROTOCOL_HANDLER
on
> MACOSX, instead of gfx::VectorIconId::VECTOR_ICON_NONE after your change. Or
is
> this intended for some reason? 

vector icons work fine on osx. At the time these ifdefs code were written, that
wasn't the case.

https://codereview.chromium.org/2385063005/diff/100001/ui/gfx/vector_icons/mi...
File ui/gfx/vector_icons/microphone.icon (right):

https://codereview.chromium.org/2385063005/diff/100001/ui/gfx/vector_icons/mi...
ui/gfx/vector_icons/microphone.icon:24: END
On 2016/10/11 12:26:57, lshang wrote:
> Why adding a new microphone icon here? What is used for microphone icon on
> desktop before this change?

a png which is now deleted

[lshang, 2016-10-13 05:03:10]

lgtm

I'm working on reusing PermissionRequest on Android, seems not causing too much
trouble in doing that. Will probably need to change build.gn file back and
restore some content in this CL, but I think it's OK to me.

https://codereview.chromium.org/2385063005/diff/100001/chrome/app/theme/theme...
File chrome/app/theme/theme_resources.grd (left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/app/theme/theme...
chrome/app/theme/theme_resources.grd:264: <structure type="chrome_scaled_image"
name="IDR_INFOBAR_MEDIA_STREAM_SCREEN"
file="common/infobar_media_stream_screen_share.png" />
On 2016/10/12 15:12:35, Evan Stade (ooo till 10-20) wrote:
> On 2016/10/11 12:26:57, lshang wrote:
> > Why removing these images?
> 
> they are no longer used. On desktop, we use vector icons. On android, we use
> android-specific images (this was already true but it changed from a LINK to a
> DECLARE in android_resources because the desktop image is gone now).

Got it.

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
File
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc
(left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc:48:
return gfx::VectorIconId::VECTOR_ICON_NONE;
On 2016/10/12 15:12:36, Evan Stade (ooo till 10-20) wrote:
> On 2016/10/11 12:26:57, lshang wrote:
> > Did you remove this because this file will never get compiled on MACOSX? I
can
> > only see it's non-android from BUILD.gn file, but don't know if there's
other
> > guarantee of it. Otherwise, it'll return 
gfx::VectorIconId::PROTOCOL_HANDLER
> on
> > MACOSX, instead of gfx::VectorIconId::VECTOR_ICON_NONE after your change. Or
> is
> > this intended for some reason? 
> 
> vector icons work fine on osx. At the time these ifdefs code were written,
that
> wasn't the case.

Are the visual representation of gfx::VectorIconId::PROTOCOL_HANDLER and
IDR_REGISTER_PROTOCOL_HANDLER the same when showing to users? I'm still a little
doubt about whether this will make a UI change, but since you are more informed
about this, I'll trust you in this.

[Evan Stade, 2016-10-14 14:58:28]

estade@chromium.org changed reviewers:
+ thestig@chromium.org
- thakis@chromium.org

[Evan Stade, 2016-10-14 14:58:29]

-thakis, +thestig for:
  chrome/browser/custom_handlers/
  chrome/app/theme/
  chrome/browser/android/
  chrome/browser/chrome_quota_permission_context.cc
  chrome/browser/download/
  chrome/browser/media/webrtc/

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
File
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc
(left):

https://codereview.chromium.org/2385063005/diff/100001/chrome/browser/custom_...
chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc:48:
return gfx::VectorIconId::VECTOR_ICON_NONE;
On 2016/10/13 05:03:10, lshang wrote:
> On 2016/10/12 15:12:36, Evan Stade (ooo till 10-20) wrote:
> > On 2016/10/11 12:26:57, lshang wrote:
> > > Did you remove this because this file will never get compiled on MACOSX? I
> can
> > > only see it's non-android from BUILD.gn file, but don't know if there's
> other
> > > guarantee of it. Otherwise, it'll return 
> gfx::VectorIconId::PROTOCOL_HANDLER
> > on
> > > MACOSX, instead of gfx::VectorIconId::VECTOR_ICON_NONE after your change.
Or
> > is
> > > this intended for some reason? 
> > 
> > vector icons work fine on osx. At the time these ifdefs code were written,
> that
> > wasn't the case.
> 
> Are the visual representation of gfx::VectorIconId::PROTOCOL_HANDLER and
> IDR_REGISTER_PROTOCOL_HANDLER the same when showing to users? I'm still a
little
> doubt about whether this will make a UI change, but since you are more
informed
> about this, I'll trust you in this. 

any difference is an intentional update to better match the material design
style of iconography (I can't actually recall offhand if there is a difference
or not)

[Nico, 2016-10-14 15:52:27]

code lgtm (I'm around today)

On 2016/10/10 18:57:47, Nico (mostly afk until Oct 23) wrote:
> What's the desired end game here? Will Android always use bitmaps and desktop
> always vectors?

Why are we doing vectors again then?

[Evan Stade, 2016-10-15 21:56:42]

On 2016/10/14 15:52:27, Nico (mostly afk until Oct 23) wrote:
> code lgtm (I'm around today)

thanks!

> 
> On 2016/10/10 18:57:47, Nico (mostly afk until Oct 23) wrote:
> > What's the desired end game here? Will Android always use bitmaps and
desktop
> > always vectors?
> 
> Why are we doing vectors again then?

performance, space, better scaling for weird DSFs, tweakability, composability.
Not sure what bearing Android has on what desktop should be doing? Even with
pngs, many images are different on Android than desktop. I did hear that Android
could take a monochromatic png and colorize it. Not sure if they're actively
doing that anywhere.

[Evan Stade, 2016-10-15 21:56:48]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-15 21:56:49]

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

[commit-bot: I haz the power, 2016-10-15 21:57:00]

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-10-15 22:53:44]

Description was changed from

==========
Make PermissionRequest::GetIconId return different types
on desktop and Android so that the IDR_ and vector id
versions don't coexist on any single platform.

BUG=651270
==========

to

==========
Make PermissionRequest::GetIconId return different types
on desktop and Android so that the IDR_ and vector id
versions don't coexist on any single platform.

BUG=651270
==========

[commit-bot: I haz the power, 2016-10-15 22:53:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-15 22:55:11]

Description was changed from

==========
Make PermissionRequest::GetIconId return different types
on desktop and Android so that the IDR_ and vector id
versions don't coexist on any single platform.

BUG=651270
==========

to

==========
Make PermissionRequest::GetIconId return different types
on desktop and Android so that the IDR_ and vector id
versions don't coexist on any single platform.

BUG=651270

Committed: https://crrev.com/20c051a9a415b64187d410d0ec207c0442d784f5
Cr-Commit-Position: refs/heads/master@{#425577}
==========

[commit-bot: I haz the power, 2016-10-15 22:55:12]

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