Bug 46895 – [Chromium] Implement mocks for client-based geolocation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Knottenbelt]

As part of Bug 45752, implement the mocks so that the test suite can exercise the client-based geolocation code.

[Steve Block]

Note that the Mac port (currently the only user of client-based Geolocation) already has such a mock. It would be great if you could make that mock common code to be shared by all platforms. This is the approach we use for the DeviceOrientation and Speech (see WebCore/platform/mock).

[John Knottenbelt]

Created attachment 74563 [details]
Patch

[John Knottenbelt]

From https://bugs.webkit.org/attachment.cgi?id=74382&action=review, Darin Fisher asked:

> what prevents the GeolocationController object from being destroyed before the WebGeolocationController?
> how do you avoid memory errors in that case?
> 
> this seems error prone.

Thanks for reviewing so quickly.

The destructor of WebCore::GeolocationController calls WebCore::GeolocationClient::geolocationDestroyed() on it's client. This client interface will be implemented by a class called GeolocationClientProxy which will in turn call it's client's geolocationDestroyed() method - WebKit::WebGeolocationClient::geolocationDestroyed(). 

Implementers of the client should be aware that after geolocationDestroyed() has been invoked the WebGeolocationController pointer is invalid (and could set their WebGeolocationController member to 0 on receiving geolocationDestroyed()).

[WebKit Review Bot]

Attachment 74563 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6277053

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> The destructor of WebCore::GeolocationController calls WebCore::GeolocationClient::geolocationDestroyed() on it's client. This client interface will be implemented by a class called GeolocationClientProxy which will in turn call it's client's geolocationDestroyed() method - WebKit::WebGeolocationClient::geolocationDestroyed(). 
> 
> Implementers of the client should be aware that after geolocationDestroyed() has been invoked the WebGeolocationController pointer is invalid (and could set their WebGeolocationController member to 0 on receiving geolocationDestroyed()).

OK, please make sure this is well documented.  I don't see this spelled out in the code.

[John Knottenbelt]

Created attachment 74850 [details]
Patch

[Steve Block]

Comment on attachment 74850 [details]
Patch

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

> WebKit/chromium/public/WebGeolocationClientMock.h:51
> +    WEBKIT_API void setMockGeolocationPosition(double latitude, double longitude, double accuracy);

Do you need 'MockGeolocation' in these method names?

> WebKit/chromium/src/WebGeolocationClientMock.cpp:53
> +void WebGeolocationClientMock::initialize()

Why do you need these initialize() and reset() methods? Doesn't the OwnPtr take care of deleting the WebCore mock when this object goes away?

> WebKit/chromium/src/WebGeolocationClientMock.cpp:72
> +    WebGeolocationError::Error code = WebGeolocationError::ErrorPositionUnavailable;

Is there a particular reason for defaulting to this code?

> WebKit/chromium/src/WebGeolocationClientMock.cpp:80
> +    }

what about TIMEOUT?

> WebKit/chromium/src/WebGeolocationClientMock.cpp:118
> +    m_clientMock->setController(controller.controller());

Is this the standard Chromium syntax for getting the WebCore type?

> WebKit/chromium/src/WebGeolocationClientMock.cpp:127
> +    webPosition = PassRefPtr<WebCore::GeolocationPosition>(position);

You shouldn't instantiate locals of type PassRefPtr. Create a local RefPtr and call release() or get() depending on whether you're passing ownership or passing a raw pointer.

> WebKit/chromium/src/WebGeolocationClientMock.cpp:133
> +    m_clientMock->requestPermission(PassRefPtr<WebCore::Geolocation>(request).get());

Again, this shouldn't be a PassRefPtr.

> WebKit/chromium/src/WebGeolocationServiceMock.cpp:-47
> -// move to another class and remove WebGeolocationService*.

Were these methods being called from anywhere?

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:586
> +    if (!m_webGeolocationClientMock.get())

Don't need .get() for boolean test

[John Knottenbelt]

Comment on attachment 74850 [details]
Patch

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

Thanks for a review. Patch is forthcoming. There are some points to do with PassRefPtr that I hope to get clarification on.

>> WebKit/chromium/public/WebGeolocationClientMock.h:51
>> +    WEBKIT_API void setMockGeolocationPosition(double latitude, double longitude, double accuracy);
> 
> Do you need 'MockGeolocation' in these method names?

No, will remove. I think that perhaps they were called this way because of the name of the LayoutTestController method.

>> WebKit/chromium/src/WebGeolocationClientMock.cpp:53
>> +void WebGeolocationClientMock::initialize()
> 
> Why do you need these initialize() and reset() methods? Doesn't the OwnPtr take care of deleting the WebCore mock when this object goes away?

As I understand it, for the shared libraries to work correctly, anything in the implementation (src) file needs to be WEBKIT_API or WEBKIT_PRIVATE, otherwise it needs to be inline in the header. Since the constructor is for public use, it needs to be inline and call a WEBKIT_API method, initialize(). I believe same story with the destructor (see WebNode). Darin, please comment.

>> WebKit/chromium/src/WebGeolocationClientMock.cpp:72
>> +    WebGeolocationError::Error code = WebGeolocationError::ErrorPositionUnavailable;
> 
> Is there a particular reason for defaulting to this code?

An alternative would be not to set it, and add an ASSERT / NOTREACHED in the default case of the switch. Would this be preferable?

>> WebKit/chromium/src/WebGeolocationClientMock.cpp:80
>> +    }
> 
> what about TIMEOUT?

No tests call layoutTestController.setMockGeolocationError(3), which would be timeout. Furthermore, there is no corresponding error code in WebCore::GeolocationError for timeout (only PermissionDenied and PositionUnavailable).

