Bug 34082

Created attachment 47336 [details] Patch

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.

Created attachment 47927 [details] Patch

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.

ping?

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.

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.

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 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());

> 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

Created attachment 49289 [details] Patch

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?

CCing ddkilzer as he might know who on the apple team (iphone or otherwise) is involved in geolocation.

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

Adding joth to CC list, who's working on Geolocation in Chrome.

Out of curiosity, why add the suspend() and resume() methods to Page instead of Document?

> 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?

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

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

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

> 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?

+ 2 active dom object experts From what I know, ActiveDOMObject is the correct way to implement this. Can either of you provide some advice?

(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 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?

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

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.

Closing this bug as invalid as it won't be an issue once the switch to client-based Geolocation is complete. See Bug 40373.