Bug 34082 - [Android] Android Geolocation service should not start if the WebView is paused
Summary: [Android] Android Geolocation service should not start if the WebView is paused
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 35313
  Show dependency treegraph
 
Reported: 2010-01-25 04:32 PST by Steve Block
Modified: 2011-03-24 04:01 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.83 KB, patch)
2010-01-25 04:41 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2010-02-02 05:45 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (14.03 KB, patch)
2010-02-23 06:58 PST, Steve Block
jorlow: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-01-25 04:32:21 PST
The Android Geolocation service should not start if the WebView is paused. This prevents edge-cases where script initiates the Geolocation service after the page has been put into the background. In this case, we do not start the service. It will be started when the service is resumed.
Comment 1 Steve Block 2010-01-25 04:41:17 PST
Created attachment 47336 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-01-26 14:59:10 PST
Comment on attachment 47336 [details]
Patch

This seems liek the wrong design.  Why wouldnt' WebKit reach down into WebCore and set some sort of pause?

Or ask some sort of client if it should start the location.
Comment 3 Steve Block 2010-02-02 05:45:02 PST
Created attachment 47927 [details]
Patch
Comment 4 Steve Block 2010-02-02 05:46:09 PST
The new patch adds suspend/resume methods to Page, which can be called from WebKit. This determines whether the Geolocation object is created in a suspended state.
Comment 5 Steve Block 2010-02-11 08:26:41 PST
ping?
Comment 6 Eric Seidel (no email) 2010-02-11 11:25:06 PST
Comment on attachment 47927 [details]
Patch

What does it mean for a page to be suspended?  If it only pertains to Geolocation that seems a bit odd.  There must already be a concept of "suspended" in WebCore?  What does Chromium do when a tab is not foreground?  Are there circumstances in Safari or the iPhone where Geolocation would pause or other per-page things would pause?

You're also aware that Frames can transfer between pages, right?  iFrames can be moved from one window to another as of recently, so if your code should be robust against the Page* changing.
Comment 7 Eric Seidel (no email) 2010-02-11 11:26:09 PST
Those weren't really comments positive or negative about the patch.  Mostly questions that it seems to make sense to ask about this code.  I assume similar questions went through your head when writing it.
Comment 8 Steve Block 2010-02-15 15:27:41 PST
Thanks for the comments Eric

> (From update of attachment 47927 [details])
> What does it mean for a page to be suspended?  If it only pertains to
> Geolocation that seems a bit odd.  There must already be a concept of
> "suspended" in WebCore?
I don't think there's currently a concept of page suspension in WebCore. Right now, suspension applies only to Geolocation, though I can imagine that other APIs might require a similar concept in the future, which is why I used generic names for the Page methods. The motivation for suspending Geolocation is to turn off the GPS, which consumes significant power, on mobile devices when the page is not in the foreground. I added the suspend/pause methods in response to your earlier comments - did I misinterpret them?

> Are there circumstances in Safari or the iPhone where Geolocation would pause
> or other per-page things would pause?
I'm not familiar with the iPhone Geolocation code, but I imagine that it too would want to turn off GPS for browser pages not in the foreground. The need is probably less important on desktop platforms.
 
> You're also aware that Frames can transfer between pages, right?  iFrames can
> be moved from one window to another as of recently, so if your code should be
> robust against the Page* changing.
Thanks for the heads up. I'll look into this more closely. Perhaps you can look at the general structure of the change while I check this?
Comment 9 Eric Seidel (no email) 2010-02-22 14:31:38 PST
Comment on attachment 47927 [details]
Patch

Please use an Enum instead of true/false to make callers clearer.
+PassRefPtr<Geolocation> Geolocation::create(Frame* frame, bool suspend)

Wast the page() suspension stuff already added separatedly?
 155         m_geolocation = Geolocation::create(m_frame, m_frame && m_frame->page()->isSuspended());
Comment 10 Steve Block 2010-02-23 06:52:10 PST
> Please use an Enum instead of true/false to make callers clearer.
> +PassRefPtr<Geolocation> Geolocation::create(Frame* frame, bool suspend)
OK, will do

