Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522373002:
Create/Destroy PermissionPromptAndroid when switching tabs on Android (Closed)
DescriptionCreate/Destroy PermissionPromptAndroid when switching tabs on Android
This is a follow-up CL of https://codereview.chromium.org/2315563002/.
On desktop, when switching tabs, BrowserView::OnActiveTabChanged() gets called
so that PermissionRequestManager will hide bubbles for the old tab, create
PermissionPrompt for the new tab, and displays any pending requests in the new
tab. This CL does the same thing for Android.
The final goal of this project is to reuse PermissionRequestManager to manage
permission request queue on Android, and get rid of PermissionQueueController.
BUG=606138
Committed: https://crrev.com/7b355c64e66c1799c83e40a85571856193a442db
Cr-Commit-Position: refs/heads/master@{#435725}
Patch Set 1 #
Total comments: 5
Patch Set 2 : minor change #Patch Set 3 : leave out PRM check #Patch Set 4 : add comment #
Total comments: 2
Patch Set 5 : minor change in comment #
Messages
Total messages: 44 (31 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Create PermissionPromptAndroid when switch tabs on Android BUG= ========== to ========== Create/Destroy PermissionPromptAndroid when switching tabs on Android This is a follow-up CL of https://codereview.chromium.org/2315563002/. On desktop, when switching tabs, BrowserView::OnActiveTabChanged() gets called so that PermissionRequestManager will hide bubbles for the old tab, create PermissionPrompt for the new tab, and displays any pending requests in the new tab. This CL does the same thing for Android. The final goal of this project is to reuse PermissionRequestManager to manage permission request queue on Android, and get rid of PermissionQueueController. BUG=606138 ==========
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
lshang@chromium.org changed reviewers: + bauerb@chromium.org, raymes@chromium.org
Bernhard, Raymes, could you take a look at this CL first? Thanks!
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_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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
LGTM I'm not super happy that the TabModelSelector now has to know about permissions, but it does match what we do on other platforms :-/ https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/tab_model/tab_model_selector_base.cc (right): https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:22: if (PermissionRequestManager::IsEnabled()) { Return early if it's disabled? https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:30: PermissionRequestManager::FromWebContents(old_contents)->HideBubble(); When is the PermissionRequestManager null?
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 Bernhard! https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/tab_model/tab_model_selector_base.cc (right): https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:22: if (PermissionRequestManager::IsEnabled()) { On 2016/11/24 15:30:40, Bernhard Bauer wrote: > Return early if it's disabled? Done. https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:30: PermissionRequestManager::FromWebContents(old_contents)->HideBubble(); On 2016/11/24 15:30:40, Bernhard Bauer wrote: > When is the PermissionRequestManager null? I guess https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... checks it for some reason (or just avoid crashes in case). I can also find some evidence in https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_co... that PermissionRequestManager can be null in some edge cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/tab_model/tab_model_selector_base.cc (right): https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:30: PermissionRequestManager::FromWebContents(old_contents)->HideBubble(); On 2016/11/25 00:41:06, lshang wrote: > On 2016/11/24 15:30:40, Bernhard Bauer wrote: > > When is the PermissionRequestManager null? > > I guess > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > checks it for some reason (or just avoid crashes in case). > > I can also find some evidence in > https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_co... > that PermissionRequestManager can be null in some edge cases. Please don't try to avoid crashes "just in case"; that just leads to cargo-culting. If you think it genuinely can't be null, just leave out the check -- if the assumption turns out to be wrong, you'll get crash reports. OTOH, if you have the null-check in here in an attempt to be safe but it is superfluous, you'll never learn about it. In this particular case, PermissionRequestManager is WebContentsUserData, and it is created in TabHelpers::AttachTabHelpers(). It is true that there might be WebContents that don't have these tab helpers attached (anything that is not shown in a tab, for example), but given that this code path is called from the Java TabModel, it seems unlikely that that would apply here.
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.
This looks good in the sense that it matches the desktop codepath but, I'm curious how the current permission infobar codepaths hide/show infobars when switching tabs. If that logic is built into the infobar system we might want to be careful that the two hide/show codepaths work well together and make sense.
On 2016/11/25 09:52:24, Bernhard Bauer wrote: > https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... > File chrome/browser/ui/android/tab_model/tab_model_selector_base.cc (right): > > https://codereview.chromium.org/2522373002/diff/60001/chrome/browser/ui/andro... > chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:30: > PermissionRequestManager::FromWebContents(old_contents)->HideBubble(); > On 2016/11/25 00:41:06, lshang wrote: > > On 2016/11/24 15:30:40, Bernhard Bauer wrote: > > > When is the PermissionRequestManager null? > > > > I guess > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > > checks it for some reason (or just avoid crashes in case). > > > > I can also find some evidence in > > > https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_co... > > that PermissionRequestManager can be null in some edge cases. > > Please don't try to avoid crashes "just in case"; that just leads to > cargo-culting. If you think it genuinely can't be null, just leave out the check > -- if the assumption turns out to be wrong, you'll get crash reports. OTOH, if > you have the null-check in here in an attempt to be safe but it is superfluous, > you'll never learn about it. > > In this particular case, PermissionRequestManager is WebContentsUserData, and it > is created in TabHelpers::AttachTabHelpers(). It is true that there might be > WebContents that don't have these tab helpers attached (anything that is not > shown in a tab, for example), but given that this code path is called from the > Java TabModel, it seems unlikely that that would apply here. Thanks for the explanation. Yeah you're right. I've removed the null check there.
On 2016/11/28 02:35:16, raymes wrote: > This looks good in the sense that it matches the desktop codepath but, I'm > curious how the current permission infobar codepaths hide/show infobars when > switching tabs. If that logic is built into the infobar system we might want to > be careful that the two hide/show codepaths work well together and make sense. Hi Raymes, as discussed, I added a comment there. PTAL again? Thanks!
lgtm thanks Liu! https://codereview.chromium.org/2522373002/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model_selector_base.cc (right): https://codereview.chromium.org/2522373002/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:25: // Visible permission infobars will get repainted when switching tabs, but we maybe change: repainted -> "hidden and shown when switching tabs by the infobar infrastructure"
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...
lshang@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor@ for OWNER review of chrome/browser/ui/android/tab_model PTAL thanks! https://codereview.chromium.org/2522373002/diff/120001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model_selector_base.cc (right): https://codereview.chromium.org/2522373002/diff/120001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model_selector_base.cc:25: // Visible permission infobars will get repainted when switching tabs, but we On 2016/12/01 00:45:48, raymes wrote: > maybe change: repainted -> "hidden and shown when switching tabs by the infobar > infrastructure" 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 bauerb@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2522373002/#ps140001 (title: "minor change in comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480627823701750,
"parent_rev": "267d46f158881149d7df902ea64806229662f6f9", "commit_rev":
"d914bc94f48072c623c4b7ae40b790aa5922eef0"}
Message was sent while issue was closed.
Description was changed from ========== Create/Destroy PermissionPromptAndroid when switching tabs on Android This is a follow-up CL of https://codereview.chromium.org/2315563002/. On desktop, when switching tabs, BrowserView::OnActiveTabChanged() gets called so that PermissionRequestManager will hide bubbles for the old tab, create PermissionPrompt for the new tab, and displays any pending requests in the new tab. This CL does the same thing for Android. The final goal of this project is to reuse PermissionRequestManager to manage permission request queue on Android, and get rid of PermissionQueueController. BUG=606138 ========== to ========== Create/Destroy PermissionPromptAndroid when switching tabs on Android This is a follow-up CL of https://codereview.chromium.org/2315563002/. On desktop, when switching tabs, BrowserView::OnActiveTabChanged() gets called so that PermissionRequestManager will hide bubbles for the old tab, create PermissionPrompt for the new tab, and displays any pending requests in the new tab. This CL does the same thing for Android. The final goal of this project is to reuse PermissionRequestManager to manage permission request queue on Android, and get rid of PermissionQueueController. BUG=606138 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Create/Destroy PermissionPromptAndroid when switching tabs on Android This is a follow-up CL of https://codereview.chromium.org/2315563002/. On desktop, when switching tabs, BrowserView::OnActiveTabChanged() gets called so that PermissionRequestManager will hide bubbles for the old tab, create PermissionPrompt for the new tab, and displays any pending requests in the new tab. This CL does the same thing for Android. The final goal of this project is to reuse PermissionRequestManager to manage permission request queue on Android, and get rid of PermissionQueueController. BUG=606138 ========== to ========== Create/Destroy PermissionPromptAndroid when switching tabs on Android This is a follow-up CL of https://codereview.chromium.org/2315563002/. On desktop, when switching tabs, BrowserView::OnActiveTabChanged() gets called so that PermissionRequestManager will hide bubbles for the old tab, create PermissionPrompt for the new tab, and displays any pending requests in the new tab. This CL does the same thing for Android. The final goal of this project is to reuse PermissionRequestManager to manage permission request queue on Android, and get rid of PermissionQueueController. BUG=606138 Committed: https://crrev.com/7b355c64e66c1799c83e40a85571856193a442db Cr-Commit-Position: refs/heads/master@{#435725} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7b355c64e66c1799c83e40a85571856193a442db Cr-Commit-Position: refs/heads/master@{#435725} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
