Bug 134989 – Don't send geolocation permission requests when the page is not visible

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Don't send geolocation permission requests when the page is not visible

[Sam Weinig]

Created attachment 235017 [details]
Patch

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.

[Gavin Barraclough]

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"

[Sam Weinig]

Created attachment 235031 [details]
Patch

[Sam Weinig]

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

[Sam Weinig]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Sam Weinig]

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

[Sam Weinig]

Committed r171188: <http://trac.webkit.org/changeset/171188>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 135021

[Sam Weinig]

Additional fix landed in r171199.