Replace the use of WindowAndroid with Tab in permissions code.

Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
TBR=sergeyu@chromium.org

Committed: https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80
Cr-Commit-Position: refs/heads/master@{#432309}

[dominickn, 2016-11-11 02:22:53]

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

[commit-bot: I haz the power, 2016-11-11 02:23:29]

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

[dominickn, 2016-11-11 02:23:51]

Description was changed from

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
==========

to

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
==========

[dominickn, 2016-11-11 02:23:51]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org, dfalcantara@chromium.org

[dominickn, 2016-11-11 02:24:14]

PTAL, thanks!

[commit-bot: I haz the power, 2016-11-11 02:56:33]

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

[commit-bot: I haz the power, 2016-11-11 02:56:34]

Dry run: This issue passed the CQ dry run.

[gone, 2016-11-11 21:09:00]

lgtm, but you should run it by ted or yusuf

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:123:
.setPositiveButton(
This indentation is a bit nuts... can you pull out the creation of the
listeners, or make each line operate on the builder variable directly?

builder.setView(view);
builder.setPositiveButton(etc)

[dominickn, 2016-11-11 22:49:39]

dominickn@chromium.org changed reviewers:
+ tedchoc@chromium.org

[dominickn, 2016-11-11 22:49:40]

Thanks! I'll fix up the indentation when we've unpacked from moving next week. 

+tedchoc - can you sanity check please?

[benwells, 2016-11-14 03:17:53]

c/b/permissions lgtm

[Ted C, 2016-11-14 17:52:54]

lgtm ... assuming the activity null-ness doesn't spin out of control

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java:23:
protected Tab mTab;
should/can both of these be private?

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:122:
Activity activity =
mDialogDelegate.getTab().getWindowAndroid().getActivity().get();
what is this showDialog request triggered by?  Is it async?  Driven by the
renderer?

The activity in a window android doesn't necessarily need to be non-null is why
I'm asking (mainly for the following assert).  But it would be a race where
you'd have to trigger the action and quickly leave the chrome activity (and get
unlucky that it would be cleanup immediately and now your windowandroid might be
associated with no activity).

[dominickn, 2016-11-14 19:57:53]

Description was changed from

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
==========

to

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
TBR=sergeyu@chromium.org
==========

[dominickn, 2016-11-14 19:58:24]

Thanks!

+cc sergeyu for TBR (trivial inherited method name change in c/b/media/webrtc)

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java:23:
protected Tab mTab;
On 2016/11/14 17:52:54, Ted C wrote:
> should/can both of these be private?

They're needed (accessed and/or set up) by a subclass -
GroupedPermissionInfoBar.

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:123:
.setPositiveButton(
On 2016/11/11 21:09:00, dfalcantara (check my queue) wrote:
> This indentation is a bit nuts... can you pull out the creation of the
> listeners, or make each line operate on the builder variable directly?
> 
> builder.setView(view);
> builder.setPositiveButton(etc)

Done.

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:122:
Activity activity =
mDialogDelegate.getTab().getWindowAndroid().getActivity().get();
On 2016/11/14 17:52:54, Ted C wrote:
> what is this showDialog request triggered by?  Is it async?  Driven by the
> renderer?
> 
> The activity in a window android doesn't necessarily need to be non-null is
why
> I'm asking (mainly for the following assert).  But it would be a race where
> you'd have to trigger the action and quickly leave the chrome activity (and
get
> unlucky that it would be cleanup immediately and now your windowandroid might
be
> associated with no activity).

This is driven by the renderer, specifically when there is a JS call to an API
which requires prompting the user for permission (e.g. navigator.geolocation).

The renderer asks the browser process if the origin is permitted to access the
API, and the browser process will prompt the user if necessary, calling into
showDialog(). Requests for different permissions also call into the controller,
which queues them and shows a new prompt for the next permission when the user
has responded to the first.

I suppose it's possible that the site could make the JS call just as the user
backgrounds the chrome activity (it doesn't necessarily need to be in response
to user input).

I'll change this to just return if the activity is null - that's safest, right?

[dominickn, 2016-11-14 19:58:55]

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

[commit-bot: I haz the power, 2016-11-14 20:01:05]

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-11-14 20:51:02]

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

[commit-bot: I haz the power, 2016-11-14 20:51:03]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-11-15 17:20:49]

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java:23:
protected Tab mTab;
On 2016/11/14 19:58:24, dominickn wrote:
> On 2016/11/14 17:52:54, Ted C wrote:
> > should/can both of these be private?
> 
> They're needed (accessed and/or set up) by a subclass -
> GroupedPermissionInfoBar.

d'oh.  I meant final.

