Use WebContents instead of Browser to construct PermissionPrompt

Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser
is not for Android, so change to use WebContents for constructor instead.

BUG=606138
Committed: https://crrev.com/1dab5af6dbc7d560d68132861dc3c345aa51fde8
Cr-Commit-Position: refs/heads/master@{#419634}

[lshang, 2016-09-12 12:27:31]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 12:27:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-12 12:39:04]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 12:39:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 12:48:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 12:48:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lshang, 2016-09-12 12:56:15]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 12:56:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 13:36:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 13:36:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lshang, 2016-09-13 03:07:12]

Patchset #4 (id:60001) has been deleted

[lshang, 2016-09-13 03:07:21]

Patchset #4 (id:80001) has been deleted

[lshang, 2016-09-13 07:19:57]

Patchset #9 (id:200001) has been deleted

[lshang, 2016-09-13 10:07:17]

Patchset #10 (id:230001) has been deleted

[lshang, 2016-09-13 10:07:54]

Patchset #9 (id:220001) has been deleted

[lshang, 2016-09-13 11:49:55]

Patchset #11 (id:290001) has been deleted

[lshang, 2016-09-13 11:50:04]

Patchset #10 (id:270001) has been deleted

[lshang, 2016-09-13 11:51:11]

Patchset #9 (id:250001) has been deleted

[lshang, 2016-09-13 11:51:21]

Patchset #4 (id:100001) has been deleted

[lshang, 2016-09-13 11:51:31]

Patchset #4 (id:120001) has been deleted

[lshang, 2016-09-13 11:51:42]

Patchset #4 (id:140001) has been deleted

[lshang, 2016-09-13 11:51:52]

Patchset #4 (id:160001) has been deleted

[lshang, 2016-09-13 11:52:08]

Patchset #2 (id:20001) has been deleted

[lshang, 2016-09-13 12:18:47]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[lshang, 2016-09-13 12:18:56]

Patchset #6 (id:350001) has been deleted

[commit-bot: I haz the power, 2016-09-13 12:19:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 13:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 13:18:59]

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_...)

[lshang, 2016-09-13 23:52:39]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 23:53:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-14 00:38:39]

Patchset #5 (id:330001) has been deleted

[lshang, 2016-09-14 00:38:44]

Patchset #2 (id:40001) has been deleted

[lshang, 2016-09-14 00:38:56]

Patchset #5 (id:390001) has been deleted

[lshang, 2016-09-14 00:39:07]

Patchset #4 (id:370001) has been deleted

[lshang, 2016-09-14 00:39:47]

Patchset #2 (id:180001) has been deleted

[lshang, 2016-09-14 00:39:56]

Patchset #2 (id:310001) has been deleted

[lshang, 2016-09-14 00:45:43]

Description was changed from

==========
Use WebContents instead of Browser to construct PermissionPrompt

Since we are reusing PermissionPrompt on Android, and Browser is not for
Android, so change to use WebContents instead.

BUG=
==========

to

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser is not
for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

[lshang, 2016-09-14 00:45:57]

Description was changed from

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser is not
for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

to

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage 
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser is not 
for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

[lshang, 2016-09-14 00:46:22]

Description was changed from

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage 
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser is not 
for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

to

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage 
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser
is not for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

[lshang, 2016-09-14 00:47:42]

lshang@chromium.org changed reviewers:
+ tsergeant@chromium.org

[lshang, 2016-09-14 00:47:43]

PTAL thanks!

[commit-bot: I haz the power, 2016-09-14 00:50:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 00:50:31]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-09-14 05:35:34]

lgtm, this is exactly what I had in mind. Let's see what owners think.

[lshang, 2016-09-14 05:40:19]

lshang@chromium.org changed reviewers:
+ raymes@chromium.org

[lshang, 2016-09-14 05:40:20]

+raymes@ for desktop permission change.

[raymes, 2016-09-15 02:06:46]

lgtm

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:21: #include
"chrome/browser/ui/browser_finder.h"
nit: can we remove this?

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:8:
#include "chrome/browser/ui/browser.h"
nit: I think we can forward declare this rather than including it?

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/mock_permission_prompt_factory.h
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/mock_permission_prompt_factory.h:28:
std::unique_ptr<PermissionPrompt> Create(content::WebContents* web_contents);
nit: forward declare WebContents

[raymes, 2016-09-15 02:07:32]

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:86:
model->AppendWebContents(web_contents, true);
Please get someone else to take a look at this (tapted?) - it looks right but
I'm not so familiar with this type of code.

[lshang, 2016-09-15 02:44:48]

lshang@chromium.org changed reviewers:
+ tapted@chromium.org

[lshang, 2016-09-15 02:44:49]

+tapted@ for cocoa code change. PTAL thanks!

[tapted, 2016-09-15 03:43:52]

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/permission_prompt_impl_views_mac.mm
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_prompt_impl_views_mac.mm:39:
return base::MakeUnique<PermissionBubbleCocoa>(web_contents);
Can this just be changed to pass
`chrome::FindBrowserWithWebContents(web_contents)`? Seems most of the other
Cocoa changes aren't required then. And since PermissionBubbleCocoa* retains a
weak pointer to Browser* (and it would be invalid to create permission prompts
for WebContents not in a Browser*, such as for packaged apps) this seems like
the more sensible API.