> Wast the page() suspension stuff already added separatedly?
>  155         m_geolocation = Geolocation::create(m_frame, m_frame &&
> m_frame->page()->isSuspended());
No, it's added in this patch
Comment 11 Steve Block 2010-02-23 06:58:12 PST
Created attachment 49289 [details]
Patch
Comment 12 Eric Seidel (no email) 2010-02-23 12:15:25 PST
Comment on attachment 49289 [details]
Patch

So Page::suspend() and Page::resume() are called by the embedder?  Why doesn't Page::resume() do anything to already suspended geolocations?

Is Geolocation done entirely in WebCore?  I'm surprised that the embedder can't control this directly from the embedder's side of things.  Why is WebCore code required at all.  Can't the embedder just drop the geolocation requests on the floor when it wants to?
Comment 13 Eric Seidel (no email) 2010-02-23 12:15:59 PST
CCing ddkilzer as he might know who on the apple team (iphone or otherwise) is involved in geolocation.
Comment 14 Steve Block 2010-02-23 13:04:26 PST
> So Page::suspend() and Page::resume() are called by the embedder?
Correct

>  Why doesn't
> Page::resume() do anything to already suspended geolocations?
If we go for this route of adding Page::suspend/resume, it should. Currently in
Android, calling Geolocation::suspend/resume is done from WebKit, but it would
be better to do it from Page::suspend/resume. This is already on my list of
things to fix. I've opened Bug 35313 to track this.

> Is Geolocation done entirely in WebCore?
The platform implementations of GeolocationService live in WebCore/platform.

> I'm surprised that the embedder can't
> control this directly from the embedder's side of things. Why is WebCore code
> required at all.  Can't the embedder just drop the geolocation requests on the
> floor when it wants to?
It could. This was my approach in my first patch on this bug, where
GeolocationServiceAndroid used a new PlatformBridge::isWebViewPaused() method
to determine whether the page is suspended. But you suggested, rightly I think,
that it would be neater if WebKit were to 'reach down into WebCore and set some
sort of pause'.
Comment 15 Steve Block 2010-02-24 13:33:22 PST
Adding joth to CC list, who's working on Geolocation in Chrome.
Comment 16 David Kilzer (:ddkilzer) 2010-02-25 22:46:40 PST
Out of curiosity, why add the suspend() and resume() methods to Page instead of Document?
Comment 17 Steve Block 2010-02-26 02:38:27 PST
> Out of curiosity, why add the suspend() and resume() methods to Page instead of
> Document?
The use case this is trying to solve is to make sure that when a browser window/tab is in the background, all Geolocation objects are suspended. So we want to suspend the entire Page, not just individual Documents.