[Ted C, 2016-11-15 17:23:02]

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:122:
Activity activity =
mDialogDelegate.getTab().getWindowAndroid().getActivity().get();
On 2016/11/14 19:58:24, dominickn wrote:
> On 2016/11/14 17:52:54, Ted C wrote:
> > what is this showDialog request triggered by?  Is it async?  Driven by the
> > renderer?
> > 
> > The activity in a window android doesn't necessarily need to be non-null is
> why
> > I'm asking (mainly for the following assert).  But it would be a race where
> > you'd have to trigger the action and quickly leave the chrome activity (and
> get
> > unlucky that it would be cleanup immediately and now your windowandroid
might
> be
> > associated with no activity).
> 
> This is driven by the renderer, specifically when there is a JS call to an API
> which requires prompting the user for permission (e.g. navigator.geolocation).
> 
> The renderer asks the browser process if the origin is permitted to access the
> API, and the browser process will prompt the user if necessary, calling into
> showDialog(). Requests for different permissions also call into the
controller,
> which queues them and shows a new prompt for the next permission when the user
> has responded to the first.
> 
> I suppose it's possible that the site could make the JS call just as the user
> backgrounds the chrome activity (it doesn't necessarily need to be in response
> to user input).
> 
> I'll change this to just return if the activity is null - that's safest,
right?

Yeah, that seems reasonable to me.  You might need to call
mDialogDelegate.onDismiss();
or
mDialogDelegate.destroy();

as well to make sure you are cleaning up your native counterparts.

[dominickn, 2016-11-15 18:42:08]

Thanks!

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java:23:
protected Tab mTab;
On 2016/11/15 17:20:49, Ted C wrote:
> On 2016/11/14 19:58:24, dominickn wrote:
> > On 2016/11/14 17:52:54, Ted C wrote:
> > > should/can both of these be private?
> > 
> > They're needed (accessed and/or set up) by a subclass -
> > GroupedPermissionInfoBar.
> 
> d'oh.  I meant final.

Tab can be final, but mContentSettingsTypes can't be because
GroupedPermissionInfoBar needs to mutate it based on which permissions the user
decided to set.

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2493013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:122:
Activity activity =
mDialogDelegate.getTab().getWindowAndroid().getActivity().get();
On 2016/11/15 17:23:02, Ted C wrote:
> On 2016/11/14 19:58:24, dominickn wrote:
> > On 2016/11/14 17:52:54, Ted C wrote:
> > > what is this showDialog request triggered by?  Is it async?  Driven by the
> > > renderer?
> > > 
> > > The activity in a window android doesn't necessarily need to be non-null
is
> > why
> > > I'm asking (mainly for the following assert).  But it would be a race
where
> > > you'd have to trigger the action and quickly leave the chrome activity
(and
> > get
> > > unlucky that it would be cleanup immediately and now your windowandroid
> might
> > be
> > > associated with no activity).
> > 
> > This is driven by the renderer, specifically when there is a JS call to an
API
> > which requires prompting the user for permission (e.g.
navigator.geolocation).
> > 
> > The renderer asks the browser process if the origin is permitted to access
the
> > API, and the browser process will prompt the user if necessary, calling into
> > showDialog(). Requests for different permissions also call into the
> controller,
> > which queues them and shows a new prompt for the next permission when the
user
> > has responded to the first.
> > 
> > I suppose it's possible that the site could make the JS call just as the
user
> > backgrounds the chrome activity (it doesn't necessarily need to be in
response
> > to user input).
> > 
> > I'll change this to just return if the activity is null - that's safest,
> right?
> 
> Yeah, that seems reasonable to me.  You might need to call
> mDialogDelegate.onDismiss();
> or
> mDialogDelegate.destroy();
> 
> as well to make sure you are cleaning up your native counterparts.

D'oh, good catch thanks!

[dominickn, 2016-11-15 19:34:16]

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

[commit-bot: I haz the power, 2016-11-15 19:35:03]

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-11-15 19:53:46]

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

[commit-bot: I haz the power, 2016-11-15 19:53:47]

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

[dominickn, 2016-11-15 23:27:49]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-11-15 23:27:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, benwells@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2493013002/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2016-11-15 23:28:37]

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-11-16 00:19:21]

Description was changed from

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
TBR=sergeyu@chromium.org
==========

to

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
TBR=sergeyu@chromium.org
==========

[commit-bot: I haz the power, 2016-11-16 00:19:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-16 00:23:02]

Description was changed from

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
TBR=sergeyu@chromium.org
==========

to

==========
Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG=658125
TBR=sergeyu@chromium.org

Committed: https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80
Cr-Commit-Position: refs/heads/master@{#432309}
==========

[commit-bot: I haz the power, 2016-11-16 00:23:03]

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