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.
Created attachment 119201 [details] Patch Initial patch. This is just to see what the EWS says.
Comment on attachment 119201 [details] Patch Attachment 119201 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10872239
Created attachment 119204 [details] Patch Take two, lets see what EWS says.
Comment on attachment 119204 [details] Patch Attachment 119204 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10870257
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.
(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
(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.
Created attachment 119209 [details] Patch
I see, thanks for the clarification, and there you go, gtk is green =)
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
Created attachment 119216 [details] Patch Unbreak Windows build.
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.
Created attachment 119612 [details] Patch
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 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)
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 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).