Don't show the Location Settings Dialog if the tab isn't interactable.

Don't show the Location Settings Dialog if the tab isn't interactable.

If the tab for the web_contents that requested location isn't interactable, the Location Settings Dialog shouldn't be shown. This could happen if the user changed tabs, is in the tab switcher, or if Chrome isn't active.

BUG=710255
Review-Url: https://codereview.chromium.org/2817873002
Cr-Commit-Position: refs/heads/master@{#465795}
Committed: https://chromium.googlesource.com/chromium/src/+/b8a8a089caa1188776c83e4344df494d6f30b969

[benwells, 2017-04-13 07:43:20]

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

[commit-bot: I haz the power, 2017-04-13 07:43:45]

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-04-13 09:02:49]

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

[commit-bot: I haz the power, 2017-04-13 09:02:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[benwells, 2017-04-13 10:58:53]

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

[commit-bot: I haz the power, 2017-04-13 10:59:04]

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-04-13 11:30:34]

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

[commit-bot: I haz the power, 2017-04-13 11:30:35]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-13 11:35:43]

benwells@chromium.org changed reviewers:
+ msramek@chromium.org, tedchoc@chromium.org

[benwells, 2017-04-13 11:35:44]

+msramek for privacy eyes
+tedchoc for android eyes

[msramek, 2017-04-13 11:41:28]

LGTM, thanks for the fix!

[Ted C, 2017-04-13 15:47:08]

lgtm w/ long rambling thoughts

