Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate

chrome/browser/permissions/grouped_permission_infobar_delegate_android.h

 // 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.
 
-#ifndef CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_H_
-#define CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_H_
+#ifndef CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_ANDROID_H_
+#define CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_ANDROID_H_
 
 #include <memory>
 
 #include "base/callback.h"
 #include "components/content_settings/core/common/content_settings_types.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 
 class GURL;
 class InfoBarService;
 
-namespace infobars {
-class InfoBarManager;
-}
-
-// Configures an InfoBar to display a group of permission requests, each of
+// An InfoBar that displays a group of permission requests, each of
 // which can be allowed or blocked independently.

[Peter Kasting, 2016/09/23 00:42:06]

Nit: Wrap at 80 columns

[lshang, 2016/09/27 01:50:42]

Done.

 // TODO(tsergeant): Expand this class so it can be used without subclassing.
 class GroupedPermissionInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
-  // Implementation is in platform-specific infobar file.
-  static std::unique_ptr<infobars::InfoBar> CreateInfoBar(
-      infobars::InfoBarManager* infobar_manager,
-      std::unique_ptr<GroupedPermissionInfoBarDelegate> delegate);
+  ~GroupedPermissionInfoBarDelegate() override;
 
+  static infobars::InfoBar* Create(
+      InfoBarService* infobar_service,
+      const GURL& requesting_origin,
+      const std::vector<ContentSettingsType>& types);
+
+  bool persist() const { return persist_; }
+  void set_persist(bool persist) { persist_ = persist; }
+  size_t permission_count() const { return types_.size(); }
+
+  // Returns true if the infobar should display a toggle to allow users to
+  // opt-out of persisting their accept/deny decision.
+  bool ShouldShowPersistenceToggle() const;
+
+  ContentSettingsType GetContentSettingType(size_t position) const;
+  int GetIconIdForPermission(size_t position) const;
+
+  // Message text to display for an individual permission at |position|.

[Peter Kasting, 2016/09/23 00:42:06]

Nit: Probably no need for this comment or the next one, as their meanings are
basically clear from their names, but if you do keep these comments, or add them
for the things above, use parallel structure for all comments, e.g. "Toggle
Allow/Block for an individual permission at |position|."

[lshang, 2016/09/27 01:50:42]

Done.

+  base::string16 GetMessageTextFragment(size_t position) const;
+
+  // Toggle Allow/Block for each permission type.
+  void ToggleAccept(size_t position, bool new_value);

[Peter Kasting, 2016/09/23 00:42:05]

Nit: The function name here says "accept" and the comment says "allow/block". 
Use consistent language everywhere.

[lshang, 2016/09/27 01:50:43]

Done.

+
+  // ConfirmInfoBarDelegate:
+  base::string16 GetMessageText() const override;
+
+ protected:
+  bool GetAcceptState(size_t position);
+
+ private:
   GroupedPermissionInfoBarDelegate(
       const GURL& requesting_origin,
       const std::vector<ContentSettingsType>& types);
-  ~GroupedPermissionInfoBarDelegate() override;
 
-  bool ShouldShowPersistenceToggle() const;
-  bool persist() const { return persist_; }
-  void set_persist(bool persist) { persist_ = persist; }
-
-  base::string16 GetMessageText() const override;
-
-  int GetPermissionCount() const;
-  ContentSettingsType GetContentSettingType(int index) const;
-  int GetIconIdForPermission(int index) const;
-  // Message text to display for an individual permission at |index|.
-  base::string16 GetMessageTextFragment(int index) const;
-
-  void ToggleAccept(int position, bool new_value);
-
- protected:
-  bool GetAcceptState(int position);
-
- private:
   // ConfirmInfoBarDelegate:
+  InfoBarIdentifier GetIdentifier() const override;
+  Type GetInfoBarType() const override;
+  int GetButtons() const override;
   base::string16 GetButtonLabel(InfoBarButton button) const override;
-  int GetButtons() const override;
-
-  // InfoBarDelegate:
-  Type GetInfoBarType() const override;
 
   const GURL requesting_origin_;
   const std::vector<ContentSettingsType> types_;
   std::vector<bool> accept_states_;
   bool persist_;

[Peter Kasting, 2016/09/23 00:42:06]

Nit: At least this member's meaning is not totally clear to me just from the
header.  Maybe a comment, or a clearer name about what's being persisted or
when?

[lshang, 2016/09/27 01:50:42]

Done. Added a comment for it.

 
   DISALLOW_COPY_AND_ASSIGN(GroupedPermissionInfoBarDelegate);
 };
 
-#endif  // CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_H_
+#endif  // CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_ANDROID_H_