Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate

chrome/browser/permissions/grouped_permission_infobar_delegate.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_
 
 #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
 // which can be allowed or blocked independently.
 // TODO(tsergeant): Expand this class so it can be used without subclassing.
 class GroupedPermissionInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:

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

Nit: Make as much of this as private as possible.

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

Done.

   // Implementation is in platform-specific infobar file.
   static std::unique_ptr<infobars::InfoBar> CreateInfoBar(

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

Today it seems there is only an Android implementation of this.  Is this used on
other platforms?  Are you in the process of adding it to other platforms?  I'd
like to avoid needless flexibility/indirection/cross-platform-ness if we're not
actually going to use this in a cross-platform way.

Having a CreateInfoBar() method is very unusual.  Normally we have a Create()
method that constructs both infobar and delegate and adds the delegate to the
proper infobar manager.  Before asking for a specific design I need to
understand the requirements.

This also affects the desired identifier; if it's only used on Android, we
should have _ANDROID on it.

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

This infobar is Android-only.
I think that this design mostly just came about to simplify integration with
existing code in media_stream_infobar_delegate_android.cc. I'm open to your
suggestions to improve this.
Ack, we should add _ANDROID to the identifier in this CL.

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

OK.  Can it be moved to Android-specific files or subdirs then?  This way it's
clear the code is Android-only, and we'll have no problems just inlining
Android-specific things like creating the infobar.
Merging these two classes (see question in other reply) would help simplify
things.

In the absence of that, right now this class is an implementation detail of
MediaStreamInfoBarDelegateAndroid, and as such should probably have no public
creation functions of any kind, just a protected constructor. 
MedaStreamInfoBarDelegateAndroid should inline the code in this CreateInfoBar()
method into its Create() method directly.

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

Done.

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

On 2016/09/08 04:06:24, Peter Kasting wrote: 
Done.

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

How about we add "_android" at the end of file name to indicate that it's a
Android-specific thing? Are you OK with this approach? I think it would be
better to put it in permissions/, along with permission_infobar_delegate and
permission_update_infobar_delegate.

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

Yes, adding _android on the filename is fine.

       infobars::InfoBarManager* infobar_manager,
       std::unique_ptr<GroupedPermissionInfoBarDelegate> delegate);
 
   GroupedPermissionInfoBarDelegate(

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

Nit: InfoBarDelegate constructors should never be public.  Make protected;
public creation of infobar delegates should always go through Create()
functions.

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

Done. Create delegate within Create() method.

       const GURL& requesting_origin,
       const std::vector<ContentSettingsType>& types);
   ~GroupedPermissionInfoBarDelegate() override;
 
   base::string16 GetMessageText() const override;

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

Nit: Needs a // ConfirmInfoBarDelegate: heading.

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

Done.

 
   int GetPermissionCount() const;

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

Nit: For whatever of these are basically simple getters with no other
functionality, prefer to name them unix_hacker()-style and inline them here.

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

Done.

   ContentSettingsType GetContentSettingType(int index) const;
   int GetIconIdForPermission(int index) const;
   // Message text to display for an individual permission at |index|.

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

Nit: Blank line above any above-function comments.

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

Done.

   base::string16 GetMessageTextFragment(int index) const;
 
   void ToggleAccept(int position, bool new_value);

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

Nit: Needs descriptive comment

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

Done.

 
  protected:
   bool GetAcceptState(int position);
 
  private:
   // ConfirmInfoBarDelegate:
   base::string16 GetButtonLabel(InfoBarButton button) const override;
   int GetButtons() const override;
+  infobars::InfoBarDelegate::InfoBarIdentifier GetIdentifier() const override;

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

Nit: Move this function to the top of this group of functions (match vtable
order).  See also next comment.

Is infobars::InfoBarDelegate:: really necessary here?

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

Done.
Done.

 
   // InfoBarDelegate:

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

Nit: While here: Remove this comment, move the function here up to join the
three above, place above GetIdentifier() (match order in base class)

(Make sure to fix .cc order to match the .h order too)

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

Done.

   Type GetInfoBarType() const override;
 
   const GURL requesting_origin_;
   const std::vector<ContentSettingsType> types_;
   std::vector<bool> accept_states_;
 
   DISALLOW_COPY_AND_ASSIGN(GroupedPermissionInfoBarDelegate);
 };
 
 #endif  // CHROME_BROWSER_PERMISSIONS_GROUPED_PERMISSION_INFOBAR_DELEGATE_H_