https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:246: if
(tab_model && tab_model->GetActiveWebContents() != web_contents) {
I wonder if this should be !tab_model ||

Though that might break things if we were ever to enable location requesting in
panels like contextual search where it isn't associated with a model.

Keep it as is, I wonder if we should add something like
!web_contents->IsHidden() or something to capture other cases?

We also added TabAndroid::IsUserInteractable to handle cases where you are in
the tab switcher where we wouldn't necessarily want to popup stuff either.  We
likely could use that call instead of the tab model if we wanted to be a bit
more restrictive.

[benwells, 2017-04-14 00:13:53]

https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:246: if
(tab_model && tab_model->GetActiveWebContents() != web_contents) {
On 2017/04/13 15:47:08, Ted C wrote:
> I wonder if this should be !tab_model ||

It was like this originally, but it broke some tests that have a null tab model.

> 
> Though that might break things if we were ever to enable location requesting
in
> panels like contextual search where it isn't associated with a model.
> 
> Keep it as is, I wonder if we should add something like
> !web_contents->IsHidden() or something to capture other cases?
> 
> We also added TabAndroid::IsUserInteractable to handle cases where you are in
> the tab switcher where we wouldn't necessarily want to popup stuff either.  We
> likely could use that call instead of the tab model if we wanted to be a bit
> more restrictive.

I think that's a good idea. I'll poke with that next week. Another case to
handle is that Chrome isn't the active application ... I was going to handle
that on the Java side but maybe IsUserInteractable or / or IsHidden can do it
instead.

[Ted C, 2017-04-14 16:27:34]

On 2017/04/14 00:13:53, benwells wrote:
>
https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
> File chrome/browser/geolocation/geolocation_permission_context_android.cc
> (right):
> 
>
https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
> chrome/browser/geolocation/geolocation_permission_context_android.cc:246: if
> (tab_model && tab_model->GetActiveWebContents() != web_contents) {
> On 2017/04/13 15:47:08, Ted C wrote:
> > I wonder if this should be !tab_model ||
> 
> It was like this originally, but it broke some tests that have a null tab
model.
> 
> > 
> > Though that might break things if we were ever to enable location requesting
> in
> > panels like contextual search where it isn't associated with a model.
> > 
> > Keep it as is, I wonder if we should add something like
> > !web_contents->IsHidden() or something to capture other cases?
> > 
> > We also added TabAndroid::IsUserInteractable to handle cases where you are
in
> > the tab switcher where we wouldn't necessarily want to popup stuff either. 
We
> > likely could use that call instead of the tab model if we wanted to be a bit
> > more restrictive.
> 
> I think that's a good idea. I'll poke with that next week. Another case to
> handle is that Chrome isn't the active application ... I was going to handle
> that on the Java side but maybe IsUserInteractable or / or IsHidden can do it
> instead.

I "think" IsUserInteractable handles that as we call Tab#hide in
Tab#onActivityHidden.

It probably would return true if you're in multi-window but interacting with
another application.  But I think it still gets you much closer to it.

[Ted C, 2017-04-14 16:27:40]

On 2017/04/14 00:13:53, benwells wrote:
>
https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
> File chrome/browser/geolocation/geolocation_permission_context_android.cc
> (right):
> 
>
https://codereview.chromium.org/2817873002/diff/20001/chrome/browser/geolocat...
> chrome/browser/geolocation/geolocation_permission_context_android.cc:246: if
> (tab_model && tab_model->GetActiveWebContents() != web_contents) {
> On 2017/04/13 15:47:08, Ted C wrote:
> > I wonder if this should be !tab_model ||
> 
> It was like this originally, but it broke some tests that have a null tab
model.
> 
> > 
> > Though that might break things if we were ever to enable location requesting
> in
> > panels like contextual search where it isn't associated with a model.
> > 
> > Keep it as is, I wonder if we should add something like
> > !web_contents->IsHidden() or something to capture other cases?
> > 
> > We also added TabAndroid::IsUserInteractable to handle cases where you are
in
> > the tab switcher where we wouldn't necessarily want to popup stuff either. 
We
> > likely could use that call instead of the tab model if we wanted to be a bit
> > more restrictive.
> 
> I think that's a good idea. I'll poke with that next week. Another case to
> handle is that Chrome isn't the active application ... I was going to handle
> that on the Java side but maybe IsUserInteractable or / or IsHidden can do it
> instead.

I "think" IsUserInteractable handles that as we call Tab#hide in
Tab#onActivityHidden.

It probably would return true if you're in multi-window but interacting with
another application.  But I think it still gets you much closer to it.

[benwells, 2017-04-19 05:24:03]

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

[commit-bot: I haz the power, 2017-04-19 05:24:20]

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

[benwells, 2017-04-19 05:25:27]

Description was changed from

==========
Don't show the Location Settings Dialog if the current tab has changed.

If the active tab isn't for the web contents that requested location,
don't show the Location Settings Dialog.

BUG=710255
==========

to

==========
Don't show the Location Settings Dialog if the tab isn't interactable.

If the tab for the web_contents that requested location isn't interactable, the
Location Settings Dialog shouldn't be shown. This could happen if the user
changed tabs, is in the tab switcher, or if Chrome isn't active.

BUG=710255
==========

[commit-bot: I haz the power, 2017-04-19 06:48:34]

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

[commit-bot: I haz the power, 2017-04-19 06:48:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[benwells, 2017-04-19 07:14:10]

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

[commit-bot: I haz the power, 2017-04-19 07:14:40]

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-04-19 07:58:37]

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

[commit-bot: I haz the power, 2017-04-19 07:58:37]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-19 08:34:49]

Ted - this is using IsUserInteractable. Care to take another look?

[Ted C, 2017-04-19 17:03:51]

lgtm

[benwells, 2017-04-19 22:47:23]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-04-19 22:47:23]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2817873002/#ps80001
(title: "Tests again")

[commit-bot: I haz the power, 2017-04-19 22:48:04]

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-04-19 22:53:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492642043250780,
"parent_rev": "f628d6b4367d88b92e0bcf56886450e26ed58d55", "commit_rev":
"b8a8a089caa1188776c83e4344df494d6f30b969"}

[commit-bot: I haz the power, 2017-04-19 22:53:15]

Description was changed from

==========
Don't show the Location Settings Dialog if the tab isn't interactable.

If the tab for the web_contents that requested location isn't interactable, the
Location Settings Dialog shouldn't be shown. This could happen if the user
changed tabs, is in the tab switcher, or if Chrome isn't active.

BUG=710255
==========

to

==========
Don't show the Location Settings Dialog if the tab isn't interactable.

If the tab for the web_contents that requested location isn't interactable, the
Location Settings Dialog shouldn't be shown. This could happen if the user
changed tabs, is in the tab switcher, or if Chrome isn't active.

BUG=710255

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

[commit-bot: I haz the power, 2017-04-19 22:53:16]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b8a8a089caa1188776c83e4344df...