Bug 74499 – GeolocationClient should have a setController() method.

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Alexander Færøy]

The current implementation of GeolocationClient does not have a setController() method, which means that the clients that inherit from GeolocationClient passes the WebPage from which they can get the geolocationController()->client() from. It would be nicer if the GeolocationController called m_client->setController(this) upon construction such that keeping a pointer to the WebPage becomes unnecessary. 

Patch coming up.

[Alexander Færøy]

Created attachment 119201 [details]
Patch

Initial patch. This is just to see what the EWS says.

[Gustavo Noronha (kov)]

Comment on attachment 119201 [details]
Patch

Attachment 119201 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10872239

[Alexander Færøy]

Created attachment 119204 [details]
Patch

Take two, lets see what EWS says.

[Gustavo Noronha (kov)]

Comment on attachment 119204 [details]
Patch

Attachment 119204 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10870257

[Gustavo Noronha (kov)]

Comment on attachment 119204 [details]
Patch

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

I'm not sure this change is a good one. It's common to keep the page as a member of the clients. I also wonder if it's possible that you might end up holding a different controller than what the page is associated with in some cases. Also, it doesn't seem to gain much in terms of code simplification. Would you mind arguing the case for going through with this change?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:46
> +    void setController(GeolocationController*);
> +

The GTK+ build fails because this class is inside the WebCore namespace and there's no using namespace WebCore in this file, so you have to prefix it.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> (From update of attachment 119204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119204&action=review
> 
> I'm not sure this change is a good one. It's common to keep the page as a member of the clients. I also wonder if it's possible that you might end up holding a different controller than what the page is associated with in some cases. Also, it doesn't seem to gain much in terms of code simplification. Would you mind arguing the case for going through with this change?

It makes a lot of sense if you want to reuse your implementation across webkit1 and 2. This is also already how it is done for the DeviceOrientation/Motion clients, and for the Chromium geolocation client if I am not wrong

[Alexander Færøy]

(In reply to comment #5)
> I'm not sure this change is a good one. It's common to keep the page as a member of the clients. I also wonder if it's possible that you might end up holding a different controller than what the page is associated with in some cases. Also, it doesn't seem to gain much in terms of code simplification. Would you mind arguing the case for going through with this change?

The patch is made to unify this client such that it works more like the DeviceOrientation clients.


> The GTK+ build fails because this class is inside the WebCore namespace and there's no using namespace WebCore in this file, so you have to prefix it.

Will fix and upload a new version of the patch.

[Alexander Færøy]

Created attachment 119209 [details]
Patch

[Gustavo Noronha (kov)]

I see, thanks for the clarification, and there you go, gtk is green =)

[WebKit Review Bot]

Comment on attachment 119209 [details]
Patch

Attachment 119209 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10830248

New failing tests:
fast/media/media-svg-crash.html
css2.1/20110323/background-intrinsic-007.htm
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
fast/backgrounds/animated-svg-as-mask.html
fast/forms/number/spin-in-multi-column.html
http/tests/security/canvas-remote-read-svg-image.html
compositing/images/direct-svg-image.html
css2.1/20110323/background-intrinsic-009.htm
css3/images/cross-fade-overflow-position.html
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
css2.1/20110323/background-intrinsic-005.htm
http/tests/security/canvas-remote-read-data-url-svg-image.html
fast/canvas/svg-taint.html
css2.1/20110323/background-intrinsic-003.htm

[Alexander Færøy]

Created attachment 119216 [details]
Patch

Unbreak Windows build.

[Kenneth Rohde Christiansen]

Comment on attachment 119216 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        GeolocationClient should have a setController() method.
> +        https://bugs.webkit.org/show_bug.cgi?id=74499
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Already covered by existing tests.
> +

You forgot to tell why this change makes sense, what is the reasoning behind it.

[Alexander Færøy]

Created attachment 119612 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119612 [details]
Patch

Attachment 119612 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10929006

New failing tests:
fast/media/media-svg-crash.html
http/tests/inspector-enabled/dedicated-workers-list.html
css2.1/20110323/background-intrinsic-007.htm
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
fast/backgrounds/animated-svg-as-mask.html
css2.1/20110323/background-intrinsic-008.htm
http/tests/security/canvas-remote-read-svg-image.html
compositing/images/direct-svg-image.html
css2.1/20110323/background-intrinsic-009.htm
http/tests/inspector/network-preflight-options.html
css2.1/20110323/background-intrinsic-002.htm
css3/images/cross-fade-overflow-position.html
css2.1/20110323/background-intrinsic-005.htm
http/tests/security/canvas-remote-read-data-url-svg-image.html
css2.1/20110323/background-intrinsic-001.htm
fast/canvas/svg-taint.html
css2.1/20110323/background-intrinsic-003.htm

[Simon Hausmann]

Comment on attachment 119612 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.cpp:47
> +void WebGeolocationClient::setController(GeolocationController*)
> +{
> +}

I think the emptyness of this method deserves a comment (i.e. the fact that the update is propagated differently)

[Alexander Færøy]

After a short discussion on IRC, Simon and I decided not to continue with this patch, since it will be easier to just do a clean reimplementation of this client for WK2.

[Eric Seidel (no email)]

Comment on attachment 119612 [details]
Patch

Cleared review? from attachment 119612 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).