Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2755593002: Remove pointer from PermissionPromptAndroid to InfoBar (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 1 week ago by Timothy Loh
Modified:
6 months, 1 week ago
Reviewers:
dominickn, tapted, benwells
CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove pointer from PermissionPromptAndroid to InfoBar This patch removes the pointer from PermissionPromptAndroid to InfoBar, as preparation for supporting modal permission dialogs on android via the PermissionRequestManager. Currently, the pointer is only used for Hide() and IsVisible(). We can use the PermissionRequest vector to see if a prompt is being shown, and the calls to Hide() can no-op as they are already handled by the InfoBarManager (e.g. page navigation, or after the user resolves the prompt). If the PermissionServiceImpl receives a mojo connection error, the PermissionRequestManager just dummies out its callback for active prompts (on Android) as we return false in CanAcceptRequestUpdate(). We override EqualsDelegate() as the PermissionRequestManager will try to Show() a queued bubble when it's called back after the user resolves a prompt, but the callback happens before the active prompt is closed. The parent's EqualsDelegate() would consider the new delegate the same as the current one, and so it wouldn't show a new infobar. This means we no longer have the following cycle of pointers: PermissionPromptAndroid -> GroupedPermissionInfoBar -> GroupedPermissionInfoBarDelegate -> PermissionPromptAndroid When we later add modal permission dialogs, the PermissionPromptAndroid will just need to check when it creates the actual UI what object to make, but it won't need extra logic in Hide() or IsVisble(). BUG=606138 Review-Url: https://codereview.chromium.org/2755593002 Cr-Commit-Position: refs/heads/master@{#463551} Committed: https://chromium.googlesource.com/chromium/src/+/897fa146046239d6384aab548a93045f12bd5711

Patch Set 1 #

Patch Set 2 : rebase over 2757483002 #

Total comments: 2

Patch Set 3 : rm PermissionPromptDestroyed() #

Patch Set 4 : rm nl #

Total comments: 5

Patch Set 5 : rm unused includes #

Total comments: 2

Patch Set 6 : hidesautomatically #

Patch Set 7 : rebase and stuff #

Patch Set 8 : upd comment #

Patch Set 9 : update comment more #

Total comments: 6

Patch Set 10 : fix comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -43 lines) Patch
M chrome/browser/permissions/grouped_permission_infobar_delegate_android.h View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc View 1 2 3 4 5 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/permissions/permission_prompt_android.h View 1 2 3 4 5 3 chunks +1 line, -7 lines 0 comments Download
M chrome/browser/permissions/permission_prompt_android.cc View 1 2 3 4 5 6 7 8 2 chunks +12 lines, -21 lines 0 comments Download
M chrome/browser/permissions/permission_request_manager.cc View 1 2 3 4 5 2 chunks +4 lines, -9 lines 0 comments Download
M chrome/browser/ui/cocoa/permission_bubble/permission_bubble_cocoa.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/permission_bubble/permission_bubble_cocoa.mm View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/permission_bubble/mock_permission_prompt.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/permission_bubble/mock_permission_prompt.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/permission_bubble/permission_prompt.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/permission_bubble/permission_prompt_impl.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 63 (44 generated)
Timothy Loh
7 months, 1 week ago (2017-03-15 03:38:11 UTC) #5
Timothy Loh
+dominickn This depends on https://codereview.chromium.org/2757483002/ as otherwise I need to introduce an extra bool temporarily ...
7 months, 1 week ago (2017-03-16 00:42:28 UTC) #12
dominickn
Thanks! https://codereview.chromium.org/2755593002/diff/20001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (left): https://codereview.chromium.org/2755593002/diff/20001/chrome/browser/permissions/permission_prompt_android.cc#oldcode26 chrome/browser/permissions/permission_prompt_android.cc:26: infobar_delegate->PermissionPromptDestroyed(); Since this is the only method that ...
7 months, 1 week ago (2017-03-16 03:58:23 UTC) #15
Timothy Loh
https://codereview.chromium.org/2755593002/diff/20001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (left): https://codereview.chromium.org/2755593002/diff/20001/chrome/browser/permissions/permission_prompt_android.cc#oldcode26 chrome/browser/permissions/permission_prompt_android.cc:26: infobar_delegate->PermissionPromptDestroyed(); On 2017/03/16 03:58:23, dominickn wrote: > Since this ...
7 months, 1 week ago (2017-03-17 00:52:47 UTC) #19
dominickn
lgtm % nits https://codereview.chromium.org/2755593002/diff/60001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2755593002/diff/60001/chrome/browser/permissions/permission_prompt_android.cc#newcode8 chrome/browser/permissions/permission_prompt_android.cc:8: #include "chrome/browser/infobars/infobar_service.h" This is now unused. ...
7 months, 1 week ago (2017-03-17 01:45:26 UTC) #21
Timothy Loh
https://codereview.chromium.org/2755593002/diff/60001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2755593002/diff/60001/chrome/browser/permissions/permission_prompt_android.cc#newcode8 chrome/browser/permissions/permission_prompt_android.cc:8: #include "chrome/browser/infobars/infobar_service.h" On 2017/03/17 01:45:25, dominickn wrote: > This ...
7 months, 1 week ago (2017-03-17 02:34:21 UTC) #25
Timothy Loh
benwells@ for OWNERS, thanks!
7 months, 1 week ago (2017-03-17 02:49:28 UTC) #28
benwells
https://codereview.chromium.org/2755593002/diff/80001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2755593002/diff/80001/chrome/browser/permissions/permission_prompt_android.cc#newcode47 chrome/browser/permissions/permission_prompt_android.cc:47: // false, it doesn't call into here and instead ...
7 months ago (2017-03-20 03:00:43 UTC) #31
Timothy Loh
https://codereview.chromium.org/2755593002/diff/80001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2755593002/diff/80001/chrome/browser/permissions/permission_prompt_android.cc#newcode47 chrome/browser/permissions/permission_prompt_android.cc:47: // false, it doesn't call into here and instead ...
7 months ago (2017-03-20 04:00:41 UTC) #32
benwells
On 2017/03/20 04:00:41, Timothy Loh wrote: > https://codereview.chromium.org/2755593002/diff/80001/chrome/browser/permissions/permission_prompt_android.cc > File chrome/browser/permissions/permission_prompt_android.cc (right): > > https://codereview.chromium.org/2755593002/diff/80001/chrome/browser/permissions/permission_prompt_android.cc#newcode47 ...
7 months ago (2017-03-20 04:11:59 UTC) #33
Timothy Loh
On 2017/03/20 04:11:59, benwells wrote: > I'm not suggesting you keep the pointer / make ...
7 months ago (2017-03-20 04:18:02 UTC) #34
Timothy Loh
Status update: I noticed that due to https://codereview.chromium.org/2522373002 the permissions system is still trying to ...
6 months, 3 weeks ago (2017-03-28 00:48:35 UTC) #35
Timothy Loh
Added a HidesAutomatically() as suggested, PTAL. https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/permissions/permission_prompt_android.cc#newcode47 chrome/browser/permissions/permission_prompt_android.cc:47: // CanAcceptRequestUpdate() return ...
6 months, 2 weeks ago (2017-04-10 04:23:48 UTC) #49
benwells
lgtm https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/permissions/permission_prompt_android.cc File chrome/browser/permissions/permission_prompt_android.cc (right): https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/permissions/permission_prompt_android.cc#newcode47 chrome/browser/permissions/permission_prompt_android.cc:47: // CanAcceptRequestUpdate() return true. On 2017/04/10 04:23:48, Timothy ...
6 months, 2 weeks ago (2017-04-10 07:00:58 UTC) #52
Timothy Loh
+tapted for small change in chrome/browser/ui/cocoa/permission_bubble/permission_bubble_cocoa.*
6 months, 1 week ago (2017-04-11 03:34:54 UTC) #54
tapted
lgtm https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/ui/permission_bubble/permission_prompt.h File chrome/browser/ui/permission_bubble/permission_prompt.h (right): https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/ui/permission_bubble/permission_prompt.h#newcode65 chrome/browser/ui/permission_bubble/permission_prompt.h:65: // Returns true the prompt UI will manage ...
6 months, 1 week ago (2017-04-11 05:21:26 UTC) #55
Timothy Loh
https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/ui/permission_bubble/permission_prompt.h File chrome/browser/ui/permission_bubble/permission_prompt.h (right): https://codereview.chromium.org/2755593002/diff/180001/chrome/browser/ui/permission_bubble/permission_prompt.h#newcode65 chrome/browser/ui/permission_bubble/permission_prompt.h:65: // Returns true the prompt UI will manage hiding ...
6 months, 1 week ago (2017-04-11 05:34:53 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2755593002/220001
6 months, 1 week ago (2017-04-11 05:35:28 UTC) #60
commit-bot: I haz the power
6 months, 1 week ago (2017-04-11 06:26:03 UTC) #63
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/897fa146046239d6384aab548a93...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa