Bug 147685 - Toggle GPS state based on page visibility to save battery
Summary: Toggle GPS state based on page visibility to save battery
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-05 09:46 PDT by Chris Dumez
Modified: 2015-08-06 15:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.86 KB, patch)
2015-08-05 13:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (23.67 KB, patch)
2015-08-06 09:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (23.69 KB, patch)
2015-08-06 14:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-08-05 09:46:56 PDT
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.
Comment 1 Chris Dumez 2015-08-05 13:04:13 PDT
Created attachment 258293 [details]
Patch
Comment 2 Benjamin Poulain 2015-08-05 17:38:38 PDT
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.
Comment 3 Chris Dumez 2015-08-05 18:50:14 PDT
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.
Comment 4 Benjamin Poulain 2015-08-05 20:55:14 PDT
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... :/
Comment 5 Chris Dumez 2015-08-06 09:52:51 PDT
Created attachment 258370 [details]
Patch
Comment 6 Chris Dumez 2015-08-06 09:53:58 PDT
The tests are not checking that the geolocation *provider* is active. I moved the isGeolocationProviderActive() test function from internals to testRunner.
Comment 7 Benjamin Poulain 2015-08-06 14:28:35 PDT
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.
Comment 8 Chris Dumez 2015-08-06 14:45:18 PDT
Created attachment 258401 [details]
Patch
Comment 9 Chris Dumez 2015-08-06 15:10:29 PDT
rdar://problem/22178457
Comment 10 WebKit Commit Bot 2015-08-06 15:38:01 PDT
Comment on attachment 258401 [details]
Patch

Clearing flags on attachment: 258401

Committed r188068: <http://trac.webkit.org/changeset/188068>
Comment 11 WebKit Commit Bot 2015-08-06 15:38:06 PDT
All reviewed patches have been landed.  Closing bug.