Bug 45752

Created attachment 69331 [details] Patch

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.

Created attachment 69332 [details] Patch

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:

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

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 on attachment 69332 [details] Patch r- based on Comment #6.

Created attachment 74857 [details] Patch

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 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 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.

Created attachment 75692 [details] Patch

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.

Created attachment 75812 [details] Patch

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.

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.

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.

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.

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 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

Created attachment 75926 [details] Patch

Comment on attachment 75926 [details] Patch r=me

Comment on attachment 75926 [details] Patch Clearing flags on attachment: 75926 Committed r73724: <http://trac.webkit.org/changeset/73724>

All reviewed patches have been landed. Closing bug.