Remove ContentViewCore::PauseOrResumeGeolocation()

Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page
visibility mechanisim, so the logic associated with
onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
Committed: https://crrev.com/5df3a5d1a16eabf9c1d6ae306b5957596cb3b0ca
Cr-Commit-Position: refs/heads/master@{#425601}

[Jinsuk Kim, 2016-09-30 03:54:43]

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

[commit-bot: I haz the power, 2016-09-30 03:54:50]

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-30 05:23:29]

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

[commit-bot: I haz the power, 2016-09-30 05:23:34]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2016-09-30 05:53:39]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2016-09-30 22:59:43]

boliu@chromium.org changed reviewers:
+ timvolodine@chromium.org

[boliu, 2016-09-30 22:59:44]

this by itself is more or less fine. but this call was added before webview
supported page visibility. maybe we don't actually need this now

+timvolodine: does chrome pause or throttle geolocation updates when a tab is in
the background? how does that logic work exactly?

[timvolodine, 2016-10-03 13:02:42]

On 2016/09/30 22:59:44, boliu wrote:
> this by itself is more or less fine. but this call was added before webview
> supported page visibility. maybe we don't actually need this now
> 
> +timvolodine: does chrome pause or throttle geolocation updates when a tab is
in
> the background? how does that logic work exactly?

I think we actually close geolocation service mojo connection when page goes
invisible, i.e.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc...

(+cc:mvanouwerkerk@ correct me if I am missing anything)

[Michael van Ouwerkerk, 2016-10-03 16:13:01]

On 2016/10/03 13:02:42, timvolodine wrote:
> On 2016/09/30 22:59:44, boliu wrote:
> > this by itself is more or less fine. but this call was added before webview
> > supported page visibility. maybe we don't actually need this now
> > 
> > +timvolodine: does chrome pause or throttle geolocation updates when a tab
is
> in
> > the background? how does that logic work exactly?
> 
> I think we actually close geolocation service mojo connection when page goes
> invisible, i.e.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc...
> 
> (+cc:mvanouwerkerk@ correct me if I am missing anything)

Yes that's correct, assuming pageVisibilityChanged() actually gets called on
WebView.

[boliu, 2016-10-03 16:36:46]

On 2016/10/03 16:13:01, Michael van Ouwerkerk wrote:
> On 2016/10/03 13:02:42, timvolodine wrote:
> > On 2016/09/30 22:59:44, boliu wrote:
> > > this by itself is more or less fine. but this call was added before
webview
> > > supported page visibility. maybe we don't actually need this now
> > > 
> > > +timvolodine: does chrome pause or throttle geolocation updates when a tab
> is
> > in
> > > the background? how does that logic work exactly?
> > 
> > I think we actually close geolocation service mojo connection when page goes
> > invisible, i.e.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc...
> > 
> > (+cc:mvanouwerkerk@ correct me if I am missing anything)
> 
> Yes that's correct, assuming pageVisibilityChanged() actually gets called on
> WebView.

Yes, pageVisibilityChanged is called in WebView starting in m49, iirc. So great,
can just delete all this code.

[Jinsuk Kim, 2016-10-03 23:24:40]

Removed the ContentViewCoreImpl::PauseOrResumeGeolocation. Can
GeolocationServiceContext::Pause/ResumeUpdates() be removed as well? Still kept
now.

[bo658713, 2016-10-03 23:28:10]

I think so. WebView was the only caller.

On Oct 3, 2016 4:24 PM, <jinsukkim@chromium.org> wrote:

> Removed the ContentViewCoreImpl::PauseOrResumeGeolocation. Can
> GeolocationServiceContext::Pause/ResumeUpdates() be removed as well?
> Still kept
> now.
>
> https://codereview.chromium.org/2379253002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jinsuk Kim, 2016-10-03 23:57:16]

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

[commit-bot: I haz the power, 2016-10-03 23:57:58]

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

[boliu, 2016-10-04 00:25:00]

boliu@chromium.org changed reviewers:
- bo658713@gmail.com

