Make PermissionRequest::GetIconId return different types

chrome/browser/permissions/permission_request_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_request_impl.h"
 
 #include "build/build_config.h"
 #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,
     Profile* profile,
     bool has_gesture,
     const PermissionDecidedCallback& permission_decided_callback,
     const base::Closure delete_callback)
     : request_origin_(request_origin),
       permission_type_(permission_type),
       profile_(profile),
       has_gesture_(has_gesture),
       permission_decided_callback_(permission_decided_callback),
       delete_callback_(delete_callback),
       is_finished_(false),
       action_taken_(false) {}
 
 PermissionRequestImpl::~PermissionRequestImpl() {
   DCHECK(is_finished_);
   if (!action_taken_) {
     PermissionUmaUtil::PermissionIgnored(permission_type_, GetGestureType(),
                                          request_origin_, profile_);
   }
 }
 
-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;
 #if defined(ENABLE_NOTIFICATIONS)
     case content::PermissionType::NOTIFICATIONS:
     case content::PermissionType::PUSH_MESSAGING:
       return gfx::VectorIconId::NOTIFICATIONS;
 #endif
 #if defined(OS_CHROMEOS)
     // TODO(xhwang): fix this icon, see crrev.com/863263007
     case content::PermissionType::PROTECTED_MEDIA_IDENTIFIER:
       return gfx::VectorIconId::CHROME_PRODUCT;
 #endif
     case content::PermissionType::MIDI_SYSEX:
       return gfx::VectorIconId::MIDI;
     case content::PermissionType::FLASH:
       return gfx::VectorIconId::EXTENSION;
     default:
       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_) {
     case content::PermissionType::GEOLOCATION:
       message_id = IDS_GEOLOCATION_INFOBAR_PERMISSION_FRAGMENT;
       break;
 #if defined(ENABLE_NOTIFICATIONS)
     case content::PermissionType::NOTIFICATIONS:
     case content::PermissionType::PUSH_MESSAGING:
       message_id = IDS_NOTIFICATION_PERMISSIONS_FRAGMENT;
@@ skipping 69 matching lines @@
       NOTREACHED();
       return PermissionRequestType::UNKNOWN;
   }
 }
 
 PermissionRequestGestureType PermissionRequestImpl::GetGestureType()
     const {
   return has_gesture_ ? PermissionRequestGestureType::GESTURE
                       : PermissionRequestGestureType::NO_GESTURE;
 }