Don't send geolocation permission requests when the page is not visible
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.
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"
Created attachment 235031 [details] Patch
(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.
(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 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
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 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.
(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.
Committed r171188: <http://trac.webkit.org/changeset/171188>
Re-opened since this is blocked by bug 135021
Additional fix landed in r171199.