[boliu, 2016-10-04 00:25:00]

lgtm!

you'll need someone else to review to review devices obviously

[boliu, 2016-10-04 00:43:42]

On 2016/10/04 00:25:00, boliu wrote:
> lgtm!
> 
> you'll need someone else to review to review devices obviously

oh, tim is already on the review. PTAL

[commit-bot: I haz the power, 2016-10-04 01:25:36]

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

[commit-bot: I haz the power, 2016-10-04 01:25:36]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-10-04 13:01:36]

On 2016/10/04 00:43:42, boliu wrote:
> On 2016/10/04 00:25:00, boliu wrote:
> > lgtm!
> > 
> > you'll need someone else to review to review devices obviously
> 
> oh, tim is already on the review. PTAL

device/geolocation looks ok to me, however I am slightly puzzled why the
PauseOrResumeGeolocation can be removed from the AwContents::SetIsPaused method?
It seems this can be called anytime via WebView.opPause right?

[boliu, 2016-10-04 14:29:07]

On 2016/10/04 13:01:36, timvolodine wrote:
> On 2016/10/04 00:43:42, boliu wrote:
> > On 2016/10/04 00:25:00, boliu wrote:
> > > lgtm!
> > > 
> > > you'll need someone else to review to review devices obviously
> > 
> > oh, tim is already on the review. PTAL
> 
> device/geolocation looks ok to me, however I am slightly puzzled why the
> PauseOrResumeGeolocation can be removed from the AwContents::SetIsPaused
method?
> It seems this can be called anytime via WebView.opPause right?

WebView.onPause already directly controls page visibility in webview.

[timvolodine, 2016-10-04 14:36:46]

On 2016/10/04 14:29:07, boliu wrote:
> On 2016/10/04 13:01:36, timvolodine wrote:
> > On 2016/10/04 00:43:42, boliu wrote:
> > > On 2016/10/04 00:25:00, boliu wrote:
> > > > lgtm!
> > > > 
> > > > you'll need someone else to review to review devices obviously
> > > 
> > > oh, tim is already on the review. PTAL
> > 
> > device/geolocation looks ok to me, however I am slightly puzzled why the
> > PauseOrResumeGeolocation can be removed from the AwContents::SetIsPaused
> method?
> > It seems this can be called anytime via WebView.opPause right?
> 
> WebView.onPause already directly controls page visibility in webview.

do you mean onPause will set visibility to false and implicitly disable
geolocation that way?
(I thought it was actually possible to invoke onPause on a visible WebView?)

[boliu, 2016-10-04 15:05:16]

On 2016/10/04 14:36:46, timvolodine wrote:
> On 2016/10/04 14:29:07, boliu wrote:
> > On 2016/10/04 13:01:36, timvolodine wrote:
> > > On 2016/10/04 00:43:42, boliu wrote:
> > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > lgtm!
> > > > > 
> > > > > you'll need someone else to review to review devices obviously
> > > > 
> > > > oh, tim is already on the review. PTAL
> > > 
> > > device/geolocation looks ok to me, however I am slightly puzzled why the
> > > PauseOrResumeGeolocation can be removed from the AwContents::SetIsPaused
> > method?
> > > It seems this can be called anytime via WebView.opPause right?
> > 
> > WebView.onPause already directly controls page visibility in webview.
> 
> do you mean onPause will set visibility to false and implicitly disable
> geolocation that way?

Yes

> (I thought it was actually possible to invoke onPause on a visible WebView?)

Well, what exactly do you mean by "visible" webview, because there's a lot of
"visibility" concepts floating around. Actually I explained it on the recent
android-webview-dev thread:
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...

[timvolodine, 2016-10-04 15:31:41]

