Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate

chrome/browser/permissions/grouped_permission_infobar_delegate.cc

 // Copyright 2016 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/grouped_permission_infobar_delegate.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/url_formatter/elide_url.h"
 #include "ui/base/l10n/l10n_util.h"
 
 GroupedPermissionInfoBarDelegate::GroupedPermissionInfoBarDelegate(
     const GURL& requesting_origin,
     const std::vector<ContentSettingsType>& types)
     : requesting_origin_(requesting_origin),
       types_(types),
       accept_states_(types_.size(), true) {}
 
 GroupedPermissionInfoBarDelegate::~GroupedPermissionInfoBarDelegate() {}
 
 infobars::InfoBarDelegate::Type
 GroupedPermissionInfoBarDelegate::GetInfoBarType() const {
   return PAGE_ACTION_TYPE;
 }
 
 int GroupedPermissionInfoBarDelegate::GetButtons() const {
   if (GetPermissionCount() >= 2)

[Peter Kasting, 2016/09/08 04:06:24]

Nit: Prefer "> 1" to ">= 2" (slightly conceptually simpler) (2 places)

[lshang, 2016/09/21 07:09:32]

Done.

     return ConfirmInfoBarDelegate::InfoBarButton::BUTTON_OK;
   else

[Peter Kasting, 2016/09/08 04:06:24]

Nit: No else after return (3 places)

[lshang, 2016/09/21 07:09:33]

Done.

     return ConfirmInfoBarDelegate::GetButtons();
 }
 
