Bug 46895

Summary: [Chromium] Implement mocks for client-based geolocation
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, fishd, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 45752, 49258, 49735, 50061    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description John Knottenbelt 2010-09-30 06:46:17 PDT
As part of Bug 45752, implement the mocks so that the test suite can exercise the client-based geolocation code.
Comment 1 Steve Block 2010-10-01 04:59:47 PDT
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).
Comment 2 John Knottenbelt 2010-11-22 09:38:33 PST
Created attachment 74563 [details]
Patch
Comment 3 John Knottenbelt 2010-11-22 09:45:10 PST
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()).
Comment 4 WebKit Review Bot 2010-11-22 09:45:43 PST
Attachment 74563 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6277053
Comment 5 Darin Fisher (:fishd, Google) 2010-11-22 14:30:13 PST
(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.
Comment 6 John Knottenbelt 2010-11-25 03:52:13 PST
Created attachment 74850 [details]
Patch
Comment 7 Steve Block 2010-11-25 06:59:19 PST
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
Comment 8 John Knottenbelt 2010-11-25 09:16:44 PST
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.
Comment 9 John Knottenbelt 2010-11-26 04:03:14 PST
Created attachment 74915 [details]
Patch
Comment 10 WebKit Review Bot 2010-11-28 15:08:02 PST
Attachment 74915 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6407082
Comment 11 Darin Fisher (:fishd, Google) 2010-11-29 08:50:57 PST
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.
Comment 12 John Knottenbelt 2010-11-30 03:55:51 PST
(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.
Comment 13 Darin Fisher (:fishd, Google) 2010-11-30 09:57:16 PST
(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.
Comment 14 John Knottenbelt 2010-12-06 07:25:13 PST
Created attachment 75694 [details]
Patch
Comment 15 WebKit Review Bot 2010-12-06 07:30:40 PST
Attachment 75694 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6790061
Comment 16 John Knottenbelt 2010-12-07 07:09:48 PST
Created attachment 75813 [details]
Patch
Comment 17 John Knottenbelt 2010-12-07 07:11:17 PST
Updated the patch because WebGeolocationController has become a heap-only object.

(In reply to comment #16)
> Created an attachment (id=75813) [details]
> Patch
Comment 18 WebKit Review Bot 2010-12-07 09:08:34 PST
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.
Comment 19 WebKit Review Bot 2010-12-07 10:09:19 PST
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.
Comment 20 WebKit Review Bot 2010-12-07 11:10:28 PST
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.
Comment 21 WebKit Review Bot 2010-12-07 12:11:53 PST
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.
Comment 22 WebKit Review Bot 2010-12-07 12:20:08 PST
Attachment 75813 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6861054
Comment 23 WebKit Review Bot 2010-12-07 21:39:20 PST
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.
Comment 24 Steve Block 2010-12-08 09:38:49 PST
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.
Comment 25 John Knottenbelt 2010-12-08 10:32:23 PST
Created attachment 75927 [details]
Patch
Comment 26 WebKit Review Bot 2010-12-08 13:13:50 PST
Attachment 75927 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6815125
Comment 27 Steve Block 2010-12-10 06:59:22 PST
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+
Comment 28 John Knottenbelt 2010-12-10 07:37:33 PST
Created attachment 76198 [details]
Patch
Comment 29 Steve Block 2010-12-10 08:22:35 PST
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?
Comment 30 John Knottenbelt 2010-12-10 08:35:57 PST
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.
Comment 31 WebKit Review Bot 2010-12-10 08:38:11 PST
Comment on attachment 76198 [details]
Patch

Rejecting attachment 76198 [details] from commit-queue.

jknotten@chromium.org 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.
Comment 32 WebKit Commit Bot 2010-12-10 10:54:21 PST
Comment on attachment 76198 [details]
Patch

Clearing flags on attachment: 76198

Committed r73745: <http://trac.webkit.org/changeset/73745>
Comment 33 WebKit Commit Bot 2010-12-10 10:54:29 PST
All reviewed patches have been landed.  Closing bug.