WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45752
[chromium] Implement client based geolocation bindings
https://bugs.webkit.org/show_bug.cgi?id=45752
Summary
[chromium] Implement client based geolocation bindings
Jonathan Dixon
Reported
2010-09-14 07:48:24 PDT
Implement Chromium port of client-based geolocation as non-client based (service based) geolocation may be deprecated as per
bug #40373
Attachments
Patch
(47.17 KB, patch)
2010-09-30 06:32 PDT
,
John Knottenbelt
no flags
Details
Formatted Diff
Diff
Patch
(47.20 KB, patch)
2010-09-30 06:40 PDT
,
John Knottenbelt
no flags
Details
Formatted Diff
Diff
Patch
(29.87 KB, patch)
2010-11-25 04:38 PST
,
John Knottenbelt
no flags
Details
Formatted Diff
Diff
Patch
(37.92 KB, patch)
2010-12-06 07:21 PST
,
John Knottenbelt
no flags
Details
Formatted Diff
Diff
Patch
(38.24 KB, patch)
2010-12-07 07:06 PST
,
John Knottenbelt
no flags
Details
Formatted Diff
Diff
Patch
(38.19 KB, patch)
2010-12-08 10:31 PST
,
John Knottenbelt
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
John Knottenbelt
Comment 1
2010-09-30 06:32:10 PDT
Created
attachment 69331
[details]
Patch
WebKit Review Bot
Comment 2
2010-09-30 06:34:34 PDT
Attachment 69331
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
John Knottenbelt
Comment 3
2010-09-30 06:40:34 PDT
Created
attachment 69332
[details]
Patch
Jonathan Dixon
Comment 4
2010-09-30 07:36:28 PDT
Comment on
attachment 69332
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=69332&action=review
Great! I defer to a WebKit reviewer for real review, but here's some initial thoughts
> WebKit/chromium/public/WebGeolocationClient.h:44 > + // is published.
Not convinced we need to duplicate this fixme right through the stack, the one in GeolocationControllerClient probably covers it. If the property that is visible to JS changes, there will be a bug to track this anyway.
> WebKit/chromium/public/WebGeolocationController.h:31 > +namespace WebCore { class GeolocationController; }
make #if WEBKIT_IMPLEMENTATION consistent
> WebKit/chromium/public/WebGeolocationError.h:31 > +#if WEBKIT_IMPLEMENTATION
make #if WEBKIT_IMPLEMENTATION consistent
> WebKit/chromium/public/WebGeolocationPermissionRequest.h:40 > + WebGeolocationPermissionRequest(WebCore::Geolocation* geolocation)
forward declare WebCore::Geolocation make #if WEBKIT_IMPLEMENTATION consistent
> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1501 > + // TODO(jknotten): Implement setMockGeolocationPosition client-based
use FIXME:
WebKit Review Bot
Comment 5
2010-09-30 08:35:54 PDT
Attachment 69332
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/4231030
Steve Block
Comment 6
2010-10-01 06:00:26 PDT
Comment on
attachment 69332
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=69332&action=review
> WebCore/ChangeLog:9 > + is enabled.
Need to describe how this is tested. I think the answer is 'not yet - awaiting mock - see
bug 46895
'
> WebCore/WebCore.gyp/WebCore.gyp:1207 > + ['exclude', '/GeolocationService.*$'],
It might be be better to split this large patch up - you could have a preliminary patch to add the ENABLE_CLIENT_BASED_GEOLOCATION guards for the Service in the existing code/makefiles. Many small patches are generally better than one large one.
> WebKit/chromium/public/WebGeolocationClient.h:32 > +class WebGeolocationController;
Are these needed?
> WebKit/chromium/public/WebGeolocationClient.h:34 > +class WebGeolocationClient {
I think it's best to be consistent with naming and call this WebGeolocationControllerClient. I know the WebCore name doesn't follow the common pattern - if you'd like to fix it, go ahead!
> WebKit/chromium/public/WebGeolocationController.h:46 > + WEBKIT_API void errorOccurred(const WebGeolocationError&);
Why do you pass these by ref, rather than as a pointer? Passing by ref introduces the need for the 'isNull' logic, which we could achieve by just looking at the pointer.
> WebKit/chromium/public/WebGeolocationError.h:33 > +namespace WTF { template <typename T> class PassRefPtr; }
I'm not sure I've seen this single-line style for a namespace before in WebKit?
> WebKit/chromium/src/GeolocationControllerClientProxy.cpp:35 > + if (m_client)
When would the client be null? If this will should happen, maybe it's best to assert in the constructor then not check here.
> WebKit/chromium/src/WebGeolocationController.cpp:41 > + PassRefPtr<WebCore::GeolocationPosition> position(p);
You shouldn't use PassRefPtr for locals - use RefPtr instead. It's also misleading, as you're not passing ownership anywhere.
> WebKit/chromium/src/WebGeolocationController.cpp:42 > + m_controller->positionChanged(position.get());
Just to check we're on the same page - The (Pass)RefPtr will go out of scope here. That's OK because the WebCore controller takes it's own ref for the last position.
> WebKitTools/ChangeLog:8 > + Comment out non-client-based mock implementations for now.
Maybe 'disable mock Geolocation bindings for client-based implementation' is more clear?
> WebKitTools/DumpRenderTree/chromium/WebViewHost.h:133 > +#if !ENABLE(CLIENT_BASED_GEOLOCATION)
Is your plan to guard everything that's specific to the non-client-based approach, or just those that need to be guarded to keep everything compiling?
David Kilzer (:ddkilzer)
Comment 7
2010-10-24 08:36:57 PDT
Comment on
attachment 69332
[details]
Patch r- based on
Comment #6
.
John Knottenbelt
Comment 8
2010-11-25 04:38:03 PST
Created
attachment 74857
[details]
Patch
Steve Block
Comment 9
2010-11-25 06:37:15 PST
Comment on
attachment 74857
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=74857&action=review
> WebKit/chromium/public/WebGeolocationClient.h:29 > +namespace WebKit {
blank line after namespace?
> WebKit/chromium/public/WebGeolocationController.h:45 > + }
Is it OK to put impl in these public header files?
> WebKit/chromium/public/WebGeolocationController.h:56 > + operator bool() const;
Why is this needed?
> WebKit/chromium/public/WebGeolocationController.h:60 > + WebCore::GeolocationController* m_controller;
in WebGeolocationPermissionRequest you use m_private. Should probably be consistent.
> WebKit/chromium/public/WebGeolocationPermissionRequest.h:46 > + WEBKIT_API void reset();
Why is this required?
> WebKit/chromium/public/WebGeolocationPermissionRequest.h:49 > + WebGeolocationPermissionRequest(WTF::PassRefPtr<WebCore::Geolocation>);
This shouldn't take a PassRefPtr - we're not transferring ownership. The WebCore API passes a raw pointer and doesn't expect the client to add a ref.
> WebKit/chromium/public/WebGeolocationPermissionRequest.h:50 > + operator WTF::PassRefPtr<WebCore::Geolocation>() const;
Why is this required?
> WebKit/chromium/src/ChromeClientImpl.cpp:778 > +#if !ENABLE(CLIENT_BASED_GEOLOCATION)
May have to remove these - see my comment in
Bug 50061
> WebKit/chromium/src/GeolocationClientProxy.h:33 > +namespace WebCore { class GeolocationPosition; }
not sure about this single line style for namespaces?
> WebKit/chromium/src/GeolocationClientProxy.h:46 > + // FIXME: The V2 Geolocation specification proposes that this property is
As joth commented, probably no need to propagate comment this up the stack
> WebKit/chromium/src/WebGeolocationController.cpp:42 > + m_controller->positionChanged(PassRefPtr<WebCore::GeolocationPosition>(*webPosition).get());
You shouldn't be using a PassRefPtr here. The WebCore API takes a raw pointer. It looks like you should be creating a local RefPtr, then passing the result of its get() method. The WebCore::GeolocationController can then take its own ref if it wants to hold onto the object.
> WebKit/chromium/src/WebGeolocationPermissionRequest.cpp:47 > + m_private->setIsAllowed(true);
true ?!!
> WebKit/chromium/src/WebViewImpl.cpp:314 > + , m_geolocationClientProxy(new GeolocationClientProxy(client->geolocationClient()))
Do you need to test for a null client?
John Knottenbelt
Comment 10
2010-11-25 08:50:11 PST
Comment on
attachment 74857
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=74857&action=review
Thanks for the review. Patch forthcoming.
>> WebKit/chromium/public/WebGeolocationClient.h:29 >> +namespace WebKit { > > blank line after namespace?
Added.
>> WebKit/chromium/public/WebGeolocationController.h:45 >> + } > > Is it OK to put impl in these public header files?
There are other examples of implementation in the public header files, for example WebNode. Darin, please comment.
>> WebKit/chromium/public/WebGeolocationController.h:56 >> + operator bool() const; > > Why is this needed?
We can get rid of the operator bool(). It used to be useful :)
>> WebKit/chromium/public/WebGeolocationPermissionRequest.h:46 >> + WEBKIT_API void reset(); > > Why is this required?
We are required to manually reset() the WebPrivatePtr (m_private) in destructor. Followed the pattern from WebNode. Would it be acceptable to simply call m_private.reset() in the destructor? Darin, please comment.
>> WebKit/chromium/public/WebGeolocationPermissionRequest.h:49 >> + WebGeolocationPermissionRequest(WTF::PassRefPtr<WebCore::Geolocation>); > > This shouldn't take a PassRefPtr - we're not transferring ownership. The WebCore API passes a raw pointer and doesn't expect the client to add a ref.
Agree.
>> WebKit/chromium/public/WebGeolocationPermissionRequest.h:50 >> + operator WTF::PassRefPtr<WebCore::Geolocation>() const; > > Why is this required?
This is used in the WebGeolocationClientmock to extract the Geolocation object to pass into the WebCore::GeolocationClientMock. Since the PassRefPtr is not required, I propose to rename this method to WebCore::Geolocation *geolocation() const.
>> WebKit/chromium/src/ChromeClientImpl.cpp:778 >> +#if !ENABLE(CLIENT_BASED_GEOLOCATION) > > May have to remove these - see my comment in
Bug 50061
Yes, I'll move the condition compilation directives into the method definitions and add a FIXME to remove after non-client-based geolocation is deprecated.
>> WebKit/chromium/src/GeolocationClientProxy.h:33 >> +namespace WebCore { class GeolocationPosition; } > > not sure about this single line style for namespaces?
Changed to multiple line style. There are existing examples of both styles, is one more accepted than the other?
>> WebKit/chromium/src/GeolocationClientProxy.h:46 >> + // FIXME: The V2 Geolocation specification proposes that this property is > > As joth commented, probably no need to propagate comment this up the stack
Agree.
>> WebKit/chromium/src/WebGeolocationController.cpp:42 >> + m_controller->positionChanged(PassRefPtr<WebCore::GeolocationPosition>(*webPosition).get()); > > You shouldn't be using a PassRefPtr here. The WebCore API takes a raw pointer. It looks like you should be creating a local RefPtr, then passing the result of its get() method. The WebCore::GeolocationController can then take its own ref if it wants to hold onto the object.
Agree.
>> WebKit/chromium/src/WebViewImpl.cpp:314 >> + , m_geolocationClientProxy(new GeolocationClientProxy(client->geolocationClient())) > > Do you need to test for a null client?
Looks like I do:
https://bugs.webkit.org/show_bug.cgi?id=43240
Darin Fisher (:fishd, Google)
Comment 11
2010-11-29 09:00:17 PST
Comment on
attachment 74857
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=74857&action=review
>>> WebKit/chromium/public/WebGeolocationController.h:45 >>> + } >> >> Is it OK to put impl in these public header files? > > There are other examples of implementation in the public header files, for example WebNode. Darin, please comment.
Nulling out a pointer like this is OK. It doesn't introduce any linker dependencies on WebCore.
>>> WebKit/chromium/public/WebGeolocationPermissionRequest.h:46 >>> + WEBKIT_API void reset(); >> >> Why is this required? > > We are required to manually reset() the WebPrivatePtr (m_private) in destructor. Followed the pattern from WebNode. Would it be acceptable to simply call m_private.reset() in the destructor? Darin, please comment.
WebPrivatePtr::reset() is not marked WEBKIT_API, so you cannot call it from a public method in a header file. If you did, then you would break the WEBKIT_DLL build. We do not mark WebPrivatePtr::reset() with WEBKIT_API since it is 1) protected by WEBKIT_IMPLEMENTATION and 2) has linker dependencies on WTF::RefCounted<T>::{de}ref, which are not exported from the webkit DLL.
John Knottenbelt
Comment 12
2010-12-06 07:21:53 PST
Created
attachment 75692
[details]
Patch
Jonathan Dixon
Comment 13
2010-12-07 02:42:18 PST
Comment on
attachment 75692
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=75692&action=review
> WebKit/chromium/public/WebGeolocationController.h:45 > + }
Maybe the usage of this would be clearer if we turned it into a identity class? i.e. ban copy & assign (and the default c'tor), remove the reset() method, have them created on the heap and the guy that vends them will transfer ownership rather than expect the receiver to make a byte copy of it. This will give a little more heap churn, but is clearer IMO. Alternatively we could leave the design as is but rename it to FooControllerHandle or something, but that feels a little obscure.
John Knottenbelt
Comment 14
2010-12-07 07:06:55 PST
Created
attachment 75812
[details]
Patch
WebKit Review Bot
Comment 15
2010-12-07 08:51:18 PST
Attachment 75812
[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
Comment 16
2010-12-07 09:52:30 PST
Attachment 75812
[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
Comment 17
2010-12-07 10:53:43 PST
Attachment 75812
[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
Comment 18
2010-12-07 11:54:54 PST
Attachment 75812
[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
Comment 19
2010-12-07 21:35:39 PST
Attachment 75812
[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 20
2010-12-08 09:22:06 PST
Comment on
attachment 75812
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=75812&action=review
> WebKit/chromium/public/WebGeolocationClient.h:29 > +namespace WebKit {
blank line
> WebKit/chromium/public/WebGeolocationController.h:57 > + WebCore::GeolocationController* m_controller;
In WebGeolocationPermissionRequest you call this m_private.
> WebKit/chromium/public/WebGeolocationPermissionRequest.h:58 > + }
I think correct style is to put this all on one line
> WebKit/chromium/public/WebGeolocationPermissionRequestContainer.h:37 > +class WebGeolocationPermissionRequestContainer : public WebNonCopyable {
Is WebGeolocationPermissionRequestManager/Tracker a better name than container? Container sounds like it wraps one request. Also, this class probably warrants a comment about how it's used.
> WebKit/chromium/src/GeolocationClientProxy.cpp:49 > + // There are tests that create a page with a null WebViewClient. Those tests won't
I don't think there's any need to mention that it's tests that don't pass a client - this may change. Just say that we support there not being a client, providing we don't do any Geolocation.
> WebKit/chromium/src/WebGeolocationController.cpp:50 > + RefPtr<GeolocationError> error = PassRefPtr<GeolocationError>(webError);
Put on one line to match other uses.
> WebKit/chromium/src/WebGeolocationController.cpp:54 > +GeolocationController* WebGeolocationController::controller() const
For WebGeolocationClient you put this inline in the header.
> WebKit/chromium/src/WebGeolocationController.cpp:61 > +// by the consumer of Chromium WebKit.
Maybe put this with the declaration?
> WebKit/chromium/src/WebGeolocationPermissionRequestContainer.cpp:38 > +typedef HashMap<Geolocation*, int> GeolocationIDMap;
CamelCase - GeolocationIdMap
> WebKit/chromium/src/WebGeolocationPermissionRequestContainer.cpp:50 > +
extra line
John Knottenbelt
Comment 21
2010-12-08 10:31:02 PST
Created
attachment 75926
[details]
Patch
Steve Block
Comment 22
2010-12-10 06:54:49 PST
Comment on
attachment 75926
[details]
Patch r=me
WebKit Review Bot
Comment 23
2010-12-10 07:12:09 PST
Comment on
attachment 75926
[details]
Patch Clearing flags on attachment: 75926 Committed
r73724
: <
http://trac.webkit.org/changeset/73724
>
WebKit Review Bot
Comment 24
2010-12-10 07:12:17 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug