Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341953004:
Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate (Closed)
|
Created:
4 years, 3 months ago by lshang Modified:
4 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, dominickn Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDecouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate
Currently MediaStreamIndoBarDelegate is a subclass of
GroupedPermissionInfoBarDelegate(GPIBD), making it harder to implement
PermissionPromptAndroid and get GPIBD ready for other permission types.
So this CL decouples media infobar delegate from GPIBD, and let it inherit
PermissionInfoBarDelegate to keep the persistence toggle.
This is only a temporary state of permission infobar refactoring. After
PermissionPromptAndroid gets done and used for all other permission types,
we'll merge media stream infobar delegate into grouped infobar delegate.
BUG=458606, 606138
Committed: https://crrev.com/c7a0d167bd0ce89c638fcf0a7bf4fb527fb05f9c
Cr-Commit-Position: refs/heads/master@{#420864}
Patch Set 1 #
Total comments: 20
Patch Set 2 : address review comments #
Total comments: 4
Patch Set 3 : nits change #
Total comments: 13
Patch Set 4 : address review comments #
Messages
Total messages: 69 (51 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
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.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate revert some BUG=458606, 606138 ========== to ========== Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate Currently MediaStreamIndoBarDelegate is a subclass of GroupedPermissionInfoBarDelegate(GPIBD), making it harder to implement PermissionPromptAndroid and get GPIBD ready for other permission types. So this CL decouples media infobar delegate from GPIBD, and let it inherit PermissionInfoBarDelegate to keep the persistence toggle. This is only a temporary state of permission infobar refactoring. After PermissionPromptAndroid gets done and used for all other permission types, we'll merge media stream infobar delegate into grouped infobar delegate. BUG=458606, 606138 ==========
lshang@chromium.org changed reviewers: + dominickn@chromium.org, tsergeant@chromium.org
+tsergeant@ for overall review and +dominickn@ for persistence toggle perspective. PTAL thanks!
First pass! https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (left): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:16: #include "components/content_settings/core/common/content_settings_types.h" Nit: I think you still need this header for CONTENT_SETTINGS_TYPE_* https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:16: #include "chrome/browser/profiles/profile.h" If you're not calling methods on Profile, you can just forward declare "class Profile" and not include the header. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:29: Profile* profile, Don't add the Profile* argument here. Instead, call Profile::FromBrowserContext(web_contents->GetBrowserContext()) in the MediaStreamInfoBarDelegateAndroid constructor. That way, you don't need to change the PermissionBubbleMediaAccess file. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h:38: bool persist() const { return persist_; } set_persist() is inherited from the parent class so you don't need it here. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h:60: bool persist_; This is on the parent PermissionInfoBarDelegate class so you don't need it. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.cc (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.cc:40: std::vector<int> PermissionInfoBarDelegate::content_setting() const { Nit: it's strange to have a singular content_setting() name for a method that returns a vector of things. Rename to content_settings()? https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.h (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.h:8: #include <memory> Nit: #include <vector> https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.h:33: virtual bool ShouldShowPersistenceToggle() const; Should this be made virtual, or should you just check the MIC and CAMERA content settings types (like it does for geolocation right now)? That may be cleaner. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/permission_infobar.cc (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/permission_infobar.cc:14: #include "chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h" Nit: is this needed here? https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/permission_infobar.cc:59: std::vector<int> content_settings(delegate->content_setting()); Nit: this change isn't needed? If you revert both of them here you don't need to change this file at all 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...
Patchset #2 (id:160001) has been deleted
Thanks Dom! PTAL again? https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (left): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:16: #include "components/content_settings/core/common/content_settings_types.h" On 2016/09/20 08:01:36, dominickn wrote: > Nit: I think you still need this header for CONTENT_SETTINGS_TYPE_* Done. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:16: #include "chrome/browser/profiles/profile.h" On 2016/09/20 08:01:36, dominickn wrote: > If you're not calling methods on Profile, you can just forward declare "class > Profile" and not include the header. Still need this after we change to get Profile from WebContents:-) https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:29: Profile* profile, On 2016/09/20 08:01:36, dominickn wrote: > Don't add the Profile* argument here. Instead, call > Profile::FromBrowserContext(web_contents->GetBrowserContext()) in the > MediaStreamInfoBarDelegateAndroid constructor. That way, you don't need to > change the PermissionBubbleMediaAccess file. Done. Get Profile from WebContents. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h:38: bool persist() const { return persist_; } On 2016/09/20 08:01:36, dominickn wrote: > set_persist() is inherited from the parent class so you don't need it here. Done. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h:60: bool persist_; On 2016/09/20 08:01:36, dominickn wrote: > This is on the parent PermissionInfoBarDelegate class so you don't need it. persist_ is a private member of parent class so we can't directly access that here. I added the persist() get method in the parent class. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.cc (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.cc:40: std::vector<int> PermissionInfoBarDelegate::content_setting() const { On 2016/09/20 08:01:36, dominickn wrote: > Nit: it's strange to have a singular content_setting() name for a method that > returns a vector of things. Rename to content_settings()? Done. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.h (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.h:8: #include <memory> On 2016/09/20 08:01:36, dominickn wrote: > Nit: #include <vector> Done. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.h:33: virtual bool ShouldShowPersistenceToggle() const; On 2016/09/20 08:01:36, dominickn wrote: > Should this be made virtual, or should you just check the MIC and CAMERA content > settings types (like it does for geolocation right now)? That may be cleaner. Good idea, have merged two media types into ShouldShowPersistenceToggle() of parent class PermissionInfoBarDelegate. Done. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/permission_infobar.cc (right): https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/permission_infobar.cc:14: #include "chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h" On 2016/09/20 08:01:36, dominickn wrote: > Nit: is this needed here? Done. https://codereview.chromium.org/2341953004/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/permission_infobar.cc:59: std::vector<int> content_settings(delegate->content_setting()); On 2016/09/20 08:01:36, dominickn wrote: > Nit: this change isn't needed? If you revert both of them here you don't need to > change this file at all in this CL? Done. Only change the method name here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-OWNER lgtm % nits https://codereview.chromium.org/2341953004/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:40: Profile* profile = Nit: inline this call into the new MediaStreamInfoBarDelegateAndroid call, since you don't use the variable again. https://codereview.chromium.org/2341953004/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:94: base::Bind(&MediaStreamInfoBarDelegateAndroid::DoNothing, Nit: can you declare DoNothing in the anonymous namespace in this file, and not have it be a class method (and hence not need to pass base::Unretained)?
lgtm too
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...
https://codereview.chromium.org/2341953004/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:40: Profile* profile = On 2016/09/21 05:09:16, dominickn wrote: > Nit: inline this call into the new MediaStreamInfoBarDelegateAndroid call, since > you don't use the variable again. Done. https://codereview.chromium.org/2341953004/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:94: base::Bind(&MediaStreamInfoBarDelegateAndroid::DoNothing, On 2016/09/21 05:09:16, dominickn wrote: > Nit: can you declare DoNothing in the anonymous namespace in this file, and not > have it be a class method (and hence not need to pass base::Unretained)? Done.
lshang@chromium.org changed reviewers: + dfalcantara@chromium.org, raymes@chromium.org, sergeyu@chromium.org
dfalcantara@chromium.org: Please review changes in chrome/browser/ui/android/infobars/ raymes@chromium.org: Please review changes in chrome/browser/permissions/ sergeyu@chromium.org: Please review changes in chrome/browser/media/ Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
permissions lgtm
+Dom as FYI because he touched these last.
Didn't realize Dom was already on here. https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:69: return PAGE_ACTION_TYPE; This needs its own ID for tracking in infobar_delegate.h.
https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:69: return PAGE_ACTION_TYPE; On 2016/09/21 20:37:38, dfalcantara (OOO 9.22-9.26) wrote: > This needs its own ID for tracking in infobar_delegate.h. InfoBarDelegate::Type currently only has two values: WARNING_TYPE and PAGE_ACTION_TYPE, we returns its own identifier as above in GetIdentifier(). "its own ID" you mean by?
lgtm https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:69: return PAGE_ACTION_TYPE; On 2016/09/22 00:06:42, lshang wrote: > On 2016/09/21 20:37:38, dfalcantara (OOO 9.22-9.26) wrote: > > This needs its own ID for tracking in infobar_delegate.h. > > InfoBarDelegate::Type currently only has two values: WARNING_TYPE and > PAGE_ACTION_TYPE, we returns its own identifier as above in GetIdentifier(). > "its own ID" you mean by? Whoops, my mistake. Was thinking about the function above.
On 2016/09/22 00:08:41, dfalcantara (OOO 9.22-9.26) wrote: > lgtm > > https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... > File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc > (right): > > https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... > chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:69: return > PAGE_ACTION_TYPE; > On 2016/09/22 00:06:42, lshang wrote: > > On 2016/09/21 20:37:38, dfalcantara (OOO 9.22-9.26) wrote: > > > This needs its own ID for tracking in infobar_delegate.h. > > > > InfoBarDelegate::Type currently only has two values: WARNING_TYPE and > > PAGE_ACTION_TYPE, we returns its own identifier as above in GetIdentifier(). > > "its own ID" you mean by? > > Whoops, my mistake. Was thinking about the function above. Thanks!
lgtm with some style nits. https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:48: new MediaStreamInfoBarDelegateAndroid( Use base::MakeUnique here https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:120: return l10n_util::GetStringUTF16((button == BUTTON_OK) Maybe use switch statement to select the message - that will help to ensure that this code is updated when a new value is added in InfoBarButton https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:140: if (controller_->IsAskingForAudio()) Add {} https://google.github.io/styleguide/cppguide.html#Conditionals https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:143: if (controller_->IsAskingForVideo()) same here and for similar expressions below. https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.cc:41: return std::vector<int>(1, content_settings_type_); return std::vector<int> { content_settings_type_ };
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.
https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:48: new MediaStreamInfoBarDelegateAndroid( On 2016/09/24 00:05:00, Sergey Ulanov wrote: > Use base::MakeUnique here Can't use MakeUnique here because constructor of MediaStreamInfoBarDelegateAndroid is private. Also took a look at other types like GeolocationInfoBarDelegateAndroid, NotificationPermissionInfoBarDelegate, etc., they all use std::unique_ptr<PermissionInfoBarDelegate>(new T(args)) like this:-) https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:120: return l10n_util::GetStringUTF16((button == BUTTON_OK) On 2016/09/24 00:05:00, Sergey Ulanov wrote: > Maybe use switch statement to select the message - that will help to ensure that > this code is updated when a new value is added in InfoBarButton Done. https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:140: if (controller_->IsAskingForAudio()) On 2016/09/24 00:05:00, Sergey Ulanov wrote: > Add {} > https://google.github.io/styleguide/cppguide.html#Conditionals Done. https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc:143: if (controller_->IsAskingForVideo()) On 2016/09/24 00:05:00, Sergey Ulanov wrote: > same here and for similar expressions below. Done. https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.cc (right): https://codereview.chromium.org/2341953004/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.cc:41: return std::vector<int>(1, content_settings_type_); On 2016/09/24 00:05:00, Sergey Ulanov wrote: > return std::vector<int> { content_settings_type_ }; Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, tsergeant@chromium.org, sergeyu@chromium.org, raymes@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2341953004/#ps220001 (title: "address review 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 ========== Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate Currently MediaStreamIndoBarDelegate is a subclass of GroupedPermissionInfoBarDelegate(GPIBD), making it harder to implement PermissionPromptAndroid and get GPIBD ready for other permission types. So this CL decouples media infobar delegate from GPIBD, and let it inherit PermissionInfoBarDelegate to keep the persistence toggle. This is only a temporary state of permission infobar refactoring. After PermissionPromptAndroid gets done and used for all other permission types, we'll merge media stream infobar delegate into grouped infobar delegate. BUG=458606, 606138 ========== to ========== Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate Currently MediaStreamIndoBarDelegate is a subclass of GroupedPermissionInfoBarDelegate(GPIBD), making it harder to implement PermissionPromptAndroid and get GPIBD ready for other permission types. So this CL decouples media infobar delegate from GPIBD, and let it inherit PermissionInfoBarDelegate to keep the persistence toggle. This is only a temporary state of permission infobar refactoring. After PermissionPromptAndroid gets done and used for all other permission types, we'll merge media stream infobar delegate into grouped infobar delegate. BUG=458606, 606138 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate Currently MediaStreamIndoBarDelegate is a subclass of GroupedPermissionInfoBarDelegate(GPIBD), making it harder to implement PermissionPromptAndroid and get GPIBD ready for other permission types. So this CL decouples media infobar delegate from GPIBD, and let it inherit PermissionInfoBarDelegate to keep the persistence toggle. This is only a temporary state of permission infobar refactoring. After PermissionPromptAndroid gets done and used for all other permission types, we'll merge media stream infobar delegate into grouped infobar delegate. BUG=458606, 606138 ========== to ========== Decouple MediaStreamInfoBarDelegate from GroupedPermissionInfoBarDelegate Currently MediaStreamIndoBarDelegate is a subclass of GroupedPermissionInfoBarDelegate(GPIBD), making it harder to implement PermissionPromptAndroid and get GPIBD ready for other permission types. So this CL decouples media infobar delegate from GPIBD, and let it inherit PermissionInfoBarDelegate to keep the persistence toggle. This is only a temporary state of permission infobar refactoring. After PermissionPromptAndroid gets done and used for all other permission types, we'll merge media stream infobar delegate into grouped infobar delegate. BUG=458606, 606138 Committed: https://crrev.com/c7a0d167bd0ce89c638fcf0a7bf4fb527fb05f9c Cr-Commit-Position: refs/heads/master@{#420864} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c7a0d167bd0ce89c638fcf0a7bf4fb527fb05f9c Cr-Commit-Position: refs/heads/master@{#420864} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
