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

[Peter Kasting, 2016/09/21 18:44:39]

Nit: Configures an InfoBar to display -> An infobar that displays

[lshang, 2016/09/22 02:11:31]

Done.

 // which can be allowed or blocked independently.
 // TODO(tsergeant): Expand this class so it can be used without subclassing.
 class GroupedPermissionInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
+  ~GroupedPermissionInfoBarDelegate() override;

[Peter Kasting, 2016/09/21 18:44:39]

Nit: May be able to make private as well, depending on how this is owned.

[lshang, 2016/09/22 02:11:31]

This can't be private, since we have grouped permission infobar.

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

I don't really understand that reply.

Normally the infobar delegate is only destroyed by the core infobar system,
which does it through polymorphism, so making the destructor private doesn't
cause issues.  Is there a particular line of code that doesn't compile if you do
this?

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

Yeah, the compiler seems complaining about one line in
GroupedPermissionInfoBarDelegate::Create() in
grouped_permission_infobar_delegate_android.cc:

infobars::InfoBar* GroupedPermissionInfoBarDelegate::Create(...){
  ...
  base::MakeUnique<GroupedPermissionInfoBar>(base::WrapUnique(
                                             ^
      new GroupedPermissionInfoBarDelegate(requesting_origin, types))));
}

I also took a look at other infobar delegates, for those which has corresponding
infobars(inherit from ConfirmInfoBar), they Create() in the same way and their
destructors remain public. And for those which just has a delegate and
CreateConfirmInfoBar() in Create(), their destructors can be public.

[Peter Kasting, 2016/09/27 02:05:42]

I think the problem is that GroupedPermissionInfoBar() takes a
std::unique_ptr<GroupedPermissionInfoBarDelegate> as opposed to a
std::unique_ptr<ConfirmInfoBarDelegate>.  Probably if you changed that (and
maybe also added a <ConfirmInfoBarDelegate> to the WrapUnique() call here, not
sure if that'd really be necessary though) this would work.

However, it's kind of nice to have the declaration that way, as this makes sure
there's a compile-time check on what's passed to GroupedPermissionInfoBar, and
you can't just pass in any old thing and have it static_casted blindly later. 
Given that the constructor is already private, leaving the destructor public
seems like the better route to me.

Might be worth a comment on the destructor:

  // Public so we can have std::unique_ptr<GroupedPermissionInfoBarDelegate>.

[lshang, 2016/09/28 00:55:05]

Done.

+
   // Implementation is in platform-specific infobar file.

[Peter Kasting, 2016/09/21 18:44:39]

Nit: This comment should disappear.

[lshang, 2016/09/22 02:11:31]

Done.

-  static std::unique_ptr<infobars::InfoBar> CreateInfoBar(
-      infobars::InfoBarManager* infobar_manager,
-      std::unique_ptr<GroupedPermissionInfoBarDelegate> delegate);
-
-  GroupedPermissionInfoBarDelegate(
+  static infobars::InfoBar* Create(
+      InfoBarService* infobar_service,
       const GURL& requesting_origin,
       const std::vector<ContentSettingsType>& types);
-  ~GroupedPermissionInfoBarDelegate() override;
 
   bool ShouldShowPersistenceToggle() const;

[Peter Kasting, 2016/09/21 18:44:39]

Nit: This deserves a comment, I think.

[lshang, 2016/09/22 02:11:31]

Done.

   bool persist() const { return persist_; }

[Peter Kasting, 2016/09/21 18:44:39]

Nit: I suggest grouping all cheap inlined accessors into a block, and then
placing all the non-cheap functions below them.

[lshang, 2016/09/22 02:11:30]

Done.

   void set_persist(bool persist) { persist_ = persist; }
 
+  size_t GetPermissionCount() const { return types_.size(); }

[Peter Kasting, 2016/09/21 18:44:40]

Nit: Inlined accessors should be named unix_hacker()-style and with no "get".

[lshang, 2016/09/22 02:11:31]

Done.

+  ContentSettingsType GetContentSettingType(size_t index) const;
+  int GetIconIdForPermission(size_t index) const;
+
+  // ConfirmInfoBarDelegate:
   base::string16 GetMessageText() const override;

[Peter Kasting, 2016/09/21 18:44:39]

Nit: Place overrides either above or below non-overrides, not in the middle.

[lshang, 2016/09/22 02:11:31]

Done.

 
-  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;
+  base::string16 GetMessageTextFragment(size_t index) const;
 
-  void ToggleAccept(int position, bool new_value);
+  // Toggle Allow/Block for each permission type.
+  void ToggleAccept(size_t position, bool new_value);

[Peter Kasting, 2016/09/21 18:44:39]

Nit: Why |index| on most APIs, but |position| on this and the next?

[lshang, 2016/09/22 02:11:31]

Done. Changed them all to |position|.

 
  protected:
-  bool GetAcceptState(int position);
+  bool GetAcceptState(size_t position);
 
  private:
+  GroupedPermissionInfoBarDelegate(
+      const GURL& requesting_origin,
+      const std::vector<ContentSettingsType>& types);
+
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(GroupedPermissionInfoBarDelegate);
 };
 
-#endif  // CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_H_
+// Implementation is in platform-specific infobar file.

[Peter Kasting, 2016/09/21 18:44:39]

Seems like you have an Android-specific infobar and an Android-specific infobar
delegate, so you don't really need the abstraction of this function, or for it
to be placed in this file.  Caller code can just create/access the infobar
directly and its constructor could be public.

[lshang, 2016/09/22 02:11:30]

Done. Also referred to other infobars, Removed this abstraction and create the
infobar directly in delegate's Create() method.

+std::unique_ptr<infobars::InfoBar> CreateGroupedPermissionInfoBar(
+    std::unique_ptr<GroupedPermissionInfoBarDelegate> delegate);
+
+#endif  // CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_ANDROID_H_