Bug 45752 - [chromium] Implement client based geolocation bindings
Summary: [chromium] Implement client based geolocation bindings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: John Knottenbelt
URL:
Keywords:
Depends on: 47586 48518 50061
Blocks: 40373 46895 50562
  Show dependency treegraph
 
Reported: 2010-09-14 07:48 PDT by Jonathan Dixon
Modified: 2010-12-10 07:12 PST (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Dixon 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
Comment 1 John Knottenbelt 2010-09-30 06:32:10 PDT
Created attachment 69331 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 John Knottenbelt 2010-09-30 06:40:34 PDT
Created attachment 69332 [details]
Patch
Comment 4 Jonathan Dixon 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:
Comment 5 WebKit Review Bot 2010-09-30 08:35:54 PDT
Attachment 69332 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4231030
Comment 6 Steve Block 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?
Comment 7 David Kilzer (:ddkilzer) 2010-10-24 08:36:57 PDT
Comment on attachment 69332 [details]
Patch

r- based on Comment #6.
Comment 8 John Knottenbelt 2010-11-25 04:38:03 PST
Created attachment 74857 [details]
Patch
Comment 9 Steve Block 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?
Comment 10 John Knottenbelt 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
Comment 11 Darin Fisher (:fishd, Google) 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.
Comment 12 John Knottenbelt 2010-12-06 07:21:53 PST
Created attachment 75692 [details]
Patch
Comment 13 Jonathan Dixon 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.
Comment 14 John Knottenbelt 2010-12-07 07:06:55 PST
Created attachment 75812 [details]
Patch
Comment 15 WebKit Review Bot 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.
Comment 16 WebKit Review Bot 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.
Comment 17 WebKit Review Bot 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.
Comment 18 WebKit Review Bot 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.
Comment 19 WebKit Review Bot 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.
Comment 20 Steve Block 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
Comment 21 John Knottenbelt 2010-12-08 10:31:02 PST
Created attachment 75926 [details]
Patch
Comment 22 Steve Block 2010-12-10 06:54:49 PST
Comment on attachment 75926 [details]
Patch

r=me
Comment 23 WebKit Review Bot 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>
Comment 24 WebKit Review Bot 2010-12-10 07:12:17 PST
All reviewed patches have been landed.  Closing bug.