Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319443002:
Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate (Closed)
DescriptionAdd InfoBarIdentifier to GroupedPermissionInfoBarDelegate
This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage
other permission types.
BUG=606138
Committed: https://crrev.com/3b4d59bb5648b72721014642f3a72685547dd589
Cr-Commit-Position: refs/heads/master@{#421758}
Patch Set 1 #
Total comments: 36
Patch Set 2 : address review comments #Patch Set 3 : address review comments #
Total comments: 30
Patch Set 4 : address comments #
Total comments: 17
Patch Set 5 : address review comments #Patch Set 6 : minor change in comments #Messages
Total messages: 63 (44 generated)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate BUG= ========== to ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138, 458606 ==========
lshang@chromium.org changed reviewers: + tsergeant@chromium.org
Tim, PTAL thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138, 458606 ========== to ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138 ==========
lgtm
lshang@chromium.org changed reviewers: + jwd@chromium.org, pkasting@chromium.org, raymes@chromium.org
+raymes@ for chrome/browser/permissions/ +pkasting@ for components/infobars/ +jwd@ for tools/metrics/ PTAL, thanks! :-)
A lot of the questions and nits here are not precisely related to this change, but it seems this code was written previously without my review, and the previous authors/reviewers were less familiar with infobar conventions. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:26: if (GetPermissionCount() >= 2) Nit: Prefer "> 1" to ">= 2" (slightly conceptually simpler) (2 places) https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:28: else Nit: No else after return (3 places) https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:40: return ConfirmInfoBarDelegate::GetButtonLabel(button); 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. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:54: return types_.size(); Nit: This is an implicit cast from size_t to int. Don't do that; return a size_t directly. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:60: return types_[index]; 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. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:87: return l10n_util::GetStringUTF16(message_id); 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); https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.h (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:25: public: Nit: Make as much of this as private as possible. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( 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. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:31: GroupedPermissionInfoBarDelegate( Nit: InfoBarDelegate constructors should never be public. Make protected; public creation of infobar delegates should always go through Create() functions. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:36: base::string16 GetMessageText() const override; Nit: Needs a // ConfirmInfoBarDelegate: heading. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:38: int GetPermissionCount() const; Nit: For whatever of these are basically simple getters with no other functionality, prefer to name them unix_hacker()-style and inline them here. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:41: // Message text to display for an individual permission at |index|. Nit: Blank line above any above-function comments. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:44: void ToggleAccept(int position, bool new_value); Nit: Needs descriptive comment https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:53: infobars::InfoBarDelegate::InfoBarIdentifier GetIdentifier() const override; 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? https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:55: // InfoBarDelegate: 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)
Thanks for the detailed review, Peter. As the original author of this class, I've answered some of the design questions below. The gist of it is basically that most of these decisions were made to integrate simply with existing code, but we're happy to change them as we move away from existing code and build our own integration from scratch. Since almost all of these comments are unrelated to the patch at hand, would you be happy for your suggestions to be implemented in a separate CL? https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:40: return ConfirmInfoBarDelegate::GetButtonLabel(button); On 2016/09/08 04:06:24, Peter Kasting wrote: > 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. 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. > 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? 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. > 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. 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. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.h (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( On 2016/09/08 04:06:24, Peter Kasting wrote: > 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. This infobar is Android-only. > 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. 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. > This also affects the desired identifier; if it's only used on Android, we > should have _ANDROID on it. Ack, we should add _ANDROID to the identifier in this CL.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/08 05:19:57, tsergeant wrote: > Thanks for the detailed review, Peter. As the original author of this class, > I've answered some of the design questions below. Thank you! > Since almost all of these comments are unrelated to the patch at hand, would you > be happy for your suggestions to be implemented in a separate CL? I'm not opposed to splitting things apart in principle. For the original purpose of this CL specifically -- adding a new unique identifier -- I think it might not make sense to do that as a standalone change before making this class independently instantiable; see reply below. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:40: return ConfirmInfoBarDelegate::GetButtonLabel(button); On 2016/09/08 05:19:57, tsergeant wrote: > On 2016/09/08 04:06:24, Peter Kasting wrote: > > 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. > > 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. 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. > > 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? > > 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. 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? > > 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. > > 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. Hmm, I don't get that on Win Dev. I just get successive individual prompts. > 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. 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. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.h (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( On 2016/09/08 05:19:57, tsergeant wrote: > On 2016/09/08 04:06:24, Peter Kasting wrote: > > 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. > > This infobar is Android-only. 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. > > 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. > > 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. 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.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/08 06:24:09, Peter Kasting wrote: > > > Since almost all of these comments are unrelated to the patch at hand, would > you > > be happy for your suggestions to be implemented in a separate CL? > > I'm not opposed to splitting things apart in principle. For the original > purpose of this CL specifically -- adding a new unique identifier -- I think it > might not make sense to do that as a standalone change before making this class > independently instantiable; see reply below. > > > > > 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? > > > > 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. > > 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? Thanks for your patient comments Peter! I've discussed with tsergeant@ and we think your idea is the right way to go. We'll first work on removing MediaStreamInfoBarDelegateAndroid subclass and then come back to this CL. Please wait for updates, thanks!
histograms lgtm
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:120001) has been deleted
Patchset #3 (id:160001) has been deleted
pkasting@: thanks a lot for your detailed comments, really learned a lot! I've updated this CL to address your comments. Could you take a look at this CL again? Thanks! https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:26: if (GetPermissionCount() >= 2) On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: Prefer "> 1" to ">= 2" (slightly conceptually simpler) (2 places) Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:28: else On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: No else after return (3 places) Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:54: return types_.size(); On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: This is an implicit cast from size_t to int. Don't do that; return a > size_t directly. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:60: return types_[index]; On 2016/09/08 04:06:24, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.cc:87: return l10n_util::GetStringUTF16(message_id); On 2016/09/08 04:06:24, Peter Kasting wrote: > 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); Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.h (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:25: public: On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: Make as much of this as private as possible. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( > > This also affects the desired identifier; if it's only used on Android, we > > should have _ANDROID on it. > > Ack, we should add _ANDROID to the identifier in this CL. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( On 2016/09/08 04:06:24, Peter Kasting wrote: > 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. > Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( On 2016/09/08 06:24:08, Peter Kasting wrote: > On 2016/09/08 05:19:57, tsergeant wrote: > > On 2016/09/08 04:06:24, Peter Kasting wrote: > > > 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. > > > > This infobar is Android-only. > > 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. > 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. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:31: GroupedPermissionInfoBarDelegate( On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: InfoBarDelegate constructors should never be public. Make protected; > public creation of infobar delegates should always go through Create() > functions. Done. Create delegate within Create() method. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:36: base::string16 GetMessageText() const override; On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: Needs a // ConfirmInfoBarDelegate: heading. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:38: int GetPermissionCount() const; On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: For whatever of these are basically simple getters with no other > functionality, prefer to name them unix_hacker()-style and inline them here. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:41: // Message text to display for an individual permission at |index|. On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: Blank line above any above-function comments. Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:44: void ToggleAccept(int position, bool new_value); On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: Needs descriptive comment Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:53: infobars::InfoBarDelegate::InfoBarIdentifier GetIdentifier() const override; On 2016/09/08 04:06:24, Peter Kasting wrote: > Nit: Move this function to the top of this group of functions (match vtable > order). See also next comment. Done. > > Is infobars::InfoBarDelegate:: really necessary here? Done. https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:55: // InfoBarDelegate: On 2016/09/08 04:06:24, Peter Kasting wrote: > 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) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
So did "We'll first work on removing MediaStreamInfoBarDelegateAndroid subclass and then come back to this CL." go by the wayside? This definitely looks better, thanks! https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/grouped_permission_infobar_delegate.h (right): https://codereview.chromium.org/2319443002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/grouped_permission_infobar_delegate.h:27: static std::unique_ptr<infobars::InfoBar> CreateInfoBar( On 2016/09/21 07:09:33, lshang wrote: > On 2016/09/08 06:24:08, Peter Kasting wrote: > > On 2016/09/08 05:19:57, tsergeant wrote: > > > On 2016/09/08 04:06:24, Peter Kasting wrote: > > > > 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. > > > > > > This infobar is Android-only. > > > > 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. > > > > 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. Yes, adding _android on the filename is fine. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:41: if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) { Nit: {} not necessary https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:43: } else if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) { Nit: No else after return https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:100: return ConfirmInfoBarDelegate::GetButtons(); Nit: I still think it's strange for these two methods to fall back to the base class in opposite cases. tsergeant's comment about "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" makes sense to me. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:107: } else { Nit: No else after return https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:21: // Configures an InfoBar to display a group of permission requests, each of Nit: Configures an InfoBar to display -> An infobar that displays https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:26: ~GroupedPermissionInfoBarDelegate() override; Nit: May be able to make private as well, depending on how this is owned. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:28: // Implementation is in platform-specific infobar file. Nit: This comment should disappear. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:34: bool ShouldShowPersistenceToggle() const; Nit: This deserves a comment, I think. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:35: bool persist() const { return persist_; } Nit: I suggest grouping all cheap inlined accessors into a block, and then placing all the non-cheap functions below them. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:38: size_t GetPermissionCount() const { return types_.size(); } Nit: Inlined accessors should be named unix_hacker()-style and with no "get". https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:43: base::string16 GetMessageText() const override; Nit: Place overrides either above or below non-overrides, not in the middle. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:49: void ToggleAccept(size_t position, bool new_value); Nit: Why |index| on most APIs, but |position| on this and the next? https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:73: // Implementation is in platform-specific infobar file. 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.
Patchset #4 (id:200001) has been deleted
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Peter! I've addressed all your comments, could you PTALA? > So did "We'll first work on removing MediaStreamInfoBarDelegateAndroid subclass > and then come back to this CL." go by the wayside? Yeah, that is under review in the depending CL (https://codereview.chromium.org/2341953004/) :-) https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:41: if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) { On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: {} not necessary Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:43: } else if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) { On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: No else after return Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:100: return ConfirmInfoBarDelegate::GetButtons(); On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: I still think it's strange for these two methods to fall back to the base > class in opposite cases. > > tsergeant's comment about "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" makes sense to me. Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:107: } else { On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: No else after return Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:21: // Configures an InfoBar to display a group of permission requests, each of On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: Configures an InfoBar to display -> An infobar that displays Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:26: ~GroupedPermissionInfoBarDelegate() override; On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: May be able to make private as well, depending on how this is owned. This can't be private, since we have grouped permission infobar. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:28: // Implementation is in platform-specific infobar file. On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: This comment should disappear. Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:34: bool ShouldShowPersistenceToggle() const; On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: This deserves a comment, I think. > Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:35: bool persist() const { return persist_; } On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: I suggest grouping all cheap inlined accessors into a block, and then > placing all the non-cheap functions below them. Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:38: size_t GetPermissionCount() const { return types_.size(); } On 2016/09/21 18:44:40, Peter Kasting wrote: > Nit: Inlined accessors should be named unix_hacker()-style and with no "get". Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:43: base::string16 GetMessageText() const override; On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: Place overrides either above or below non-overrides, not in the middle. Done. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:49: void ToggleAccept(size_t position, bool new_value); On 2016/09/21 18:44:39, Peter Kasting wrote: > Nit: Why |index| on most APIs, but |position| on this and the next? Done. Changed them all to |position|. https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:73: // Implementation is in platform-specific infobar file. On 2016/09/21 18:44:39, Peter Kasting wrote: > 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. Done. Also referred to other infobars, Removed this abstraction and create the infobar directly in delegate's Create() method.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:26: ~GroupedPermissionInfoBarDelegate() override; On 2016/09/22 02:11:31, lshang wrote: > On 2016/09/21 18:44:39, Peter Kasting wrote: > > Nit: May be able to make private as well, depending on how this is owned. > > This can't be private, since we have grouped permission infobar. 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? https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:103: return BUTTON_OK | BUTTON_CANCEL; Nit: Simpler: // Comment explaining why we want to do different things in these cases. return (permission_count() > 1) ? BUTTON_OK : (BUTTON_OK | BUTTON_CANCEL); https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:108: if (permission_count() > 1) Nit: {} required https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:18: // which can be allowed or blocked independently. Nit: Wrap at 80 columns https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:40: // Message text to display for an individual permission at |position|. 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|." https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:44: void ToggleAccept(size_t position, bool new_value); Nit: The function name here says "accept" and the comment says "allow/block". Use consistent language everywhere. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:66: bool persist_; 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? https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:33: } Nit: Longer-term consideration: Is this the only place we call ToggleAccept() on this infobar? It seems like the desired API is really that it sets all the permissions' state at once, so maybe one call that takes n booleans (in an array or vector or something) would be better than n one-arg calls. I don't know JNI so I don't know precisely how to do this efficiently. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:71: } Nit: Similarly, if this is the only place we call these getters, maybe a single getter that takes three vectors as outparams and fills all of them would make more sense.
https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:26: ~GroupedPermissionInfoBarDelegate() override; On 2016/09/23 00:42:05, Peter Kasting wrote: > On 2016/09/22 02:11:31, lshang wrote: > > On 2016/09/21 18:44:39, Peter Kasting wrote: > > > Nit: May be able to make private as well, depending on how this is owned. > > > > This can't be private, since we have grouped permission infobar. > > 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? 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. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:103: return BUTTON_OK | BUTTON_CANCEL; On 2016/09/23 00:42:05, Peter Kasting wrote: > Nit: Simpler: > > // Comment explaining why we want to do different things in these cases. > return (permission_count() > 1) ? BUTTON_OK : (BUTTON_OK | BUTTON_CANCEL); Done. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:108: if (permission_count() > 1) On 2016/09/23 00:42:05, Peter Kasting wrote: > Nit: {} required Done. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:18: // which can be allowed or blocked independently. On 2016/09/23 00:42:06, Peter Kasting wrote: > Nit: Wrap at 80 columns Done. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:40: // Message text to display for an individual permission at |position|. On 2016/09/23 00:42:06, Peter Kasting wrote: > 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|." Done. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:44: void ToggleAccept(size_t position, bool new_value); On 2016/09/23 00:42:05, Peter Kasting wrote: > Nit: The function name here says "accept" and the comment says "allow/block". > Use consistent language everywhere. Done. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:66: bool persist_; On 2016/09/23 00:42:06, Peter Kasting wrote: > 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? Done. Added a comment for it. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:33: } On 2016/09/23 00:42:06, Peter Kasting wrote: > Nit: Longer-term consideration: Is this the only place we call ToggleAccept() on > this infobar? It seems like the desired API is really that it sets all the > permissions' state at once, so maybe one call that takes n booleans (in an array > or vector or something) would be better than n one-arg calls. > > I don't know JNI so I don't know precisely how to do this efficiently. This is for future work. We're planning to reuse PermissionPrompt and PermissionRequestManager on Android to improve feature parity between the two platforms. So in that case, GroupedPermissionInfoBarDelegate will be used by PermissionPrompt, and PermissionPrompt::Delegate expects to be called one-at-a-time(https://cs.chromium.org/chromium/src/chrome/browser/ui/website_settings/permission_prompt.h?sq=package:chromium&l=33):-) https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:71: } On 2016/09/23 00:42:06, Peter Kasting wrote: > Nit: Similarly, if this is the only place we call these getters, maybe a single > getter that takes three vectors as outparams and fills all of them would make > more sense. Probably will also need to get info at specific position by PermissionPrompt, as above, in the following work. But I'm not 100% sure about this at the moment. This is now also in consistent style with ToggleAccept(). I could come back to address this when we've implemented PermissionPromptAndroid and find that these one-at-a-time getters are unnessasary indeed:-)
https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:26: ~GroupedPermissionInfoBarDelegate() override; On 2016/09/27 01:50:42, lshang wrote: > On 2016/09/23 00:42:05, Peter Kasting wrote: > > On 2016/09/22 02:11:31, lshang wrote: > > > On 2016/09/21 18:44:39, Peter Kasting wrote: > > > > Nit: May be able to make private as well, depending on how this is owned. > > > > > > This can't be private, since we have grouped permission infobar. > > > > 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? > > 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. 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>. https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right): https://codereview.chromium.org/2319443002/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:71: } On 2016/09/27 01:50:43, lshang wrote: > On 2016/09/23 00:42:06, Peter Kasting wrote: > > Nit: Similarly, if this is the only place we call these getters, maybe a > single > > getter that takes three vectors as outparams and fills all of them would make > > more sense. > > Probably will also need to get info at specific position by PermissionPrompt, as > above, in the following work. But I'm not 100% sure about this at the moment. > This is now also in consistent style with ToggleAccept(). > > I could come back to address this when we've implemented PermissionPromptAndroid > and find that these one-at-a-time getters are unnessasary indeed:-) Seems reasonable to me to reevaluate then.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Peter! raymes@, PTAL thanks! https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2319443002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:26: ~GroupedPermissionInfoBarDelegate() override; On 2016/09/27 02:05:42, Peter Kasting wrote: > On 2016/09/27 01:50:42, lshang wrote: > > On 2016/09/23 00:42:05, Peter Kasting wrote: > > > On 2016/09/22 02:11:31, lshang wrote: > > > > On 2016/09/21 18:44:39, Peter Kasting wrote: > > > > > Nit: May be able to make private as well, depending on how this is > owned. > > > > > > > > This can't be private, since we have grouped permission infobar. > > > > > > 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? > > > > 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. > > 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>. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, jwd@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2319443002/#ps260001 (title: "minor change in comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138 ========== to ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138 ========== to ========== Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate This will also allow GroupedPermissionInfoBarDelegate to be instantiated to manage other permission types. BUG=606138 Committed: https://crrev.com/3b4d59bb5648b72721014642f3a72685547dd589 Cr-Commit-Position: refs/heads/master@{#421758} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3b4d59bb5648b72721014642f3a72685547dd589 Cr-Commit-Position: refs/heads/master@{#421758} |
