[webkit-reviews] review granted: [Bug 134989] Don't send geolocation permission requests when the page is not visible : [Attachment 235031] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 134989: Don't send geolocation permission requests when the page is not
visible
https://bugs.webkit.org/show_bug.cgi?id=134989

Attachment 235031: Patch
https://bugs.webkit.org/attachment.cgi?id=235031&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.