On 2016/10/04 15:05:16, boliu wrote:
> On 2016/10/04 14:36:46, timvolodine wrote:
> > On 2016/10/04 14:29:07, boliu wrote:
> > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > lgtm!
> > > > > > 
> > > > > > you'll need someone else to review to review devices obviously
> > > > > 
> > > > > oh, tim is already on the review. PTAL
> > > > 
> > > > device/geolocation looks ok to me, however I am slightly puzzled why the
> > > > PauseOrResumeGeolocation can be removed from the AwContents::SetIsPaused
> > > method?
> > > > It seems this can be called anytime via WebView.opPause right?
> > > 
> > > WebView.onPause already directly controls page visibility in webview.
> > 
> > do you mean onPause will set visibility to false and implicitly disable
> > geolocation that way?
> 
> Yes
> 
> > (I thought it was actually possible to invoke onPause on a visible WebView?)
> 
> Well, what exactly do you mean by "visible" webview, because there's a lot of
> "visibility" concepts floating around. Actually I explained it on the recent
> android-webview-dev thread:
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...

Ok thanks for the pointer. Basically my concern was that webview.onPause should
disable geolocation, it's also explicitly stated in the android docs:
https://developer.android.com/reference/android/webkit/WebView.html#onPause()

jinsukkim@ do we have any tests for this? can this be tested (manually) to be
sure?
maybe leave a comment somewhere to say that geolocation will be disabled via the
visibility mechanism.

[boliu, 2016-10-04 15:35:41]

On 2016/10/04 15:31:41, timvolodine wrote:
> On 2016/10/04 15:05:16, boliu wrote:
> > On 2016/10/04 14:36:46, timvolodine wrote:
> > > On 2016/10/04 14:29:07, boliu wrote:
> > > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > > lgtm!
> > > > > > > 
> > > > > > > you'll need someone else to review to review devices obviously
> > > > > > 
> > > > > > oh, tim is already on the review. PTAL
> > > > > 
> > > > > device/geolocation looks ok to me, however I am slightly puzzled why
the
> > > > > PauseOrResumeGeolocation can be removed from the
AwContents::SetIsPaused
> > > > method?
> > > > > It seems this can be called anytime via WebView.opPause right?
> > > > 
> > > > WebView.onPause already directly controls page visibility in webview.
> > > 
> > > do you mean onPause will set visibility to false and implicitly disable
> > > geolocation that way?
> > 
> > Yes
> > 
> > > (I thought it was actually possible to invoke onPause on a visible
WebView?)
> > 
> > Well, what exactly do you mean by "visible" webview, because there's a lot
of
> > "visibility" concepts floating around. Actually I explained it on the recent
> > android-webview-dev thread:
> >
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...
> 
> Ok thanks for the pointer. Basically my concern was that webview.onPause
should
> disable geolocation,

I mean that was my concern too, and then I asked you if chrome pauses
geolocation updates on visibility change, and you said yes :p

> it's also explicitly stated in the android docs:
> https://developer.android.com/reference/android/webkit/WebView.html#onPause()

I think we updated android docs after we made this change.. but don't really
remember

> 
> jinsukkim@ do we have any tests for this?

Probably not..? Any test that want to check something *doesn't* happen is
probably super flaky

> can this be tested (manually) to be sure?

Yeah I guess.

> maybe leave a comment somewhere to say that geolocation will be disabled via
the
> visibility mechanism.

[Jinsuk Kim, 2016-10-05 00:42:00]

On 2016/10/04 15:35:41, boliu wrote:
> On 2016/10/04 15:31:41, timvolodine wrote:
> > On 2016/10/04 15:05:16, boliu wrote:
> > > On 2016/10/04 14:36:46, timvolodine wrote:
> > > > On 2016/10/04 14:29:07, boliu wrote:
> > > > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > > > lgtm!
> > > > > > > > 
> > > > > > > > you'll need someone else to review to review devices obviously
> > > > > > > 
> > > > > > > oh, tim is already on the review. PTAL
> > > > > > 
> > > > > > device/geolocation looks ok to me, however I am slightly puzzled why
> the
> > > > > > PauseOrResumeGeolocation can be removed from the
> AwContents::SetIsPaused
> > > > > method?
> > > > > > It seems this can be called anytime via WebView.opPause right?
> > > > > 
> > > > > WebView.onPause already directly controls page visibility in webview.
> > > > 
> > > > do you mean onPause will set visibility to false and implicitly disable
> > > > geolocation that way?
> > > 
> > > Yes
> > > 
> > > > (I thought it was actually possible to invoke onPause on a visible
> WebView?)
> > > 
> > > Well, what exactly do you mean by "visible" webview, because there's a lot
> of
> > > "visibility" concepts floating around. Actually I explained it on the
recent
> > > android-webview-dev thread:
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...
> > 
> > Ok thanks for the pointer. Basically my concern was that webview.onPause
> should
> > disable geolocation,
> 
> I mean that was my concern too, and then I asked you if chrome pauses
> geolocation updates on visibility change, and you said yes :p
> 
> > it's also explicitly stated in the android docs:
> >
https://developer.android.com/reference/android/webkit/WebView.html#onPause()
> 
> I think we updated android docs after we made this change.. but don't really
> remember
> 
> > 
> > jinsukkim@ do we have any tests for this?
> 
> Probably not..? Any test that want to check something *doesn't* happen is
> probably super flaky
> 
> > can this be tested (manually) to be sure?
> 
> Yeah I guess.
> 

I verified with Android WebView test shell that geolocation service gets
reset through |pageVisibilityChanged| when the shell is sent background.

[boliu, 2016-10-05 15:33:55]

On 2016/10/05 00:42:00, Jinsuk_OOO (- Oct 10) wrote:
> I verified with Android WebView test shell that geolocation service gets
> reset through |pageVisibilityChanged| when the shell is sent background.

tim: ping!

[timvolodine, 2016-10-05 15:53:14]

On 2016/10/05 00:42:00, Jinsuk_OOO (- Oct 10) wrote:
> On 2016/10/04 15:35:41, boliu wrote:
> > On 2016/10/04 15:31:41, timvolodine wrote:
> > > On 2016/10/04 15:05:16, boliu wrote:
> > > > On 2016/10/04 14:36:46, timvolodine wrote:
> > > > > On 2016/10/04 14:29:07, boliu wrote:
> > > > > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > > > > lgtm!
> > > > > > > > > 
> > > > > > > > > you'll need someone else to review to review devices obviously
> > > > > > > > 
> > > > > > > > oh, tim is already on the review. PTAL
> > > > > > > 
> > > > > > > device/geolocation looks ok to me, however I am slightly puzzled
why
> > the
> > > > > > > PauseOrResumeGeolocation can be removed from the
> > AwContents::SetIsPaused
> > > > > > method?
> > > > > > > It seems this can be called anytime via WebView.opPause right?
> > > > > > 
> > > > > > WebView.onPause already directly controls page visibility in
webview.
> > > > > 
> > > > > do you mean onPause will set visibility to false and implicitly
disable
> > > > > geolocation that way?
> > > > 
> > > > Yes
> > > > 
> > > > > (I thought it was actually possible to invoke onPause on a visible
> > WebView?)
> > > > 
> > > > Well, what exactly do you mean by "visible" webview, because there's a
lot
> > of
> > > > "visibility" concepts floating around. Actually I explained it on the
> recent
> > > > android-webview-dev thread:
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...
> > > 
> > > Ok thanks for the pointer. Basically my concern was that webview.onPause
> > should
> > > disable geolocation,
> > 
> > I mean that was my concern too, and then I asked you if chrome pauses
> > geolocation updates on visibility change, and you said yes :p
> > 
> > > it's also explicitly stated in the android docs:
> > >
> https://developer.android.com/reference/android/webkit/WebView.html#onPause()
> > 
> > I think we updated android docs after we made this change.. but don't really
> > remember
> > 
> > > 
> > > jinsukkim@ do we have any tests for this?
> > 
> > Probably not..? Any test that want to check something *doesn't* happen is
> > probably super flaky
> > 
> > > can this be tested (manually) to be sure?
> > 
> > Yeah I guess.
> > 
> 
> I verified with Android WebView test shell that geolocation service gets
> reset through |pageVisibilityChanged| when the shell is sent background.

