Make PermissionRequest::GetIconId return different types

chrome/browser/permissions/permission_request_impl.cc

Index: chrome/browser/permissions/permission_request_impl.cc
diff --git a/chrome/browser/permissions/permission_request_impl.cc b/chrome/browser/permissions/permission_request_impl.cc
index 94e8b54c13873f870b93da37fc987293dd7da90b..761d1db218193dd8d32a60f81f4d8614f599b4fe 100644
--- a/chrome/browser/permissions/permission_request_impl.cc
+++ b/chrome/browser/permissions/permission_request_impl.cc
@@ -8,16 +8,11 @@
 #include "chrome/browser/permissions/permission_uma_util.h"
 #include "chrome/browser/permissions/permission_util.h"
 #include "chrome/grit/generated_resources.h"
-#include "chrome/grit/theme_resources.h"
 #include "components/url_formatter/elide_url.h"
 #include "net/base/escape.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/vector_icons_public.h"
 
-#if defined(OS_ANDROID)
-#include "chrome/browser/android/android_theme_resources.h"
-#endif
-
 PermissionRequestImpl::PermissionRequestImpl(
     const GURL& request_origin,
     content::PermissionType permission_type,
@@ -42,10 +37,7 @@ PermissionRequestImpl::~PermissionRequestImpl() {
   }
 }
 
-gfx::VectorIconId PermissionRequestImpl::GetVectorIconId() const {
-#if defined(OS_ANDROID)
-  return gfx::VectorIconId::VECTOR_ICON_NONE;
-#else
+PermissionRequest::IconId PermissionRequestImpl::GetIconId() const {

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

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]

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]

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."

   switch (permission_type_) {
     case content::PermissionType::GEOLOCATION:
       return gfx::VectorIconId::LOCATION_ON;
@@ -67,25 +59,8 @@ gfx::VectorIconId PermissionRequestImpl::GetVectorIconId() const {
       NOTREACHED();
       return gfx::VectorIconId::VECTOR_ICON_NONE;
   }
-#endif
 }
 
-#if defined(OS_ANDROID)
-int PermissionRequestImpl::GetIconId() const {

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

Where did this move to?

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

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...

-  switch (permission_type_) {
-    case content::PermissionType::GEOLOCATION:
-      return IDR_ANDROID_INFOBAR_GEOLOCATION;
-    case content::PermissionType::MIDI_SYSEX:
-      return IDR_ALLOWED_MIDI_SYSEX;
-    case content::PermissionType::FLASH:
-      return IDR_ALLOWED_PLUGINS;
-    default:
-      NOTREACHED();
-      return 0;
-  }
-}
-#endif
-
 base::string16 PermissionRequestImpl::GetMessageTextFragment() const {
   int message_id;
   switch (permission_type_) {