Remove RenderFrameHost pointer from ChooserController.

Remove RenderFrameHost pointer from ChooserController.

This change removes the raw RenderFrameHost pointer from
ChooserController and instead either computes necessary values (such as
the title) at construction time or stores a weak pointer to the object
derived from the RenderFrameHost.

BUG=697486
Review-Url: https://codereview.chromium.org/2746313002
Cr-Commit-Position: refs/heads/master@{#456876}
Committed: https://chromium.googlesource.com/chromium/src/+/f50d8d33c5da873242189cf73176d78a48549762

[Reilly Grant (use Gerrit), 2017-03-13 23:19:14]

reillyg@chromium.org changed reviewers:
+ juncai@chromium.org, msw@chromium.org

[Reilly Grant (use Gerrit), 2017-03-13 23:19:15]

msw@, please review */views/*
juncai@, please review everything else.

[Reilly Grant (use Gerrit), 2017-03-13 23:20:32]

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

[commit-bot: I haz the power, 2017-03-13 23:21:43]

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, 2017-03-13 23:43:40]

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

[commit-bot: I haz the power, 2017-03-13 23:43:41]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2017-03-13 23:48:51]

This does seem much simpler than the prior CL; thanks!

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/chooser_controller.cc (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.cc:39: title_ =
l10n_util::GetStringFUTF16(
We should also assign this value to |title_| if the above block early-outs
because it can't find the extension or registry. Optionally move this code to a
file-local anon namespace function, it'll only need a RenderFrameHost* arg.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_context.h (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_context.h:52: base::WeakPtr<UsbChooserContext>
AsWeakPtr();
optional nit: rename GetWeakPtr()?

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.cc (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.cc:135: bool
UsbChooserController::IsPaired(size_t index) const {
This isn't really a blocker, but it would be nice to avoid duplication between
this and WebUSBPermissionProvider::HasDevicePermission. (Is it actually safe for
WebUSBPermissionProvider to cache |render_frame_host_|?) Perhaps the two
functions could be combined at take a UsbDevice and [UsbChooserContext +
requesting_origin + embedding_origin] (or WebContents if this class could safely
cache a WebContents pointers).

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.cc:158: return
device::FindInWebUsbAllowedOrigins(device->webusb_allowed_origins(),
aside: too bad FindInWebUsbAllowedOrigins and FindOriginInDescriptorSet are near
duplicates, more code that would be nice to consolidate.

[juncai, 2017-03-14 00:10:49]

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.cc (left):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.cc:91: if
(!usb_service_observer_.IsObserving(usb_service))
question: this if condition is removed, any reason?
https://cs.chromium.org/chromium/src/base/scoped_observer.h?l=58
It uses a vector so when an observer can be added more than once.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.h (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.h:69: bool embedded_frame_;
nit: the variable name |embedded_frame_| doesn't sound like a bool, maybe change
it to: |is_embedded_frame_|?

[Reilly Grant (use Gerrit), 2017-03-14 20:43:10]

Address comments.

[Reilly Grant (use Gerrit), 2017-03-14 20:43:58]

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

[Reilly Grant (use Gerrit), 2017-03-14 20:44:11]

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/chooser_controller.cc (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.cc:39: title_ =
l10n_util::GetStringFUTF16(
On 2017/03/13 23:48:51, msw wrote:
> We should also assign this value to |title_| if the above block early-outs
> because it can't find the extension or registry. Optionally move this code to
a
> file-local anon namespace function, it'll only need a RenderFrameHost* arg.

Done.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_context.h (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_context.h:52: base::WeakPtr<UsbChooserContext>
AsWeakPtr();
On 2017/03/13 23:48:51, msw wrote:
> optional nit: rename GetWeakPtr()?

base::SupportsWeakPointer uses the name AsWeakPtr() which I think makes sense
for an object that vends weak pointers to itself.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.cc (left):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.cc:91: if
(!usb_service_observer_.IsObserving(usb_service))
On 2017/03/14 00:10:48, juncai wrote:
> question: this if condition is removed, any reason?
> https://cs.chromium.org/chromium/src/base/scoped_observer.h?l=58
> It uses a vector so when an observer can be added more than once. 

usb_service_observer_.Add is only called here in the constructor so it can't be
added more than once.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.cc (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.cc:135: bool
UsbChooserController::IsPaired(size_t index) const {
On 2017/03/13 23:48:51, msw wrote:
> This isn't really a blocker, but it would be nice to avoid duplication between
> this and WebUSBPermissionProvider::HasDevicePermission. (Is it actually safe
for
> WebUSBPermissionProvider to cache |render_frame_host_|?) Perhaps the two
> functions could be combined at take a UsbDevice and [UsbChooserContext +
> requesting_origin + embedding_origin] (or WebContents if this class could
safely
> cache a WebContents pointers).

Done. It is safe for WebUSBPermissionProvider to cache |render_frame_host_|
because it is destroyed by UsbTabHelper when the RenderFrameHost is destroyed.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.cc:158: return
device::FindInWebUsbAllowedOrigins(device->webusb_allowed_origins(),
On 2017/03/13 23:48:51, msw wrote:
> aside: too bad FindInWebUsbAllowedOrigins and FindOriginInDescriptorSet are
near
> duplicates, more code that would be nice to consolidate.

Done.

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.h (right):

https://codereview.chromium.org/2746313002/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.h:69: bool embedded_frame_;
On 2017/03/14 00:10:48, juncai wrote:
> nit: the variable name |embedded_frame_| doesn't sound like a bool, maybe
change
> it to: |is_embedded_frame_|?

Done.

[commit-bot: I haz the power, 2017-03-14 20:44:29]

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

[juncai, 2017-03-14 20:57:22]

https://codereview.chromium.org/2746313002/diff/20001/chrome/browser/chooser_...
File chrome/browser/chooser_controller/chooser_controller.cc (left):

https://codereview.chromium.org/2746313002/diff/20001/chrome/browser/chooser_...
chrome/browser/chooser_controller/chooser_controller.cc:27: if (!owning_frame_)
This if condition is not checked in the new code, I think this will break the
test since the |render_frame_host| is nullptr at:
https://codereview.chromium.org/2746313002/diff/20001/chrome/browser/chooser_...

[commit-bot: I haz the power, 2017-03-14 21:02:02]

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

[commit-bot: I haz the power, 2017-03-14 21:02:03]

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

[Reilly Grant (use Gerrit), 2017-03-14 21:20:18]

Fix Android build and juncai@ comment.

[Reilly Grant (use Gerrit), 2017-03-14 21:20:29]

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

[Reilly Grant (use Gerrit), 2017-03-14 21:21:01]

https://codereview.chromium.org/2746313002/diff/20001/chrome/browser/chooser_...
File chrome/browser/chooser_controller/chooser_controller.cc (left):

https://codereview.chromium.org/2746313002/diff/20001/chrome/browser/chooser_...
chrome/browser/chooser_controller/chooser_controller.cc:27: if (!owning_frame_)
On 2017/03/14 20:57:22, juncai wrote:
> This if condition is not checked in the new code, I think this will break the
> test since the |render_frame_host| is nullptr at:
>
https://codereview.chromium.org/2746313002/diff/20001/chrome/browser/chooser_...

Fixed.

[commit-bot: I haz the power, 2017-03-14 21:21:30]

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

[msw, 2017-03-14 21:23:46]

nice, lgtm

[juncai, 2017-03-14 21:42:52]

LGTM.

[Reilly Grant (use Gerrit), 2017-03-14 21:55:02]

The CQ bit was unchecked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-03-14 21:55:06]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-03-14 21:56:00]

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, 2017-03-14 22:55:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489528506696820,
"parent_rev": "0fb5e64ccc4f90b29ee98b88760ba59cce03161d", "commit_rev":
"f50d8d33c5da873242189cf73176d78a48549762"}

[commit-bot: I haz the power, 2017-03-14 22:55:52]

Description was changed from

==========
Remove RenderFrameHost pointer from ChooserController.

This change removes the raw RenderFrameHost pointer from
ChooserController and instead either computes necessary values (such as
the title) at construction time or stores a weak pointer to the object
derived from the RenderFrameHost.

BUG=697486
==========

to

==========
Remove RenderFrameHost pointer from ChooserController.

This change removes the raw RenderFrameHost pointer from
ChooserController and instead either computes necessary values (such as
the title) at construction time or stores a weak pointer to the object
derived from the RenderFrameHost.

BUG=697486

Review-Url: https://codereview.chromium.org/2746313002
Cr-Commit-Position: refs/heads/master@{#456876}
Committed:
https://chromium.googlesource.com/chromium/src/+/f50d8d33c5da873242189cf73176...
==========

[commit-bot: I haz the power, 2017-03-14 22:55:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f50d8d33c5da873242189cf73176...