Bug 134989 - Don't send geolocation permission requests when the page is not visible
Summary: Don't send geolocation permission requests when the page is not visible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on: 135021
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-16 12:44 PDT by Sam Weinig
Modified: 2014-07-17 14:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (14.49 KB, patch)
2014-07-16 12:49 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (14.60 KB, patch)
2014-07-16 16:00 PDT, Sam Weinig
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (594.84 KB, application/zip)
2014-07-16 18:11 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2014-07-16 12:44:07 PDT
Don't send geolocation permission requests when the page is not visible
Comment 1 Sam Weinig 2014-07-16 12:49:08 PDT
Created attachment 235017 [details]
Patch
Comment 2 mitz 2014-07-16 12:53:47 PDT
Comment on attachment 235017 [details]
Patch

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

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
> +    if (!m_page->isVisible()) {
> +        m_pendedPermissionRequest.remove(geolocation);
> +        return;
> +    }

What if the request was made while the page was visible, but canceled while it was not, or vice versa?

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:150
> +    m_pendedPermissionRequest.clear();

It might be safer to do this before we start calling the client, just in case the client manages to call back into something that adds to the vector.
Comment 3 Gavin Barraclough 2014-07-16 12:59:00 PDT
Comment on attachment 235017 [details]
Patch

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

r+ with comments

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:48
> +    // NOTE: We don't have to remove ourselves 

Incomplete comment? "… from the Page's list of ViewStateChangeObservers"?

> Source/WebCore/Modules/geolocation/GeolocationController.h:68
> +    Page* m_page;

Should this be Page* or Page&?

> Source/WebCore/Modules/geolocation/GeolocationController.h:81
> +    // While in the "prerender" visibility state, we pend permission requests.

Comment is now incorrect, "While page is hidden" / "While page is not visible"
Comment 4 Sam Weinig 2014-07-16 16:00:47 PDT
Created attachment 235031 [details]
Patch
Comment 5 Sam Weinig 2014-07-16 16:01:44 PDT
(In reply to comment #2)
> (From update of attachment 235017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235017&action=review
> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
> > +    if (!m_page->isVisible()) {
> > +        m_pendedPermissionRequest.remove(geolocation);
> > +        return;
> > +    }
> 
> What if the request was made while the page was visible, but canceled while it was not, or vice versa?

Great catch.  I now remove unconditionally, and if it the object was in the set, we don't call the client.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:150
> > +    m_pendedPermissionRequest.clear();
> 
> It might be safer to do this before we start calling the client, just in case the client manages to call back into something that adds to the vector.

Fixed.
Comment 6 Sam Weinig 2014-07-16 16:02:30 PDT
(In reply to comment #3)
> (From update of attachment 235017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235017&action=review
> 
> r+ with comments
> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:48
> > +    // NOTE: We don't have to remove ourselves 
> 
> Incomplete comment? "… from the Page's list of ViewStateChangeObservers"?

Fixed.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.h:68
> > +    Page* m_page;
> 
> Should this be Page* or Page&?

Probably, probably the Client should be too.  I will fix both of those issues in followup cleanup. 

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.h:81
> > +    // While in the "prerender" visibility state, we pend permission requests.
> 
> Comment is now incorrect, "While page is hidden" / "While page is not visible"

Fixed.
Comment 7 Build Bot 2014-07-16 18:11:28 PDT
Comment on attachment 235031 [details]
Patch

Attachment 235031 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6430366457921536

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html
Comment 8 Build Bot 2014-07-16 18:11:32 PDT
Created attachment 235040 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Darin Adler 2014-07-16 18:12:48 PDT
Comment on attachment 235031 [details]
Patch

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

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:42
> +GeolocationController::GeolocationController(Page* page, GeolocationClient* client)
> +    : m_page(page)
> +    , m_client(client)
>  {
> +    m_page->addViewStateChangeObserver(this);
>  }

Looks like this should take a Page& since it dereferences the pointer passed to it.

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
> +    if (m_pendedPermissionRequest.remove(geolocation))
> +        return;

Are we guaranteed that if something is in the pended set it’s not also out to the client as an actual permission request from when the page was visible?

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:140
> +    if (m_pendedPermissionRequest.isEmpty())
> +        return;

Why do we need this? Is it an important optimization?

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:147
> +    Vector<RefPtr<Geolocation>> pendedPermissionRequestVector;
> +    copyToVector(m_pendedPermissionRequest, pendedPermissionRequestVector);
> +    m_pendedPermissionRequest.clear();

If we’re going to clear it, why not move it into a local set instead of copying it into a local vector?

> Source/WebCore/Modules/geolocation/GeolocationController.h:45
> +class GeolocationController : public Supplement<Page>, ViewStateChangeObserver {

Should be explicit about ViewStateChangeObserver being private inheritance rather than just omitting public.

> Source/WebCore/Modules/geolocation/GeolocationController.h:68
> +    Page* m_page;

Should be Page& since it’s never null and never given a new value.

> Source/WebCore/Modules/geolocation/GeolocationController.h:72
> +    // ViewStateChangeObserver
> +    void viewStateDidChange(ViewState::Flags oldViewState, ViewState::Flags newViewState) override;

Should be marked virtual. Not a big fan of these class name comments for overrides.

> Source/WebCore/page/Page.h:324
> +    void addViewStateChangeObserver(ViewStateChangeObserver*);
> +    void removeViewStateChangeObserver(ViewStateChangeObserver*);

These should take references instead of pointers.
Comment 10 Sam Weinig 2014-07-16 23:03:21 PDT
(In reply to comment #9)
> (From update of attachment 235031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235031&action=review
> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:42
> > +GeolocationController::GeolocationController(Page* page, GeolocationClient* client)
> > +    : m_page(page)
> > +    , m_client(client)
> >  {
> > +    m_page->addViewStateChangeObserver(this);
> >  }
> 
> Looks like this should take a Page& since it dereferences the pointer passed to it.

Done.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
> > +    if (m_pendedPermissionRequest.remove(geolocation))
> > +        return;
> 
> Are we guaranteed that if something is in the pended set it’s not also out to the client as an actual permission request from when the page was visible?

Yes, due to the Geolocation objects only sending one request out.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:140
> > +    if (m_pendedPermissionRequest.isEmpty())
> > +        return;
> 
> Why do we need this? Is it an important optimization?

Removed.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:147
> > +    Vector<RefPtr<Geolocation>> pendedPermissionRequestVector;
> > +    copyToVector(m_pendedPermissionRequest, pendedPermissionRequestVector);
> > +    m_pendedPermissionRequest.clear();
> 
> If we’re going to clear it, why not move it into a local set instead of copying it into a local vector?

Done.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.h:45
> > +class GeolocationController : public Supplement<Page>, ViewStateChangeObserver {
> 
> Should be explicit about ViewStateChangeObserver being private inheritance rather than just omitting public.

Done.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.h:68
> > +    Page* m_page;
> 
> Should be Page& since it’s never null and never given a new value.

Done. Though there are so many others like it I would like to clean up in here as well that I am being strong and not doing.

> 
> > Source/WebCore/Modules/geolocation/GeolocationController.h:72
> > +    // ViewStateChangeObserver
> > +    void viewStateDidChange(ViewState::Flags oldViewState, ViewState::Flags newViewState) override;
> 
> Should be marked virtual. Not a big fan of these class name comments for overrides.

Done and done.

> 
> > Source/WebCore/page/Page.h:324
> > +    void addViewStateChangeObserver(ViewStateChangeObserver*);
> > +    void removeViewStateChangeObserver(ViewStateChangeObserver*);
> 
> These should take references instead of pointers.

Done.
Comment 11 Sam Weinig 2014-07-17 10:35:54 PDT
Committed r171188: <http://trac.webkit.org/changeset/171188>
Comment 12 WebKit Commit Bot 2014-07-17 13:35:50 PDT
Re-opened since this is blocked by bug 135021
Comment 13 Sam Weinig 2014-07-17 14:38:59 PDT
Additional fix landed in r171199.