Also if the current UI code doesn't have a dependency on WebContents, and it
doesn't need one, we shouldn't be adding one.

Although I suppose the same could be said for the views-backed
PermissionPromptImpl.. Do the downstream things for android need changes to the
toolkit-views implementation too? (That is, is it enough to just change the two
desktop implementations of PermissionPrompt::Create(..) rather than the
platform-specific prompt implementations themselves, which assume they are on a
Browser*?)

[lshang, 2016-09-15 06:30:11]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 06:30:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 06:43:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 06:43:19]

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_...)

[lshang, 2016-09-15 08:59:45]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 08:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-15 09:27:19]

Patchset #3 (id:430001) has been deleted

[commit-bot: I haz the power, 2016-09-15 09:39:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 09:39:40]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-09-15 09:45:06]

PTALA thanks!

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:21: #include
"chrome/browser/ui/browser_finder.h"
On 2016/09/15 02:06:46, raymes wrote:
> nit: can we remove this?

Done.

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:8:
#include "chrome/browser/ui/browser.h"
On 2016/09/15 02:06:46, raymes wrote:
> nit: I think we can forward declare this rather than including it?

Done.

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:86:
model->AppendWebContents(web_contents, true);
On 2016/09/15 02:07:32, raymes wrote:
> Please get someone else to take a look at this (tapted?) - it looks right but
> I'm not so familiar with this type of code.

NaN. As Trent suggested, we will just change the interface of PermissionPrompt
but not platform-specific implementations, so we'll still use Browser to
construct PermissionPrompt on Mac.

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/permission_prompt_impl_views_mac.mm
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_prompt_impl_views_mac.mm:39:
return base::MakeUnique<PermissionBubbleCocoa>(web_contents);
On 2016/09/15 03:43:52, tapted (catching up) wrote:
> Can this just be changed to pass
> `chrome::FindBrowserWithWebContents(web_contents)`? Seems most of the other
> Cocoa changes aren't required then. And since PermissionBubbleCocoa* retains a
> weak pointer to Browser* (and it would be invalid to create permission prompts
> for WebContents not in a Browser*, such as for packaged apps) this seems like
> the more sensible API.
> 
> Also if the current UI code doesn't have a dependency on WebContents, and it
> doesn't need one, we shouldn't be adding one.
> 
> Although I suppose the same could be said for the views-backed
> PermissionPromptImpl.. Do the downstream things for android need changes to
the
> toolkit-views implementation too? (That is, is it enough to just change the
two
> desktop implementations of PermissionPrompt::Create(..) rather than the
> platform-specific prompt implementations themselves, which assume they are on
a
> Browser*?)

Thanks for your detailed comment Trent! 

I think your suggestion is a good idea, which can make this CL much simpler,
also avoid causing any potential bugs on desktop due to use WebContents instead
of Browser for implementation.

Done.

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/mock_permission_prompt_factory.h
(right):

https://codereview.chromium.org/2335623002/diff/410001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/mock_permission_prompt_factory.h:28:
std::unique_ptr<PermissionPrompt> Create(content::WebContents* web_contents);
On 2016/09/15 02:06:46, raymes wrote:
> nit: forward declare WebContents

Done.

[tapted, 2016-09-15 23:48:13]

lgtm, but make sure raymes is happy too

[lshang, 2016-09-15 23:52:06]

tsergeant@, raymes@: this CL changed a bit after tapted@'s comments, please
confirm the change, thanks!

[tsergeant, 2016-09-16 04:22:26]

👍 still lgtm

[raymes, 2016-09-20 00:44:47]

lgtm

[lshang, 2016-09-20 00:53:11]

The CQ bit was checked by lshang@chromium.org

[commit-bot: I haz the power, 2016-09-20 00:53:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 01:42:31]

Description was changed from

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage 
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser
is not for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

to

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage 
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser
is not for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

[commit-bot: I haz the power, 2016-09-20 01:42:33]

Committed patchset #3 (id:450001)

[commit-bot: I haz the power, 2016-09-20 01:46:24]

Description was changed from

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage 
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser
is not for Android, so change to use WebContents for constructor instead.

BUG=606138
==========

to

==========
Use WebContents instead of Browser to construct PermissionPrompt

Currently we use PermissionRequestManager and PermissionPrompt to manage
permission requests on desktop, and PermissionPrompt is constructed using
Browser.

Since now we are going to reuse PermissionPrompt on Android, and Browser
is not for Android, so change to use WebContents for constructor instead.

BUG=606138

Committed: https://crrev.com/1dab5af6dbc7d560d68132861dc3c345aa51fde8
Cr-Commit-Position: refs/heads/master@{#419634}
==========

[commit-bot: I haz the power, 2016-09-20 01:46:26]

Patchset 3 (id:??) landed as
https://crrev.com/1dab5af6dbc7d560d68132861dc3c345aa51fde8
Cr-Commit-Position: refs/heads/master@{#419634}