Going to background already had this behavior (due to page visibility).
Can we verify that calling the onPause method has the effect of stopping
geolocation?

[boliu, 2016-10-05 15:57:22]

On 2016/10/05 15:53:14, timvolodine wrote:
> On 2016/10/05 00:42:00, Jinsuk_OOO (- Oct 10) wrote:
> > On 2016/10/04 15:35:41, boliu wrote:
> > > On 2016/10/04 15:31:41, timvolodine wrote:
> > > > On 2016/10/04 15:05:16, boliu wrote:
> > > > > On 2016/10/04 14:36:46, timvolodine wrote:
> > > > > > On 2016/10/04 14:29:07, boliu wrote:
> > > > > > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > > > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > > > > > lgtm!
> > > > > > > > > > 
> > > > > > > > > > you'll need someone else to review to review devices
obviously
> > > > > > > > > 
> > > > > > > > > oh, tim is already on the review. PTAL
> > > > > > > > 
> > > > > > > > device/geolocation looks ok to me, however I am slightly puzzled
> why
> > > the
> > > > > > > > PauseOrResumeGeolocation can be removed from the
> > > AwContents::SetIsPaused
> > > > > > > method?
> > > > > > > > It seems this can be called anytime via WebView.opPause right?
> > > > > > > 
> > > > > > > WebView.onPause already directly controls page visibility in
> webview.
> > > > > > 
> > > > > > do you mean onPause will set visibility to false and implicitly
> disable
> > > > > > geolocation that way?
> > > > > 
> > > > > Yes
> > > > > 
> > > > > > (I thought it was actually possible to invoke onPause on a visible
> > > WebView?)
> > > > > 
> > > > > Well, what exactly do you mean by "visible" webview, because there's a
> lot
> > > of
> > > > > "visibility" concepts floating around. Actually I explained it on the
> > recent
> > > > > android-webview-dev thread:
> > > > >
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...
> > > > 
> > > > Ok thanks for the pointer. Basically my concern was that webview.onPause
> > > should
> > > > disable geolocation,
> > > 
> > > I mean that was my concern too, and then I asked you if chrome pauses
> > > geolocation updates on visibility change, and you said yes :p
> > > 
> > > > it's also explicitly stated in the android docs:
> > > >
> >
https://developer.android.com/reference/android/webkit/WebView.html#onPause()
> > > 
> > > I think we updated android docs after we made this change.. but don't
really
> > > remember
> > > 
> > > > 
> > > > jinsukkim@ do we have any tests for this?
> > > 
> > > Probably not..? Any test that want to check something *doesn't* happen is
> > > probably super flaky
> > > 
> > > > can this be tested (manually) to be sure?
> > > 
> > > Yeah I guess.
> > > 
> > 
> > I verified with Android WebView test shell that geolocation service gets
> > reset through |pageVisibilityChanged| when the shell is sent background.
> 
> Going to background already had this behavior (due to page visibility).
> Can we verify that calling the onPause method has the effect of stopping
> geolocation?

AwContents.onPause already causes pageVisibilityChanged. AwContents.onPause
calls updateContentViewCoreVisibility, and you can trace that all the way to
WebContentsImpl::WasHidden, and to blink. I mean.. what's the point of manually
verifying all of that?

[timvolodine, 2016-10-05 18:01:26]

