Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2315563002:
Add PermissionPromptAndroid skeleton (Closed)
|
Created:
4 years, 3 months ago by lshang Modified:
4 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, tfarina, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd PermissionPromptAndroid skeleton
Currently desktop and Android use different ways to handle multiple permission
requests. Desktop uses PermissionRequestManager to schedule, group and show
requests, delegating the actual UI to a Views/Cocoa-based permission prompt.
Android, however, uses PermissionQueueController, an entirely different system
sharing no code with desktop, which queues up infobars for individual requests.
We'd like to reuse PermissionRequestManager on Android to improve feature parity
between the two platforms.
On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple
permission requests and show individual toggles(allow/block) for each request.
We also need a subclass of PermissionPrompt which is responsible for showing
GroupedPermissionInfobars, coordinating permission infobars with InfoBarService,
and passing the results back to PermissionRequestManager.
This CL added the skeleton of this class, called PermissionPromptAndroid,
similar to PermissionPromptImpl on desktop. It allows multiple requests to show
and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing,
they will be plugged in SetPermissions() in the following CL.
Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to
get PermissionRequest and PermissionPrompt compiled on Android.
Since this refactoring work is not all ready yet, it's behind a runtime switch
flag for now so that we could implement and test it step by step. Finally we'll
enable it by default on Android.
BUG=606138
Committed: https://crrev.com/c4089c7219c8b04750a1a72f1ce26e111dbdb909
Cr-Commit-Position: refs/heads/master@{#427617}
Patch Set 1 #Patch Set 2 : remove unused stuff #
Total comments: 18
Patch Set 3 : address review comments #
Total comments: 1
Patch Set 4 : try #Patch Set 5 : rebase #
Total comments: 10
Patch Set 6 : address #
Total comments: 2
Patch Set 7 : add a comment #
Total comments: 4
Patch Set 8 : nit change #Patch Set 9 : rebase #Patch Set 10 : update #
Total comments: 4
Patch Set 11 : minor change #Dependent Patchsets: 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: 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...
Description was changed from ========== Add PermissionPromptAndroid revise include order some more change Merge branch 'master' into add_permission_prompt_android add two new files some change BUG= ========== to ========== Add PermissionPromptAndroid BUG= ==========
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: 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 #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
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...)
Description was changed from ========== Add PermissionPromptAndroid BUG= ========== to ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG= ==========
Description was changed from ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG= ========== to ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG= ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG= ========== to ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 ==========
Patchset #3 (id:120001) has been deleted
Patchset #4 (id:160001) has been deleted
lshang@chromium.org changed reviewers: + raymes@chromium.org
raymes@ PTAL thanks! This CL adds the skeleton of PermissionPromptAndroid, it can shows the prompt but buttons hasn't done anything to permissions yet. I made it behind a flag so that we could gradually finish all the refactoring, rather than get all things ready in a single CL. Let me know if you have any questions, and I'm also happy to chat in person:-)
Thanks Liu, this look cool :) It would be good to try to get Tim to take a look if possible (I know he's overseas though and it might be hard). https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:52: bool Cancel() override; Is it worth leaving these out until the CL when they're used, since they're not doing anything atm? https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:182: // created on Android, so create it here. Can we just move the creation line in https://cs.chromium.org/chromium/src/chrome/browser/ui/tab_helpers.cc to ensure that it gets created on android too? Or will that not work? https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:187: permission_request_manager->DisplayPendingRequests(); How come this is needed? Where will it happen in the desktop version? https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:219: #endif nit: I think this should have: #else NOTREACHED(); #endif https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:20: infobar_delegate_(nullptr), Where does this get set? https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_impl.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_request_impl.cc:59: case content::PermissionType::FLASH: I don't think we need this on android https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_request_manager.cc:358: #if !defined(OS_ANDROID) Do you know why this ifdef is needed? (and the section below). https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_manager.h (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_request_manager.h:87: static bool IsEnabled(); nit: please add a comment about what this does
I found something wrong with the prompt, please hold off review:-) https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:52: bool Cancel() override; On 2016/10/19 00:25:41, raymes wrote: > Is it worth leaving these out until the CL when they're used, since they're not > doing anything atm? Done. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:182: // created on Android, so create it here. On 2016/10/19 00:25:41, raymes wrote: > Can we just move the creation line in > https://cs.chromium.org/chromium/src/chrome/browser/ui/tab_helpers.cc to ensure > that it gets created on android too? Or will that not work? Done. Moved it so that it gets created on Android too. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:187: permission_request_manager->DisplayPendingRequests(); On 2016/10/19 00:25:41, raymes wrote: > How come this is needed? Where will it happen in the desktop version? PermissionRequestManager::DisplayPendingRequests() will create PermissionPrompt(https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_request_manager.cc?l=244), and it only gets called in BrowserView::OnActiveTabChanged()(https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_view.cc?rcl=1476818291&l=849), on desktop. So I added this line here mainly to create PermissionPromptAndroid. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:219: #endif On 2016/10/19 00:25:41, raymes wrote: > nit: I think this should have: > #else > NOTREACHED(); > #endif Done. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:20: infobar_delegate_(nullptr), On 2016/10/19 00:25:41, raymes wrote: > Where does this get set? Done. Added a line to get delegate from infobar_->delegate(). https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_impl.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_request_impl.cc:59: case content::PermissionType::FLASH: On 2016/10/19 00:25:41, raymes wrote: > I don't think we need this on android Done. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_request_manager.cc:358: #if !defined(OS_ANDROID) On 2016/10/19 00:25:41, raymes wrote: > Do you know why this ifdef is needed? (and the section below). Removed. This is not needed, probably some leftover from previous debugging:P https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_manager.h (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_request_manager.h:87: static bool IsEnabled(); On 2016/10/19 00:25:41, raymes wrote: > nit: please add a comment about what this does Done.
Hi Raymes, please review the updated patch again, thanks! I've addressed your comments, and made some changes to the PermissionPromptAndroid. I checked Tim's calendar and he is on vacation this week. I will ask him to take a look at this when he comes back. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:20: infobar_delegate_(nullptr), On 2016/10/19 00:25:41, raymes wrote: > Where does this get set? Should check with infobar_ instead of infobar_delegate_, have corrected this. https://codereview.chromium.org/2315563002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:20: infobar_delegate_(nullptr), On 2016/10/20 05:03:10, lshang wrote: > On 2016/10/19 00:25:41, raymes wrote: > > Where does this get set? > > Done. Added a line to get delegate from infobar_->delegate(). Sorry this is not correct. Please see updated reply above.
Thanks Liu! https://codereview.chromium.org/2315563002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:51: if (infobar_delegate_ && infobar_service_) { Do we actually need to check infobar_delegate_ here? https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:186: permission_request_manager->DisplayPendingRequests(); As discussed, it seems like this might not be the right place for this. It looks like it should be called once each time a tab is displayed, rather than per request. We could remove it for now and address that in a subsequent CL if it makes more sense. https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:39: this, infobar_service_, requests[0]->GetOrigin(), requests); It would be good to get someone with infobar expertise to take a look over this file to see if it seems ok. Specifically I don't know anything about infobar lifetimes :) https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:48: infobar_service_->RemoveInfoBar(infobar_); I think we can access this from the infobar itself. That would remove the need to keep a pointer to infobar_service_ which would simplify this a bit. Instead we would have: if (infobar_) { infobar_->owner()->RemoveInfobar(infobar_); infobar_ = nullptr; } https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:76: return base::WrapUnique(new PermissionPromptAndroid(web_contents)); nit: use MakeUnique instead https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_impl.cc (right): https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_request_impl.cc:11: #include "chrome/grit/theme_resources.h" nit: is this one needed?
Description was changed from ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 ========== to ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid, similar to PermissionPromptImpl on desktop. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 ==========
lshang@chromium.org changed reviewers: + dominickn@chromium.org
+dominickn@ to review this CL from infobar perspective, especially take a look at the new class PermissionPromptAndroid. Thanks! https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:186: permission_request_manager->DisplayPendingRequests(); On 2016/10/24 00:50:15, raymes wrote: > As discussed, it seems like this might not be the right place for this. It looks > like it should be called once each time a tab is displayed, rather than per > request. We could remove it for now and address that in a subsequent CL if it > makes more sense. Done. Removed it for now til we know clearly where to put it in. https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:39: this, infobar_service_, requests[0]->GetOrigin(), requests); On 2016/10/24 00:50:16, raymes wrote: > It would be good to get someone with infobar expertise to take a look over this > file to see if it seems ok. Specifically I don't know anything about infobar > lifetimes :) Will ask dominickn@ to take a look at this:-) https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:48: infobar_service_->RemoveInfoBar(infobar_); On 2016/10/24 00:50:15, raymes wrote: > I think we can access this from the infobar itself. That would remove the need > to keep a pointer to infobar_service_ which would simplify this a bit. Instead > we would have: > > if (infobar_) { > infobar_->owner()->RemoveInfobar(infobar_); > infobar_ = nullptr; > } Done. Removed the infobar_service_ and get it from web contents. https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:76: return base::WrapUnique(new PermissionPromptAndroid(web_contents)); On 2016/10/24 00:50:15, raymes wrote: > nit: use MakeUnique instead Done. https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_request_impl.cc (right): https://codereview.chromium.org/2315563002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_request_impl.cc:11: #include "chrome/grit/theme_resources.h" On 2016/10/24 00:50:16, raymes wrote: > nit: is this one needed? Done.
https://codereview.chromium.org/2315563002/diff/280001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.h (right): https://codereview.chromium.org/2315563002/diff/280001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.h:39: content::WebContents* web_contents_; Holding a bare WebContents is dangerous, since you can get into lifetime problems (I've fixed two use-after-frees due to bare WebContents pointers as members in the last couple of months). If this really needs to have a WebContents, it should be a WebContentsObserver so it can properly know when the contents is destroyed. That being said, I wonder whether it's possible to have some other class handle the actual showing/hiding. PermissionRequestManager is already a WebContentsUserData, so it always has a WebContents around. Could we define a PermissionPromptAndroid::InfoBarManager classes, which PermissionRequestManager implements? And then PermissionPromptAndroid just defers all of the infobar related calls back up? Thoughts? Happy to discuss this more.
On 2016/10/24 23:22:38, dominickn wrote: > https://codereview.chromium.org/2315563002/diff/280001/chrome/browser/permiss... > File chrome/browser/permissions/permission_prompt_android.h (right): > > https://codereview.chromium.org/2315563002/diff/280001/chrome/browser/permiss... > chrome/browser/permissions/permission_prompt_android.h:39: content::WebContents* > web_contents_; > Holding a bare WebContents is dangerous, since you can get into lifetime > problems (I've fixed two use-after-frees due to bare WebContents pointers as > members in the last couple of months). If this really needs to have a > WebContents, it should be a WebContentsObserver so it can properly know when the > contents is destroyed. > > That being said, I wonder whether it's possible to have some other class handle > the actual showing/hiding. PermissionRequestManager is already a > WebContentsUserData, so it always has a WebContents around. Could we define a > PermissionPromptAndroid::InfoBarManager classes, which PermissionRequestManager > implements? And then PermissionPromptAndroid just defers all of the infobar > related calls back up? > > Thoughts? Happy to discuss this more. Good point Dom. Using a delegate feels like it might be overkill in this case though. I think holding raw pointers can be ok as long as the lifetimes are clearly understood. In this case PermissionRequestManager owns the PermissionPromptAndroid, so it should be able to ensure that PermissionPromptAndroid is destroyed before the WebContents? Thoughts?
https://codereview.chromium.org/2315563002/diff/280001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.h (right): https://codereview.chromium.org/2315563002/diff/280001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.h:39: content::WebContents* web_contents_; On 2016/10/24 23:22:38, dominickn wrote: > Holding a bare WebContents is dangerous, since you can get into lifetime > problems (I've fixed two use-after-frees due to bare WebContents pointers as > members in the last couple of months). If this really needs to have a > WebContents, it should be a WebContentsObserver so it can properly know when the > contents is destroyed. PermissionRequestManager is a WebContentsObserver, so if WebContents gets destroyed, it would not be able to call PermissionPrompt::Show()/Hide(). But I definitely agree with you that we shouldn't hold a bare WebContents here. As I said, it's only used to get the InfoBarService, so what if we dynamically get current WebContents from PermissionRequestID (like in PermissionQueueController: https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_qu... That needs plumbing through PermissionRequestID in PermissionRequestManager and PermissionPrompt. Another approach is to pass through WebContents* in PermissionPrompt::Show()/Hide(), cause we could get web_contents() in PermissionRequestManager. This seems simpler. Are you OK with any of these approaches? > > That being said, I wonder whether it's possible to have some other class handle > the actual showing/hiding. PermissionRequestManager is already a > WebContentsUserData, so it always has a WebContents around. Could we define a > PermissionPromptAndroid::InfoBarManager classes, which PermissionRequestManager > implements? And then PermissionPromptAndroid just defers all of the infobar > related calls back up? > > Thoughts? Happy to discuss this more. I'm not sure if I get you, what will the PermissionPromptAndroid::InfoBarManager used for? Currently for PermissionPromptImpl, it has a PermissionsBubbleDialogDelegateView* bubble_delegate_, which sounds like a delegate but acturally is the view implementation of permission bubble. When need to show/hide bubbles in PermissionRequestManager, it calls into PermissionPromptImpl and create/close the UI. When buttons on the UI gets clicked, it calls into PermissionPromptImpl first and PermissionPromptAndroid defers it to PermissionRequestManager(PermissionRequestManager is already a subclass of PermissionPrompt::Delegate). Similar to that, we have the GroupedPermissionInfobar as Android permission bubble UI, and add/remove infobars in PermissionPromptAndroid. I also have a subsequent CL which makes ProcessButton() on GroupedPermissionInfoBar to go through PermissionPromptAndroid, and then defer to PermissionRequestManager, which is the same as on desktop.
Hey Dom, I've updated the patch. As we discussed, I added a comment saying it's safe to keep the WebContents pointer. PTALA thanks!(●'◡'●)
lgtm % nits https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:184: std::unique_ptr<PermissionRequest> request_ptr(new PermissionRequestImpl( Nit: #include "base/memory/ptr_util.h", and use base::MakeUnique<PermissionRequestImpl>(requesting_origin, permission_type_, profile_, user_gesture, base::Bind(), base::Bind()); https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.h (right): https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.h:44: infobars::InfoBar* infobar_; Nit: add another comment here to say // |infobar_| is owned by the InfoBarService; we keep a pointer here so we can ask the service to remove the infobar after it is added. // |delegate_| is the PermissionRequestManager, which owns this object.
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
lshang@chromium.org changed reviewers: + avi@chromium.org
Raymes, PTALA thanks! also +avi@ for owner review of chrome/browser/ui/tab_helpers.cc, the change is to move PermissionRequestManager::CreateForWebContents() to common section since we're reusing PermissionRequestManager on Android. PTAL thanks! https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:184: std::unique_ptr<PermissionRequest> request_ptr(new PermissionRequestImpl( On 2016/10/26 02:51:16, dominickn wrote: > Nit: #include "base/memory/ptr_util.h", and use > > base::MakeUnique<PermissionRequestImpl>(requesting_origin, permission_type_, > profile_, user_gesture, base::Bind(), base::Bind()); Done. https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.h (right): https://codereview.chromium.org/2315563002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.h:44: infobars::InfoBar* infobar_; On 2016/10/26 02:51:16, dominickn wrote: > Nit: add another comment here to say > > // |infobar_| is owned by the InfoBarService; we keep a pointer here so we can > ask the service to remove the infobar after it is added. > > // |delegate_| is the PermissionRequestManager, which owns this object. Done.
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #2 (id:100001) has been deleted
tab helpers lgtm
lgtm https://codereview.chromium.org/2315563002/diff/360001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/360001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:49: infobar_ = nullptr; Should we clear the pointer here even if there is no infobar_service? (I think we can clear it unconditionally) https://codereview.chromium.org/2315563002/diff/360001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:67: if (infobar_) nit: we can probably remove this
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, avi@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2315563002/#ps380001 (title: "minor change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks all! https://codereview.chromium.org/2315563002/diff/360001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2315563002/diff/360001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:49: infobar_ = nullptr; On 2016/10/26 05:37:36, raymes wrote: > Should we clear the pointer here even if there is no infobar_service? (I think > we can clear it unconditionally) Done. Yeah we should clear it if infobar_service is null. https://codereview.chromium.org/2315563002/diff/360001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_android.cc:67: if (infobar_) On 2016/10/26 05:37:36, raymes wrote: > nit: we can probably remove this Done.
Message was sent while issue was closed.
Description was changed from ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid, similar to PermissionPromptImpl on desktop. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 ========== to ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid, similar to PermissionPromptImpl on desktop. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid, similar to PermissionPromptImpl on desktop. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 ========== to ========== Add PermissionPromptAndroid skeleton Currently desktop and Android use different ways to handle multiple permission requests. Desktop uses PermissionRequestManager to schedule, group and show requests, delegating the actual UI to a Views/Cocoa-based permission prompt. Android, however, uses PermissionQueueController, an entirely different system sharing no code with desktop, which queues up infobars for individual requests. We'd like to reuse PermissionRequestManager on Android to improve feature parity between the two platforms. On Android, GroupedPermissionInfobarDelegate is ready to use to show multiple permission requests and show individual toggles(allow/block) for each request. We also need a subclass of PermissionPrompt which is responsible for showing GroupedPermissionInfobars, coordinating permission infobars with InfoBarService, and passing the results back to PermissionRequestManager. This CL added the skeleton of this class, called PermissionPromptAndroid, similar to PermissionPromptImpl on desktop. It allows multiple requests to show and grouped in one infobar. Currently 'OK' and 'Cancel' button does nothing, they will be plugged in SetPermissions() in the following CL. Also to reuse PermissionRequestManager on Android, BUILD.gn files are changed to get PermissionRequest and PermissionPrompt compiled on Android. Since this refactoring work is not all ready yet, it's behind a runtime switch flag for now so that we could implement and test it step by step. Finally we'll enable it by default on Android. BUG=606138 Committed: https://crrev.com/c4089c7219c8b04750a1a72f1ce26e111dbdb909 Cr-Commit-Position: refs/heads/master@{#427617} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c4089c7219c8b04750a1a72f1ce26e111dbdb909 Cr-Commit-Position: refs/heads/master@{#427617} |
