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
John Knottenbelt
2010-09-30 06:46:17 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). Created attachment 74563 [details]
Patch
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()). Attachment 74563 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6277053 (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. Created attachment 74850 [details]
Patch
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 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. Created attachment 74915 [details]
Patch
Attachment 74915 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6407082 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. (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. (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. Created attachment 75694 [details]
Patch
Attachment 75694 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6790061 Created attachment 75813 [details]
Patch
Updated the patch because WebGeolocationController has become a heap-only object. (In reply to comment #16) > Created an attachment (id=75813) [details] > Patch 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.
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.
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.
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.
Attachment 75813 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6861054 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 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. Created attachment 75927 [details]
Patch
Attachment 75927 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6815125 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+ Created attachment 76198 [details]
Patch
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 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 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 on attachment 76198 [details] Patch Clearing flags on attachment: 76198 Committed r73745: <http://trac.webkit.org/changeset/73745> All reviewed patches have been landed. Closing bug. |