On 2016/10/05 15:57:22, boliu wrote:
> On 2016/10/05 15:53:14, timvolodine wrote:
> > On 2016/10/05 00:42:00, Jinsuk_OOO (- Oct 10) wrote:
> > > On 2016/10/04 15:35:41, boliu wrote:
> > > > On 2016/10/04 15:31:41, timvolodine wrote:
> > > > > On 2016/10/04 15:05:16, boliu wrote:
> > > > > > On 2016/10/04 14:36:46, timvolodine wrote:
> > > > > > > On 2016/10/04 14:29:07, boliu wrote:
> > > > > > > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > > > > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > > > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > > > > > > lgtm!
> > > > > > > > > > > 
> > > > > > > > > > > you'll need someone else to review to review devices
> obviously
> > > > > > > > > > 
> > > > > > > > > > oh, tim is already on the review. PTAL
> > > > > > > > > 
> > > > > > > > > device/geolocation looks ok to me, however I am slightly
puzzled
> > why
> > > > the
> > > > > > > > > PauseOrResumeGeolocation can be removed from the
> > > > AwContents::SetIsPaused
> > > > > > > > method?
> > > > > > > > > It seems this can be called anytime via WebView.opPause right?
> > > > > > > > 
> > > > > > > > WebView.onPause already directly controls page visibility in
> > webview.
> > > > > > > 
> > > > > > > do you mean onPause will set visibility to false and implicitly
> > disable
> > > > > > > geolocation that way?
> > > > > > 
> > > > > > Yes
> > > > > > 
> > > > > > > (I thought it was actually possible to invoke onPause on a visible
> > > > WebView?)
> > > > > > 
> > > > > > Well, what exactly do you mean by "visible" webview, because there's
a
> > lot
> > > > of
> > > > > > "visibility" concepts floating around. Actually I explained it on
the
> > > recent
> > > > > > android-webview-dev thread:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...
> > > > > 
> > > > > Ok thanks for the pointer. Basically my concern was that
webview.onPause
> > > > should
> > > > > disable geolocation,
> > > > 
> > > > I mean that was my concern too, and then I asked you if chrome pauses
> > > > geolocation updates on visibility change, and you said yes :p
> > > > 
> > > > > it's also explicitly stated in the android docs:
> > > > >
> > >
> https://developer.android.com/reference/android/webkit/WebView.html#onPause()
> > > > 
> > > > I think we updated android docs after we made this change.. but don't
> really
> > > > remember
> > > > 
> > > > > 
> > > > > jinsukkim@ do we have any tests for this?
> > > > 
> > > > Probably not..? Any test that want to check something *doesn't* happen
is
> > > > probably super flaky
> > > > 
> > > > > can this be tested (manually) to be sure?
> > > > 
> > > > Yeah I guess.
> > > > 
> > > 
> > > I verified with Android WebView test shell that geolocation service gets
> > > reset through |pageVisibilityChanged| when the shell is sent background.
> > 
> > Going to background already had this behavior (due to page visibility).
> > Can we verify that calling the onPause method has the effect of stopping
> > geolocation?
> 
> AwContents.onPause already causes pageVisibilityChanged. AwContents.onPause
> calls updateContentViewCoreVisibility, and you can trace that all the way to
> WebContentsImpl::WasHidden, and to blink. I mean.. what's the point of
manually
> verifying all of that?

Because we have no tests for this and the patch tries to optimize geolocation
behavior I thought it's reasonable to assume it can be tested to verify that it
keeps the 'contract' for onPause we have in android docs. The change didn't seem
very trivial (at least to me) hence the desire for a test-run.

[Jinsuk Kim, 2016-10-11 02:04:24]