+infobars::InfoBarDelegate::InfoBarIdentifier
+GroupedPermissionInfoBarDelegate::GetIdentifier() const {
+  return GROUPED_PERMISSION_INFOBAR_DELEGATE;
+}
+
 base::string16 GroupedPermissionInfoBarDelegate::GetButtonLabel(
     InfoBarButton button) const {
   if (GetPermissionCount() >= 2) {
     return ConfirmInfoBarDelegate::GetButtonLabel(button);

[Peter Kasting, 2016/09/08 04:06:24]

It's really odd that the function above uses the base implementation for the ==1
case, whereas this uses the base for the >=1 case.

MediaStreamInfoBarDelegateAndroid seems to be the only subclass of this
delegate, and the only way it's implemented.  It handles Accept() and Cancel(),
but not things like GetButtonLabel().  That's an unusual division of labor.  Are
there imminently going to be more subclasses?  Do they all want the same labels
but completely unrelated button handling?  Also, how is the identifier you're
adding here ever actually used if there's always a subclass (currently, always
the same subclass) which will have its own, more specific identifier?

And the behavior is strange too.  If there's one permission, you get "accept"
and "deny" but if there are more you get "OK", which accepts the inline
permission changes, or if you don't want to accept them you hit the infobar
close button?  That's atypical of Chrome's permissions UI, e.g. the settings
page which is "changes are instant, there is no "accept/OK" step".  That latter
would argue for having no OK button at all in the latter case, just the close
button, and applying permission changes instantly.

[tsergeant, 2016/09/08 05:19:57]

This is an artifact of how the UI works out. It might make sense to remove these
calls to the base class entirely so the buttons/strings are hardcoded in this
class in the exact way they are used.
The eventual plan (go/chrome-permissionrequestmanager-android, which doesn't
cover infobars in detail, but should give an overview of what we're working
towards) is that GroupedPermissionInfoBarDelegate will have no subclasses.

Having MediaStreamInfoBarDelegateAndroid subclass it was a convenience measure
to make the initial implementation steps easier.

Eventually all permission requests, including the MediaStream ones, will be
routed through PermissionRequestManager, which will create Grouped infobars as
required.
This is the equivalent of some existing behavior on Desktop:

1. Open https://permission.site
2. Click camera to open a permission prompt
3. While that prompt is open, click two other buttons to queue up prompts
4. Make a decision on the camera prompt
5. A new prompt opens with the two permissions and an OK button.

As for removing the OK button -- the close button already has a specific meaning
in permission prompts ('decline to make a decision'), so it seems difficult to
mix these two concepts.

[Peter Kasting, 2016/09/08 06:24:08]

I think that would probably make sense.  I would avoid relying on the base class
implementations "sometimes" like this, and that just goes double when different
functions do it in reversed conditions.
Maybe it makes sense to add this identifier at the time when this class is first
able to be instantiated/used separately then.

In the meantime, is the right thing to do to merge
MediaStreamInfoBarDelegateAndroid up into this class?
Hmm, I don't get that on Win Dev.  I just get successive individual prompts.
Without being able to see the multi-permission UI above, it's hard to comment. 
I would assume that "decline to state" would be something we'd want to be able
to set on an individual case ("accept permissions A and C, do nothing about B")
so having it as a reply to the whole dialog wouldn't really be sufficient.

   } else {
     return l10n_util::GetStringUTF16(
         (button == BUTTON_OK) ? IDS_PERMISSION_ALLOW : IDS_PERMISSION_DENY);
   }
 }
 
 base::string16 GroupedPermissionInfoBarDelegate::GetMessageText() const {
   return l10n_util::GetStringFUTF16(
       IDS_PERMISSIONS_BUBBLE_PROMPT,
       url_formatter::FormatUrlForSecurityDisplay(requesting_origin_));
 }
 
 int GroupedPermissionInfoBarDelegate::GetPermissionCount() const {
   return types_.size();

[Peter Kasting, 2016/09/08 04:06:24]

Nit: This is an implicit cast from size_t to int.  Don't do that; return a
size_t directly.

[lshang, 2016/09/21 07:09:32]

Done.

 }
 
 ContentSettingsType GroupedPermissionInfoBarDelegate::GetContentSettingType(
     int index) const {
   DCHECK(index >= 0 && index < static_cast<int>(types_.size()));
   return types_[index];

[Peter Kasting, 2016/09/08 04:06:24]

Nit: This line means this function should be taking a size_t.

This would also make the DCHECK above simple:

  DCHECK_LT(position, types_.size());

This comment applies to the next four functions as well.

[lshang, 2016/09/21 07:09:32]

Done.

 }
 
 int GroupedPermissionInfoBarDelegate::GetIconIdForPermission(int index) const {
   DCHECK(index >= 0 && index < static_cast<int>(types_.size()));
   ContentSettingsType type = types_[index];
   if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
     return IDR_INFOBAR_MEDIA_STREAM_MIC;
   } else if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
     return IDR_INFOBAR_MEDIA_STREAM_CAMERA;
   }
   return IDR_INFOBAR_WARNING;
 }
 
 base::string16 GroupedPermissionInfoBarDelegate::GetMessageTextFragment(
     int index) const {
   DCHECK(index >= 0 && index < static_cast<int>(types_.size()));
   ContentSettingsType type = types_[index];
   int message_id;
   if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
     message_id = IDS_MEDIA_CAPTURE_AUDIO_ONLY_PERMISSION_FRAGMENT;
   } else if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
     message_id = IDS_MEDIA_CAPTURE_VIDEO_ONLY_PERMISSION_FRAGMENT;
   } else {
     NOTREACHED();
     return base::string16();
   }
   return l10n_util::GetStringUTF16(message_id);

[Peter Kasting, 2016/09/08 04:06:24]

Nit: Simpler:

  const bool is_mic = (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC);
  DCHECK(is_mic || (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA));
  return l10n_util::GetStringUTF16(
      is_mic ? IDS_MEDIA_CAPTURE_AUDIO_ONLY_PERMISSION_FRAGMENT
             : IDS_MEDIA_CAPTURE_VIDEO_ONLY_PERMISSION_FRAGMENT);

[lshang, 2016/09/21 07:09:32]

Done.

 }
 
 void GroupedPermissionInfoBarDelegate::ToggleAccept(int position,
                                                     bool new_value) {
   DCHECK(position >= 0 && position < static_cast<int>(types_.size()));
   accept_states_[position] = new_value;
 }
 
 bool GroupedPermissionInfoBarDelegate::GetAcceptState(int position) {
   DCHECK(position >= 0 && position < static_cast<int>(types_.size()));
   return accept_states_[position];
 }