Bug 74499

Summary: GeolocationClient should have a setController() method.
Product: WebKit Reporter: Alexander Færøy <ahf>
Component: WebKit Misc.Assignee: Alexander Færøy <ahf>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dglazkov, gns, hausmann, kenneth, steveblock, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexander Færøy 2011-12-14 04:29:21 PST
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.
Comment 1 Alexander Færøy 2011-12-14 04:33:55 PST
Created attachment 119201 [details]
Patch

Initial patch. This is just to see what the EWS says.
Comment 2 Gustavo Noronha (kov) 2011-12-14 04:36:55 PST
Comment on attachment 119201 [details]
Patch

Attachment 119201 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10872239
Comment 3 Alexander Færøy 2011-12-14 05:03:37 PST
Created attachment 119204 [details]
Patch

Take two, lets see what EWS says.
Comment 4 Gustavo Noronha (kov) 2011-12-14 05:13:40 PST
Comment on attachment 119204 [details]
Patch

Attachment 119204 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10870257
Comment 5 Gustavo Noronha (kov) 2011-12-14 05:22:39 PST
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.
Comment 6 Kenneth Rohde Christiansen 2011-12-14 05:28:03 PST
(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
Comment 7 Alexander Færøy 2011-12-14 05:29:19 PST
(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.
Comment 8 Alexander Færøy 2011-12-14 05:31:20 PST
Created attachment 119209 [details]
Patch
Comment 9 Gustavo Noronha (kov) 2011-12-14 05:36:54 PST
I see, thanks for the clarification, and there you go, gtk is green =)
Comment 10 WebKit Review Bot 2011-12-14 05:56:04 PST
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
Comment 11 Alexander Færøy 2011-12-14 07:18:26 PST
Created attachment 119216 [details]
Patch

Unbreak Windows build.
Comment 12 Kenneth Rohde Christiansen 2011-12-14 13:11:36 PST
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.
Comment 13 Alexander Færøy 2011-12-16 08:01:26 PST
Created attachment 119612 [details]
Patch
Comment 14 WebKit Review Bot 2011-12-16 21:14:29 PST
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
Comment 15 Simon Hausmann 2011-12-19 04:35:47 PST
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)
Comment 16 Alexander Færøy 2011-12-19 04:55:41 PST
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.
Comment 17 Eric Seidel (no email) 2011-12-19 11:28:30 PST
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).