On 2016/10/05 18:01:26, timvolodine wrote:
> On 2016/10/05 15:57:22, boliu wrote:
> > On 2016/10/05 15:53:14, timvolodine wrote:
> > > On 2016/10/05 00:42:00, Jinsuk_OOO (- Oct 10) wrote:
> > > > On 2016/10/04 15:35:41, boliu wrote:
> > > > > On 2016/10/04 15:31:41, timvolodine wrote:
> > > > > > On 2016/10/04 15:05:16, boliu wrote:
> > > > > > > On 2016/10/04 14:36:46, timvolodine wrote:
> > > > > > > > On 2016/10/04 14:29:07, boliu wrote:
> > > > > > > > > On 2016/10/04 13:01:36, timvolodine wrote:
> > > > > > > > > > On 2016/10/04 00:43:42, boliu wrote:
> > > > > > > > > > > On 2016/10/04 00:25:00, boliu wrote:
> > > > > > > > > > > > lgtm!
> > > > > > > > > > > > 
> > > > > > > > > > > > you'll need someone else to review to review devices
> > obviously
> > > > > > > > > > > 
> > > > > > > > > > > oh, tim is already on the review. PTAL
> > > > > > > > > > 
> > > > > > > > > > device/geolocation looks ok to me, however I am slightly
> puzzled
> > > why
> > > > > the
> > > > > > > > > > PauseOrResumeGeolocation can be removed from the
> > > > > AwContents::SetIsPaused
> > > > > > > > > method?
> > > > > > > > > > It seems this can be called anytime via WebView.opPause
right?
> > > > > > > > > 
> > > > > > > > > WebView.onPause already directly controls page visibility in
> > > webview.
> > > > > > > > 
> > > > > > > > do you mean onPause will set visibility to false and implicitly
> > > disable
> > > > > > > > geolocation that way?
> > > > > > > 
> > > > > > > Yes
> > > > > > > 
> > > > > > > > (I thought it was actually possible to invoke onPause on a
visible
> > > > > WebView?)
> > > > > > > 
> > > > > > > Well, what exactly do you mean by "visible" webview, because
there's
> a
> > > lot
> > > > > of
> > > > > > > "visibility" concepts floating around. Actually I explained it on
> the
> > > > recent
> > > > > > > android-webview-dev thread:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/android-webview-dev/LpBaOxxcAh...
> > > > > > 
> > > > > > Ok thanks for the pointer. Basically my concern was that
> webview.onPause
> > > > > should
> > > > > > disable geolocation,
> > > > > 
> > > > > I mean that was my concern too, and then I asked you if chrome pauses
> > > > > geolocation updates on visibility change, and you said yes :p
> > > > > 
> > > > > > it's also explicitly stated in the android docs:
> > > > > >
> > > >
> >
https://developer.android.com/reference/android/webkit/WebView.html#onPause()
> > > > > 
> > > > > I think we updated android docs after we made this change.. but don't
> > really
> > > > > remember
> > > > > 
> > > > > > 
> > > > > > jinsukkim@ do we have any tests for this?
> > > > > 
> > > > > Probably not..? Any test that want to check something *doesn't* happen
> is
> > > > > probably super flaky
> > > > > 
> > > > > > can this be tested (manually) to be sure?
> > > > > 
> > > > > Yeah I guess.
> > > > > 
> > > > 
> > > > I verified with Android WebView test shell that geolocation service gets
> > > > reset through |pageVisibilityChanged| when the shell is sent background.
> > > 
> > > Going to background already had this behavior (due to page visibility).
> > > Can we verify that calling the onPause method has the effect of stopping
> > > geolocation?
> > 
> > AwContents.onPause already causes pageVisibilityChanged. AwContents.onPause
> > calls updateContentViewCoreVisibility, and you can trace that all the way to
> > WebContentsImpl::WasHidden, and to blink. I mean.. what's the point of
> manually
> > verifying all of that?
> 
> Because we have no tests for this and the patch tries to optimize geolocation
> behavior I thought it's reasonable to assume it can be tested to verify that
it
> keeps the 'contract' for onPause we have in android docs. The change didn't
seem
> very trivial (at least to me) hence the desire for a test-run.

Used a test shell to trigger onPause|onResume where I get AwContents instance
and call its onPause/onResume manually - and verified that
AwContents.{onPause|onResume}, when triggered, invokes |pageVisibilityChanged|
as expected.

[timvolodine, 2016-10-12 15:12:45]

ok thanks, I assume this together with #27 implies that geolocation is stopped
when onPause|Resume is called?

could you pls update the description to explain why this can be removed?

https://codereview.chromium.org/2379253002/diff/40001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2379253002/diff/40001/android_webview/native/...
android_webview/native/aw_contents.cc:852:
browser_view_renderer_.SetIsPaused(paused);
could you add a comment saying that geolocation is paused via the page
visibility mechanism? not sure, maybe a better place for this is on the java
side..

[boliu, 2016-10-12 15:35:38]