>> WebKit/chromium/src/WebGeolocationClientMock.cpp:118
>> +    m_clientMock->setController(controller.controller());
> 
> Is this the standard Chromium syntax for getting the WebCore type?

In many cases the underlying webcore type is got at by casting to a PassRefPtr. However, in this case, the WebCore::GeolocationController is not ref-counted. I followed the pattern in WebDeviceOrientationController. Darin, please comment.

>> WebKit/chromium/src/WebGeolocationClientMock.cpp:127
>> +    webPosition = PassRefPtr<WebCore::GeolocationPosition>(position);
> 
> You shouldn't instantiate locals of type PassRefPtr. Create a local RefPtr and call release() or get() depending on whether you're passing ownership or passing a raw pointer.

This appears to be an exception to the normal rules, although I could be misunderstanding things. Searching for "PassRefPtr<Node>(" shows some examples where this is used to convert between WebKit and WebCore types. Darin, please comment.

>> WebKit/chromium/src/WebGeolocationClientMock.cpp:133
>> +    m_clientMock->requestPermission(PassRefPtr<WebCore::Geolocation>(request).get());
> 
> Again, this shouldn't be a PassRefPtr.

Will try to get clarification on this. Generally I agree with your statement about passing ownership, given WebKit's guidelines, but I don't understand why a lot of existing code that uses this technique would not also then be violating those guidelines.

>> WebKit/chromium/src/WebGeolocationServiceMock.cpp:-47
>> -// move to another class and remove WebGeolocationService*.
> 
> Were these methods being called from anywhere?

Yes, from LayoutTestController.

>> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:586
>> +    if (!m_webGeolocationClientMock.get())
> 
> Don't need .get() for boolean test

Ok.

[John Knottenbelt]

Created attachment 74915 [details]
Patch

[WebKit Review Bot]

Attachment 74915 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6407082

[Darin Fisher (:fishd, Google)]

Comment on attachment 74915 [details]
Patch

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

What is the master plan for ENABLE_CLIENT_BASED_GEOLOCATION?  Will we be
deleting the "else" code in the near future?

> WebKit/chromium/public/WebGeolocationClientMock.h:70
> +    WEBKIT_API void initialize();

nit: looks like 'initialize' does not need to be exported from webkit.dll since
it is only called by a private constructor.  i'd probably just get rid of the
initialize method, and move its implementation into the constructor.

> WebKit/chromium/src/WebGeolocationClientMock.cpp:54
> +void WebGeolocationClientMock::initialize()

nit: please implement the methods in the same order in which they are declared
in the header file.

> WebKitTools/DumpRenderTree/chromium/WebViewHost.h:321
> +    OwnPtr<WebKit::WebGeolocationClientMock> m_webGeolocationClientMock;

nit: for consumers of the WebKit API, it is conventional to drop the "web" prefix
on variable names.  that's why we have m_geolocationServiceMock and not
m_webGeolocationServiceMock.

[John Knottenbelt]

(In reply to comment #11)
> (From update of attachment 74915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74915&action=review
> 
> What is the master plan for ENABLE_CLIENT_BASED_GEOLOCATION?  Will we be
> deleting the "else" code in the near future?

The plan is eventually to remove the non-client based geolocation once all the ports have switched to using client based geolocation. The master bug is 
https://bugs.webkit.org/show_bug.cgi?id=40373 I'm not sure of the timescales for all the ports, but I hope to finish Chromium in the near feature.

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 74915 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74915&action=review
> > 
> > What is the master plan for ENABLE_CLIENT_BASED_GEOLOCATION?  Will we be
> > deleting the "else" code in the near future?
> 
> The plan is eventually to remove the non-client based geolocation once all the ports have switched to using client based geolocation. The master bug is 
> https://bugs.webkit.org/show_bug.cgi?id=40373 I'm not sure of the timescales for all the ports, but I hope to finish Chromium in the near feature.

OK, thanks for the info/link.

[John Knottenbelt]

Created attachment 75694 [details]
Patch

[WebKit Review Bot]

Attachment 75694 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6790061

[John Knottenbelt]

Created attachment 75813 [details]
Patch

[John Knottenbelt]

Updated the patch because WebGeolocationController has become a heap-only object.

(In reply to comment #16)
> Created an attachment (id=75813) [details]
> Patch

[WebKit Review Bot]

Attachment 75813 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75813 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75813 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75813 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75813 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6861054

[WebKit Review Bot]

Attachment 75813 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Steve Block]

Comment on attachment 75813 [details]
Patch

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

LGTM

> WebKit/chromium/src/WebGeolocationClientMock.cpp:63
> +    WebGeolocationError::Error code = WebGeolocationError::ErrorPositionUnavailable;

I'd prefer an assert to a default value for test code, but it's really just personal preference.

[John Knottenbelt]

Created attachment 75927 [details]
Patch

[WebKit Review Bot]

Attachment 75927 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6815125

[Steve Block]

Comment on attachment 75927 [details]
Patch

This looks ready to go. If you re-upload the patch once the fix for Bug 45752 is in place to check the try-bots, I can r+

[John Knottenbelt]

Created attachment 76198 [details]
Patch

[Steve Block]

Comment on attachment 76198 [details]
Patch

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

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:48
> +#else

Is this guard needed?

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:48
> +#else

Is this guard needed?

[John Knottenbelt]

Comment on attachment 76198 [details]
Patch

The guards will be coming out once we switch over to client-based geolocation in Chromium so, although we might be able to get away without them, they could be actually be useful as a reminder as to what depends on client geolocation and what doesn't.

[WebKit Review Bot]

Comment on attachment 76198 [details]
Patch

Rejecting attachment 76198 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 76198 [details]
Patch

Clearing flags on attachment: 76198

Committed r73745: <http://trac.webkit.org/changeset/73745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.