Bug 147685 – Toggle GPS state based on page visibility to save battery

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Toggle GPS state based on page visibility to save battery. At the moment, if the site you're viewing watches your position and you switch tab, the GPS stays on. This is bad for battery life.

[Chris Dumez]

Created attachment 258293 [details]
Patch

[Benjamin Poulain]

Comment on attachment 258293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258293&action=review

Hehe, I did find a reason to r- :)

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:90
> +    if (!m_page.isVisible() && updating)

If I register an observer with the page visible:
    m_isUpdating = true;
Then the page become a background tab:
    m_isUpdating = false;
    m_observers.isEmpty() is false
The background page remove the observers:
    m_isUpdating = false;
    m_observers.isEmpty() is true
The page comes back to visible
    ->We start updating on a Controller without observers

...actually, wouldn't we start updating on view state changes even without observers involved at any point?

> LayoutTests/fast/dom/Geolocation/startUpdatingOnlyWhenPageVisible.html:26
> +        shouldBeFalse("internals.isGeolocationClientUpdating()");

IMHO, that is the wrong way to test.

You can test this from end to end, including any bug in the WebKit layer. Instead of observing if the controller is updating, you should observe if the Mock object is updating or not.
If you do that, the only source of bug should be the Provider's implementation.

[Chris Dumez]

Comment on attachment 258293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258293&action=review

>> Source/WebCore/Modules/geolocation/GeolocationController.cpp:90
>> +    if (!m_page.isVisible() && updating)
> 
> If I register an observer with the page visible:
>     m_isUpdating = true;
> Then the page become a background tab:
>     m_isUpdating = false;
>     m_observers.isEmpty() is false
> The background page remove the observers:
>     m_isUpdating = false;
>     m_observers.isEmpty() is true
> The page comes back to visible
>     ->We start updating on a Controller without observers
> 
> ...actually, wouldn't we start updating on view state changes even without observers involved at any point?

Your last step says:
"""
The page comes back to visible
    ->We start updating on a Controller without observers
"""

Why would this happen? In the viewStateDidChange handler, I only start updating if there are observers:
if (changed & ViewState::IsVisible && !m_observers.isEmpty())

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:147
> +    if (changed & ViewState::IsVisible && !m_observers.isEmpty())

... see '&& !m_observers.isEmpty()' check here.

>> LayoutTests/fast/dom/Geolocation/startUpdatingOnlyWhenPageVisible.html:26
>> +        shouldBeFalse("internals.isGeolocationClientUpdating()");
> 
> IMHO, that is the wrong way to test.
> 
> You can test this from end to end, including any bug in the WebKit layer. Instead of observing if the controller is updating, you should observe if the Mock object is updating or not.
> If you do that, the only source of bug should be the Provider's implementation.

I did it this way for several reasons:
1. Doing it end to end means we need to add a Geolocation client virtual function and provide implementations in subclasses, only for the purpose of testing. Also, in the case of testing, we are using the Mock client and adding better coverage for this mock client is not an objective I think
2. Some clients (such as the Mock client) will crash if startUpdating() or stopUpdating() is called several times in a row. Therefore, we would anyway need to keep a boolean on the GeolocationController to know the "isUpdating" state to make sure we call startUpdating() / stopUpdating() only when necessary.

[Benjamin Poulain]

Haha, crap, I was too eager to r- :)

You spread the states in too many places. What about having a single function that updates both the update state and high accuracy of the client based on the current internal state?

I have to admit I did not understand your arguments against testing end-to-end... :/

[Chris Dumez]

Created attachment 258370 [details]
Patch

[Chris Dumez]

The tests are not checking that the geolocation *provider* is active. I moved the isGeolocationProviderActive() test function from internals to testRunner.

[Benjamin Poulain]

Comment on attachment 258370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258370&action=review

> Tools/ChangeLog:8
> +        Add testRunner.isGeolocationProvider() test support function.

isGeolocationProvider-> isGeolocationProviderActive

> LayoutTests/fast/dom/Geolocation/startUpdatingOnlyWhenPageVisible.html:29
> +        setTimeout(checkNotUpdatingAndShowPage, 150);

150 is a bit on the slow side.

[Chris Dumez]

Created attachment 258401 [details]
Patch

[Chris Dumez]

rdar://problem/22178457

[WebKit Commit Bot]

Comment on attachment 258401 [details]
Patch

Clearing flags on attachment: 258401

Committed r188068: <http://trac.webkit.org/changeset/188068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.