Is there a particular reason why you suggested adding the methods to Document?
Comment 18 David Kilzer (:ddkilzer) 2010-02-26 21:51:39 PST
(In reply to comment #17)
> > Out of curiosity, why add the suspend() and resume() methods to Page instead of
> > Document?
> The use case this is trying to solve is to make sure that when a browser
> window/tab is in the background, all Geolocation objects are suspended. So we
> want to suspend the entire Page, not just individual Documents.
> 
> Is there a particular reason why you suggested adding the methods to Document?

The iPhone port overrides methods that already exist on ScriptExecutionContext to suspend and resume Geolocation services:  suspendActiveDOMObjects(), resumeActiveDOMObjects() and stopActiveDOMObjects().  Document extends ScriptExecutionContext, so this seemed like a natural place for them.
Comment 19 Steve Block 2010-03-02 08:58:25 PST
> The iPhone port overrides methods that already exist on ScriptExecutionContext
> to suspend and resume Geolocation services:  suspendActiveDOMObjects(),
> resumeActiveDOMObjects() and stopActiveDOMObjects().
Do you mean that the iPhone overloads these methods in Document, or in another class that extends ScriptExecutionContext? These methods on Document are called from several places, so I'm not sure it's safe to override them.

> Document extends
> ScriptExecutionContext, so this seemed like a natural place for them.
Or did you mean that we should add new suspend and resume methods to Document?

Another option would be to make Geolocation inherit from ActiveDOMObject, so it could be suspended and resumed by the calls to the existing ScriptExecutionContext methods.
Comment 20 David Kilzer (:ddkilzer) 2010-03-02 14:29:22 PST
(In reply to comment #19)
> > The iPhone port overrides methods that already exist on ScriptExecutionContext
> > to suspend and resume Geolocation services:  suspendActiveDOMObjects(),
> > resumeActiveDOMObjects() and stopActiveDOMObjects().
> Do you mean that the iPhone overloads these methods in Document, or in another
> class that extends ScriptExecutionContext? These methods on Document are called
> from several places, so I'm not sure it's safe to override them.

Yes, it overloads them, but calls the overridden method on ScriptExecutionContext:: before doing its own work.

> > Document extends
> > ScriptExecutionContext, so this seemed like a natural place for them.
> Or did you mean that we should add new suspend and resume methods to Document?

I don't have a strong opinion here.  You have to walk the frame tree to find each Document no matter where you do it, so maybe it makes more sense to do it in Page than somewhere in each platform's WebKit implementation.

> Another option would be to make Geolocation inherit from ActiveDOMObject, so it
> could be suspended and resumed by the calls to the existing
> ScriptExecutionContext methods.

I like this option best (if it makes sense architecturally)!  Then you get the desired behavior for "free" by using existing API.  I am not the right person to ask if it makes sense, though, as I'm not intimately familiar with the concepts involved.  Sam Weinig may have an opinion on this approach.
Comment 21 Steve Block 2010-03-02 15:13:40 PST
> You have to walk the frame tree to find
> each Document no matter where you do it, so maybe it makes more sense to do it
> in Page than somewhere in each platform's WebKit implementation.
Exactly, so I think this is the best approach. Eric, would you be happy with this?

> I like this option best (if it makes sense architecturally)!  Then you get the
> desired behavior for "free" by using existing API.  I am not the right person
> to ask if it makes sense, though, as I'm not intimately familiar with the
> concepts involved.  Sam Weinig may have an opinion on this approach.
I'm not familiar with this either. I just came across them when looking at ScriptExecutionContext after your suggestion. I think that additional calls to ScriptExecutionContext::suspendActiveDOMObjects/resumeActiveDOMObjects would be required and I'm not sure whether this is safe for the other objects that implement ActiveDOMObject. Sam, do you know whether this approach would make sense architecturally and if it's safe?
Comment 22 Jeremy Orlow 2010-03-10 09:46:45 PST
+ 2 active dom object experts

From what I know, ActiveDOMObject is the correct way to implement this.  Can either of you provide some advice?
Comment 23 Andrew Wilson 2010-03-10 13:58:54 PST
(In reply to comment #22)
> From what I know, ActiveDOMObject is the correct way to implement this.  Can
> either of you provide some advice?

Agreed that ActiveDOMObject seems like the right way to go, especially since you get similar useful behavior in other cases that you may not have thought of (iframes going away, workers, back/forward history operations).

So using ActiveDOMObject and suspending the Document objects in the page seems like the right thing to do. Note that some ActiveDOMObjects (like Worker) don't handle suspend() (they return false from canSuspend()) so you'll still get some activity from those objects even when their parent Document is supposedly suspend()-ed.
Comment 24 Jeremy Orlow 2010-03-10 15:48:36 PST
Comment on attachment 49289 [details]
Patch

r- since this should be re-worked to use ActiveDOMObject

Can any of you offer Steve any specific advice or pointers on how to implement this?
Comment 25 Alexey Proskuryakov 2010-03-10 17:00:46 PST
> don't handle suspend() (they return false from canSuspend())

Side note: suspend() can still be called if canSuspend() returns true. One can prevent a document from going into a b/f cache, but a debugger can be attached and try to suspend() any time.
Comment 26 Steve Block 2010-12-14 04:40:44 PST
Note that the problem of how to architect the interaction between WebCore and WebKit will be completely avoided once we switch to client-based Geolocation, as the embedder will then have complete control over the Geolocation client.
Comment 27 Steve Block 2011-03-24 04:01:07 PDT
Closing this bug as invalid as it won't be an issue once the switch to client-based Geolocation is complete. See Bug 40373.