https://codereview.chromium.org/2379253002/diff/40001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2379253002/diff/40001/android_webview/native/...
android_webview/native/aw_contents.cc:852:
browser_view_renderer_.SetIsPaused(paused);
On 2016/10/12 15:12:44, timvolodine wrote:
> could you add a comment saying that geolocation is paused via the page
> visibility mechanism? not sure, maybe a better place for this is on the java
> side..

I prefer java side too :)

[Jinsuk Kim, 2016-10-14 07:04:02]

https://codereview.chromium.org/2379253002/diff/40001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2379253002/diff/40001/android_webview/native/...
android_webview/native/aw_contents.cc:852:
browser_view_renderer_.SetIsPaused(paused);
On 2016/10/12 15:35:38, boliu wrote:
> On 2016/10/12 15:12:44, timvolodine wrote:
> > could you add a comment saying that geolocation is paused via the page
> > visibility mechanism? not sure, maybe a better place for this is on the java
> > side..
> 
> I prefer java side too :)

Added a comment in AwContents.updateContentViewCoreVisibility(). See if it was
put in a good spot.

[timvolodine, 2016-10-14 10:41:28]

https://codereview.chromium.org/2379253002/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2379253002/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2481: //
Geolocation is alos paused/resumed via the page visibility mechanism.
"alos"?

actually I was assuming you could put this comment inside onPause(), probably
better there as it was previously explicitly paused in setIsPaused..

[Jinsuk Kim, 2016-10-14 10:47:44]

https://codereview.chromium.org/2379253002/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2379253002/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2481: //
Geolocation is alos paused/resumed via the page visibility mechanism.
On 2016/10/14 10:41:28, timvolodine wrote:
> "alos"?
> 
> actually I was assuming you could put this comment inside onPause(), probably
> better there as it was previously explicitly paused in setIsPaused..

Done.

[timvolodine, 2016-10-14 11:06:07]

lgtm % comments

also please update the description to explain why this can be removed (I think I
already asked that)

https://codereview.chromium.org/2379253002/diff/80001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2379253002/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1897: //
Geolocation is paused/resumed via the page visibility mechanism.
sorry for the back and forth: maybe put it just after nativeSetIsPaused?

[Jinsuk Kim, 2016-10-16 22:43:01]

Description was changed from

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

to

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page visibility mechanisim, so the
logic associated with onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

[Jinsuk Kim, 2016-10-16 22:47:25]

Description was changed from

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page visibility mechanisim, so the
logic associated with onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

to

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page 
visibility mechanisim, so the logic associated with 
onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

[Jinsuk Kim, 2016-10-16 22:47:36]

https://codereview.chromium.org/2379253002/diff/80001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2379253002/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1897: //
Geolocation is paused/resumed via the page visibility mechanism.
On 2016/10/14 11:06:07, timvolodine wrote:
> sorry for the back and forth: maybe put it just after nativeSetIsPaused?

Done.

[Jinsuk Kim, 2016-10-16 23:05:24]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-10-16 23:05:25]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2379253002/#ps100001
(title: "comment")

[commit-bot: I haz the power, 2016-10-16 23:05:35]

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-10-16 23:55:36]

Description was changed from

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page 
visibility mechanisim, so the logic associated with 
onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

to

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page 
visibility mechanisim, so the logic associated with 
onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

[commit-bot: I haz the power, 2016-10-16 23:55:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-16 23:57:12]

Description was changed from

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page 
visibility mechanisim, so the logic associated with 
onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782
==========

to

==========
Remove ContentViewCore::PauseOrResumeGeolocation()

Geolocation is being paused/resumed via the page
visibility mechanisim, so the logic associated with
onPause/onResume can be removed safely.

This is part of an effort to remove/split up ContentViewCore.

BUG=626782

Committed: https://crrev.com/5df3a5d1a16eabf9c1d6ae306b5957596cb3b0ca
Cr-Commit-Position: refs/heads/master@{#425601}
==========

[commit-bot: I haz the power, 2016-10-16 23:57:13]

Patchset 6 (id:??) landed as
https://crrev.com/5df3a5d1a16eabf9c1d6ae306b5957596cb3b0ca
Cr-Commit-Position: refs/heads/master@{#425601}