Bug 32068 – Implement Chromium Geolocation Sevice (GeolocationServiceChromium)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jonathan Dixon]

A working implementation of GeolocationService is required as part of https://bugs.webkit.org/show_bug.cgi?id=32065

[Marcus Bulach]

Created attachment 47840 [details]
WebCore bits

(note: this is not yet for formal review, rather a pre-design review.
 counterpart in chromium: http://codereview.chromium.org/548188/show)

[Marcus Bulach]

Created attachment 47841 [details]
WebKit bits

(same notes as previous attachment..)

[Jonathan Dixon]

assigning to marcus..

[Marcus Bulach]

Created attachment 48591 [details]
WebCore bits

jorlow, would you please take a look at this?

[Marcus Bulach]

Created attachment 48592 [details]
WebKit bits

[Jeremy Orlow]

Comment on attachment 48592 [details]
WebKit bits

Please do not separate the WebCore and WebKit bits.  You'll need to edit your .gclient file to make this work.

Normally you set the review flag to ? when you're ready for a review.

> Index: WebKit.gyp
> ===================================================================
> --- WebKit.gyp	(revision 54653)
> +++ WebKit.gyp	(working copy)
> @@ -68,6 +68,7 @@
>                  'WEBKIT_IMPLEMENTATION',
>              ],
>              'sources': [
> +                'public/GeolocationServiceBridge.h',
>                  'public/gtk/WebInputEventFactory.h',
>                  'public/linux/WebFontRendering.h',
>                  'public/linux/WebRenderTheme.h',
> @@ -231,6 +232,7 @@
>                  'src/EventListenerWrapper.h',
>                  'src/FrameLoaderClientImpl.cpp',
>                  'src/FrameLoaderClientImpl.h',
> +                'src/GeolocationServiceBridgeChromium.cpp',		
>                  'src/gtk/WebFontInfo.cpp',
>                  'src/gtk/WebFontInfo.h',
>                  'src/gtk/WebInputEventFactory.cpp',

I believe you should only be modifying the webkit.gypi.  I'm not sure why there are any files even in this.

> Index: public/GeolocationServiceBridgeChromium.h
> ===================================================================
> --- public/GeolocationServiceBridgeChromium.h	(revision 0)
> +++ public/GeolocationServiceBridgeChromium.h	(revision 0)
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (c) 2009, Google Inc. All rights reserved.

2010


> +
> +// This is the flow from WebKit -> Chromium:

Even though this is the "Chromium" API we try to avoid talking about "Chromium" in it.  Just call it "embedder"

Also, in general, WebKit looks down on comments...especially comments that can easily go stale.  Document just what this does and let the code speak for itself.

> +class GeolocationServiceInterface {
> +public:
> +  // Requests permission for this frame.
> +  virtual void requestPermissionForFrame(int bridge_id, const WebURL& url) = 0;
> +  // Asks the browser process to start updating geolocation.
> +  virtual void startUpdating(int bridge_id, bool hasHighAccuracy) = 0;
> +  // Asks the browser process to stop updating geolocation.
> +  virtual void stopUpdating(int bridge_id) = 0;
> +  // Asks the browser process to suspend updating geolocation.
> +  virtual void suspend(int bridge_id) = 0;
> +  // Asks the browser process to resume updating geolocation.
> +  virtual void resume(int bridge_id) = 0;
> +  // Attaches the GeolocationBridge to this view and returns its id, which
> +  // should be used on subsequent calls for the methods above.
> +  virtual int attachBridge(WebKit::WebGeolocationServiceBridge* geolocationServiceBridge) = 0;

This is all really hard to read.  Maybe add newlines between functions.

> Index: public/WebViewClient.h
> ===================================================================
> --- public/WebViewClient.h	(revision 54653)
> +++ public/WebViewClient.h	(working copy)
> @@ -42,6 +42,7 @@
>  
>  namespace WebKit {
>  
> +class GeolocationServiceInterface;

As you'll notice, pretty much any classes in WebKit are generally prefixed by Web.

>  class WebAccessibilityObject;
>  class WebDragData;
>  class WebFileChooserCompletion;
> @@ -277,6 +278,16 @@ public:
>      virtual void removeAutofillSuggestions(const WebString& name,
>                                             const WebString& value) { }
>  
> +    // Geolocation ---------------------------------------------------------

Newline (since most of the other sections do this)

> +    // Geolocation related functions. This is the API used by
> +    // WebKit::GeolocationServiceChromium and ChromeClientImpl to issue IPC
> +    // requests to the browser process.

Once again, make it vague and not Chromium specific.

> +
> +    // Requests permission for this frame.
> +    virtual WebKit::GeolocationServiceInterface* getGeolocationService() {
> +      return NULL;
> +    }

Put on one line.  If you put on multiple, use webkit style.

> +
>  protected:
>      ~WebViewClient() { }
>  };
> Index: src/ChromeClientImpl.cpp
> ===================================================================
> --- src/ChromeClientImpl.cpp	(revision 54653)
> +++ src/ChromeClientImpl.cpp	(working copy)
> @@ -43,6 +43,10 @@
>  #include "FloatRect.h"
>  #include "FrameLoadRequest.h"
>  #include "FrameView.h"
> +#include "Geolocation.h"
> +#include "GeolocationService.h"
> +#include "GeolocationServiceBridgeChromium.h"
> +#include "GeolocationServiceChromium.h"
>  #include "HitTestResult.h"
>  #include "IntRect.h"
>  #include "Node.h"
> @@ -674,4 +678,14 @@ NotificationPresenter* ChromeClientImpl:
>  }
>  #endif
>  
> +void ChromeClientImpl::requestGeolocationPermissionForFrame(
> +        Frame* frame, Geolocation* geolocation) {

No 80 col limit.  Unwrap most of these unless it's super long.

> +  GeolocationServiceChromium* geolocation_service =
> +      reinterpret_cast<GeolocationServiceChromium*>(
> +          geolocation->getGeolocationService());
> +  m_webView->client()->getGeolocationService()->
> +      requestPermissionForFrame(geolocation_service->geolocationServiceBridge()->getBridgeId(),
> +                                frame->document()->url());
> +}
> +
>  } // namespace WebKit
> Index: src/ChromeClientImpl.h
> ===================================================================
> --- src/ChromeClientImpl.h	(revision 54653)
> +++ src/ChromeClientImpl.h	(working copy)
> @@ -122,7 +122,7 @@ public:
>      virtual WebCore::NotificationPresenter* notificationPresenter() const;
>  #endif
>      virtual void requestGeolocationPermissionForFrame(
> -        WebCore::Frame*, WebCore::Geolocation*) { }
> +        WebCore::Frame*, WebCore::Geolocation*);

Don't wrap.  Why did you take out the {}?  Not all the clients will implement it.

>      virtual void runOpenPanel(WebCore::Frame*, PassRefPtr<WebCore::FileChooser>);
>      virtual bool setCursor(WebCore::PlatformCursorHandle) { return false; }
>      virtual void formStateDidChange(const WebCore::Node*);
> Index: src/GeolocationServiceBridgeChromium.cpp
> ===================================================================
> --- src/GeolocationServiceBridgeChromium.cpp	(revision 0)
> +++ src/GeolocationServiceBridgeChromium.cpp	(revision 0)
> @@ -0,0 +1,174 @@
> +/*
> + * Copyright (c) 2009, Google Inc. All rights reserved.

2010

> +
> +#include "GeolocationServiceChromium.h"
> +#include "GeolocationServiceBridgeChromium.h"
> +#include "Chrome.h"
> +#include "ChromeClientImpl.h"
> +#include "Frame.h"
> +#include "Geolocation.h"
> +#include "Geoposition.h"
> +#include "Page.h"
> +#include "PositionError.h"
> +#include "PositionOptions.h"
> +#include "WebFrame.h"
> +#include "WebFrameImpl.h"
> +#include "WebViewClient.h"
> +#include "WebViewImpl.h"

Alpha order.

> +
> +#if ENABLE(GEOLOCATION)
> +
> +using WebCore::Coordinates;
> +using WebCore::Frame;
> +using WebCore::Geolocation;
> +using WebCore::GeolocationServiceChromium;

Alpha order.

> +using WebCore::GeolocationServiceBridge;
> +using WebCore::GeolocationServiceClient;
> +using WebCore::Geoposition;
> +using WebCore::PositionError;
> +using WebCore::PositionOptions;
> +using WebCore::String;
> +
> +namespace WebKit {
> +
> +class GeolocationServiceBridgeImpl
> +    : public GeolocationServiceBridge, public WebGeolocationServiceBridge {

Combine those lines.

> +public:
> +    explicit GeolocationServiceBridgeImpl(GeolocationServiceChromium* c);

Only put parameter names when it's necessary to understand what the parameter is.

> +    virtual ~GeolocationServiceBridgeImpl();
> +
> +    // GeolocationServiceBridge
> +    virtual bool startUpdating(PositionOptions*);
> +    virtual void stopUpdating();
> +    virtual void suspend();
> +    virtual void resume();
> +    virtual int getBridgeId() const;
> +
> +    // WebGeolocationServiceBridge
> +    virtual void setIsAllowed(bool allowed);
> +    virtual void setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp);
> +    virtual void setLastError(int error_code, const WebString& message);
> +
> +private:
> +    WebViewClient* getWebViewClient();
> +
> +    GeolocationServiceChromium* m_GeolocationServiceChromium;

Should this be deleted automatically at te end?  If so, this should be an OwnPtr.

> +    int m_BridgeId;

m_bridgeId.

> +};
> +
> +static GeolocationServiceBridge* createGeolocationServiceBridgeImpl(GeolocationServiceChromium* c)
> +{
> +    return new GeolocationServiceBridgeImpl(c);
> +}
> +
> +GeolocationServiceBridgeImpl::GeolocationServiceBridgeImpl(GeolocationServiceChromium* c)

c is a bad name.

> +    : m_GeolocationServiceChromium(c)
> +{
> +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> +    m_BridgeId = webViewClient->getGeolocationService()->attachBridge(this);

ASSERT(m_bridgeId) since later you do an if (m_bridgeId) to see if it's valid.

But why even do this?  Can't you do it lazily on start?  (Or is start implicit when it's created?)

> +}
> +
> +GeolocationServiceBridgeImpl::~GeolocationServiceBridgeImpl()
> +{
> +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> +    // webViewClient might be NULL at this point if the frame has been disconnected.
> +    // Note that in this case, stopUpdating() has been called, and we have already
> +    // dettached ourselves.

I assume that you've seen this condition or are 99% sure it's possible?  We like to avoid error checking code "just in case".

> +    if (m_BridgeId && webViewClient)
> +        webViewClient->getGeolocationService()->dettachBridge(m_BridgeId);
> +}
> +
> +bool GeolocationServiceBridgeImpl::startUpdating(PositionOptions* positionOptions)
> +{
> +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> +    if (!m_BridgeId) m_BridgeId = webViewClient->getGeolocationService()->attachBridge(this);

Avoid putting these on the same lines.

> +    webViewClient->getGeolocationService()->startUpdating(
> +        m_BridgeId, positionOptions->enableHighAccuracy());

Combine these lines.

> +    //// TODO(bulach): this will trigger a permission request regardless.

// FIXME: This will trigger a permission request regardless. Is it correct? Confirm with andreip. 

> +    //// Is it correct? confirm with andreip.
> +    //positionChanged();
> +    return true;
> +}
> +
> +void GeolocationServiceBridgeImpl::stopUpdating()
> +{
> +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> +    webViewClient->getGeolocationService()->stopUpdating(m_BridgeId);
> +    m_BridgeId = 0;

// Do you need to disconnect it?

> +}
> +
> +void GeolocationServiceBridgeImpl::suspend()
> +{
> +    getWebViewClient()->getGeolocationService()->suspend(m_BridgeId);

Use this style everywhere instead of saving te webViewClient to a var.

> +}
> +
> +void GeolocationServiceBridgeImpl::resume()
> +{
> +    getWebViewClient()->getGeolocationService()->resume(m_BridgeId);
> +}
> +
> +int GeolocationServiceBridgeImpl::getBridgeId() const
> +{
> +    return m_BridgeId;
> +}
> +
> +void GeolocationServiceBridgeImpl::setIsAllowed(bool allowed) {
> +    m_GeolocationServiceChromium->setIsAllowed(allowed);
> +}
> +
> +void GeolocationServiceBridgeImpl::setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp) {
> +    m_GeolocationServiceChromium->setLastPosition(latitude, longitude, providesAltitude, altitude, accuracy, providesAltitudeAccuracy, altitudeAccuracy, providesHeading, heading, providesSpeed, speed, timestamp);
> +}
> +
> +void GeolocationServiceBridgeImpl::setLastError(int error_code, const WebString& message) {

{ on newline here and elsewhere.

> +    m_GeolocationServiceChromium->setLastError(error_code, message);
> +}
> +
> +WebViewClient* GeolocationServiceBridgeImpl::getWebViewClient() {
> +    Frame* frame = m_GeolocationServiceChromium->frame();
> +    if (!frame || !frame->page())
> +        return NULL;

When can this happen?  Handling these conditions is a pain, so lets only do it if we know we need to.

> +    WebKit::ChromeClientImpl* chromeClientImpl = static_cast<WebKit::ChromeClientImpl*>(frame->page()->chrome()->client());
> +    WebKit::WebViewClient* webViewClient = chromeClientImpl->webView()->client();
> +    return webViewClient;
> +}
> +
> +} // namespace WebKit
> +
> +
> +namespace WebCore {
> +
> +// Sets up the factory function for GeolocationService.
> +GeolocationServiceChromium::BridgeFactoryFunction* GeolocationServiceChromium::s_bridgeFactoryFunction = &WebKit::createGeolocationServiceBridgeImpl;
> +
> +}  // namespace WebCore

Put this at the beginning if you must...  Is there precedent for something like this elsewhere?  If not, maybe there's a better way to do this.

> +
> +
> +#endif // ENABLE(GEOLOCATION)
> 
> Property changes on: src/GeolocationServiceBridgeChromium.cpp
> ___________________________________________________________________
> Added: svn:executable
>    + *
> Added: svn:eol-style
>    + LF
> 
> Index: src/WebViewImpl.cpp
> ===================================================================
> --- src/WebViewImpl.cpp	(revision 54653)
> +++ src/WebViewImpl.cpp	(working copy)
> @@ -426,7 +426,7 @@ void WebViewImpl::mouseUp(const WebMouse
>          IntPoint contentPoint = view->windowToContents(clickPoint);
>          HitTestResult hitTestResult = focused->eventHandler()->hitTestResultAtPoint(contentPoint, false, false, ShouldHitTestScrollbars);
>          // We don't want to send a paste when middle clicking a scroll bar or a
> -        // link (which will navigate later in the code).  The main scrollbars 
> +        // link (which will navigate later in the code).  The main scrollbars
>          // have to be handled separately.
>          if (!hitTestResult.scrollbar() && !hitTestResult.isLiveLink() && focused && !view->scrollbarAtPoint(clickPoint)) {
>              Editor* editor = focused->editor();

Whitespace cleanup here or there is ok, but changes where the majority (or entirety) are white space are looked down upon.  Please remove this file.

[Jeremy Orlow]

Comment on attachment 48591 [details]
WebCore bits

> Index: platform/chromium/GeolocationServiceChromium.cpp
> ===================================================================
> --- platform/chromium/GeolocationServiceChromium.cpp	(revision 54653)
> +++ platform/chromium/GeolocationServiceChromium.cpp	(working copy)
> @@ -29,27 +29,81 @@
>   */
>  
>  #include "config.h"
> -#include "GeolocationService.h"
> +#include "GeolocationServiceChromium.h"
>  
>  namespace WebCore {
>  
> -class GeolocationServiceChromium : public GeolocationService {
> -public:
> -    GeolocationServiceChromium(GeolocationServiceClient* c)
> -        : GeolocationService(c)
> -    {
> -    }
> -    // FIXME: Implement. https://bugs.webkit.org/show_bug.cgi?id=32068
> -};
> -
> -// This guard is the counterpart of the one in WebCore/platform/GeolocationService.cpp
> -#if ENABLE(GEOLOCATION)
> -static GeolocationService* createGeolocationService(GeolocationServiceClient* c)
> +GeolocationServiceChromium::GeolocationServiceChromium(GeolocationServiceClient* c)
> +        : GeolocationService(c),
> +          m_Geolocation(reinterpret_cast<Geolocation*>(c))
> +{
> +    m_LastPosition = Geoposition::create(
> +        Coordinates::create(0.0, 0.0, false, 0.0, 0.0, false, 0.0, false, 0.0, false, 0.0),
> +        0);

All on one line.

> +    m_LastError = PositionError::create(PositionError::POSITION_UNAVAILABLE, "");
> +    m_geolocationServiceBridge.set((*s_bridgeFactoryFunction)(this));

Just use ChromiumBridge if you're going to use linking tricks anyway.  You can probably also do this in the initialization section as well.

> +}
> +
> +void GeolocationServiceChromium::setIsAllowed(bool allowed)
> +{
> +    m_Geolocation->setIsAllowed(allowed);  
> +}
> +
> +void GeolocationServiceChromium::setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp)
> +{
> +    m_LastPosition = Geoposition::create(
> +        Coordinates::create(latitude, longitude, providesAltitude, altitude, accuracy, providesAltitudeAccuracy, altitudeAccuracy, providesHeading, heading, providesSpeed, speed),
> +        timestamp);

One line.

> +    positionChanged();
> +}
> +
> +void GeolocationServiceChromium::setLastError(int error_code, const String& message)
> +{
> +    m_LastError = PositionError::create(static_cast<PositionError::ErrorCode>(error_code), message);
> +    errorOccurred();
> +}
> +
> +Frame* GeolocationServiceChromium::frame()
> +{
> +    return m_Geolocation->frame();
> +}
> +
> +bool GeolocationServiceChromium::startUpdating(PositionOptions* options)
> +{
> +    return m_geolocationServiceBridge->startUpdating(options);
> +}
> +
> +void GeolocationServiceChromium::stopUpdating()
> +{
> +    return m_geolocationServiceBridge->stopUpdating();
> +}
> +
> +void GeolocationServiceChromium::suspend()
> +{
> +    return m_geolocationServiceBridge->suspend();
> +}
> +
> +void GeolocationServiceChromium::resume()
> +{
> +    return m_geolocationServiceBridge->resume();
> +}
> +
> +Geoposition* GeolocationServiceChromium::lastPosition() const
> +{
> +    return m_LastPosition.get();
> +}
> +
> +PositionError* GeolocationServiceChromium::lastError() const
> +{
> +    return m_LastError.get();

This doesn't seem particularly safe.  You probably should be passing a PassRefPtr.

Read: http://webkit.org/coding/RefPtr.html

> +}
> +
> +static GeolocationService* createGeolocationServiceChromium(GeolocationServiceClient* c)
>  {
>      return new GeolocationServiceChromium(c);
>  }
>  
> -GeolocationService::FactoryFunction* GeolocationService::s_factoryFunction = &createGeolocationService;
> -#endif
> +// Sets up the factory function for GeolocationService.
> +GeolocationService::FactoryFunction* GeolocationService::s_factoryFunction = &createGeolocationServiceChromium;
>  
>  } // namespace WebCore
> Index: platform/chromium/GeolocationServiceChromium.h
> ===================================================================
> --- platform/chromium/GeolocationServiceChromium.h	(revision 0)
> +++ platform/chromium/GeolocationServiceChromium.h	(revision 0)
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (c) 2009, Google Inc. All rights reserved.

2010

> + * 
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + * 
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + * 
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"

This only goes in the .cpp files.

> +#include "PlatformString.h"

Alpha order.

> +#include "Geolocation.h"
> +#include "GeolocationService.h"
> +#include "Geoposition.h"
> +#include "PositionError.h"
> +
> +namespace WebCore {
> +
> +// Provides an interface for GeolocationServiceChromium to call into chromium.
> +class GeolocationServiceBridge {
> +public:
> +    // Called by GeolocationServiceChromium.
> +    virtual bool startUpdating(PositionOptions*) = 0;
> +    virtual void stopUpdating() = 0;
> +    virtual void suspend() = 0;
> +    virtual void resume() = 0;
> +
> +    // Called by the Chromium side, to identify this bridge..
> +    virtual int getBridgeId() const = 0;
> +};
> +
> +// This class extends GeolocationService, and uses GeolocationServiceBridge to
> +// call into chromium, as well as provides a few extra methods so that we can
> +// be notified of permission, position, error, etc, from chromium.
> +class GeolocationServiceChromium : public GeolocationService {
> +public:
> +    explicit GeolocationServiceChromium(GeolocationServiceClient* c);

Delete c.

> +
> +    GeolocationServiceBridge* geolocationServiceBridge() const { return m_geolocationServiceBridge.get(); }
> +    void setIsAllowed(bool allowed);
> +    void setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp);
> +    void setLastError(int error_code, const String& message);
> +    Frame* frame();
> +
> +    // From GeolocationService.
> +    virtual bool startUpdating(PositionOptions*);
> +    virtual void stopUpdating();
> +    virtual void suspend();
> +    virtual void resume();
> +    virtual Geoposition* lastPosition() const;
> +    virtual PositionError* lastError() const;
> +
> +private:
> +    Geolocation* m_Geolocation;
> +    OwnPtr<GeolocationServiceBridge*> m_geolocationServiceBridge;    

Why is this an OwnPtr to a pointer?

> +    RefPtr<Geoposition> m_LastPosition;
> +    RefPtr<PositionError> m_LastError;
> +
> +    typedef GeolocationServiceBridge* (BridgeFactoryFunction)(GeolocationServiceChromium*);
> +    static BridgeFactoryFunction* s_bridgeFactoryFunction;

Get rid of.  Use ChromiumBridge.

> +};
> +
> +} // namespace WebCore
> 
> Property changes on: platform/chromium/GeolocationServiceChromium.h
> ___________________________________________________________________
> Added: svn:eol-style
>    + LF
>

[Marcus Bulach]

Created attachment 48649 [details]
Geolocation implementation

thanks for the quick review jeremy! all comments addressed (will reply in a separate email), ran  WebKitTools/Scripts/check-webkit-style and have a single file for this patch.
would you please take another look?

thanks,
marcus

[Marcus Bulach]

(In reply to comment #6)
> (From update of attachment 48592 [details])
> Please do not separate the WebCore and WebKit bits.  You'll need to edit your
> .gclient file to make this work.

done, using a single file now.


> 
> Normally you set the review flag to ? when you're ready for a review.
done.


> 
> > Index: WebKit.gyp
> > ===================================================================
> > --- WebKit.gyp	(revision 54653)
> > +++ WebKit.gyp	(working copy)
> > @@ -68,6 +68,7 @@
> >                  'WEBKIT_IMPLEMENTATION',
> >              ],
> >              'sources': [
> > +                'public/GeolocationServiceBridge.h',
> >                  'public/gtk/WebInputEventFactory.h',
> >                  'public/linux/WebFontRendering.h',
> >                  'public/linux/WebRenderTheme.h',
> > @@ -231,6 +232,7 @@
> >                  'src/EventListenerWrapper.h',
> >                  'src/FrameLoaderClientImpl.cpp',
> >                  'src/FrameLoaderClientImpl.h',
> > +                'src/GeolocationServiceBridgeChromium.cpp',		
> >                  'src/gtk/WebFontInfo.cpp',
> >                  'src/gtk/WebFontInfo.h',
> >                  'src/gtk/WebInputEventFactory.cpp',
> 
> I believe you should only be modifying the webkit.gypi.  I'm not sure why there
> are any files even in this.

hmm.. there aren't that many files in gypi, only a few specific projects from what I understood..
I kept this in gyp instead to keep in line with existing stuff, but I'm happy to change.

> 
> > Index: public/GeolocationServiceBridgeChromium.h
> > ===================================================================
> > --- public/GeolocationServiceBridgeChromium.h	(revision 0)
> > +++ public/GeolocationServiceBridgeChromium.h	(revision 0)
> > @@ -0,0 +1,91 @@
> > +/*
> > + * Copyright (c) 2009, Google Inc. All rights reserved.
> 
> 2010
> 
> 
> > +
> > +// This is the flow from WebKit -> Chromium:
> 
> Even though this is the "Chromium" API we try to avoid talking about "Chromium"
> in it.  Just call it "embedder"
> 
> Also, in general, WebKit looks down on comments...especially comments that can
> easily go stale.  Document just what this does and let the code speak for
> itself.

I see, thanks for the clarification! Fixed.

> 
> > +class GeolocationServiceInterface {
> > +public:
> > +  // Requests permission for this frame.
> > +  virtual void requestPermissionForFrame(int bridge_id, const WebURL& url) = 0;
> > +  // Asks the browser process to start updating geolocation.
> > +  virtual void startUpdating(int bridge_id, bool hasHighAccuracy) = 0;
> > +  // Asks the browser process to stop updating geolocation.
> > +  virtual void stopUpdating(int bridge_id) = 0;
> > +  // Asks the browser process to suspend updating geolocation.
> > +  virtual void suspend(int bridge_id) = 0;
> > +  // Asks the browser process to resume updating geolocation.
> > +  virtual void resume(int bridge_id) = 0;
> > +  // Attaches the GeolocationBridge to this view and returns its id, which
> > +  // should be used on subsequent calls for the methods above.
> > +  virtual int attachBridge(WebKit::WebGeolocationServiceBridge* geolocationServiceBridge) = 0;
> 
> This is all really hard to read.  Maybe add newlines between functions.

Simplified it a lot.

> 
> > Index: public/WebViewClient.h
> > ===================================================================
> > --- public/WebViewClient.h	(revision 54653)
> > +++ public/WebViewClient.h	(working copy)
> > @@ -42,6 +42,7 @@
> >  
> >  namespace WebKit {
> >  
> > +class GeolocationServiceInterface;
> 
> As you'll notice, pretty much any classes in WebKit are generally prefixed by
> Web.

Renamed.

> 
> >  class WebAccessibilityObject;
> >  class WebDragData;
> >  class WebFileChooserCompletion;
> > @@ -277,6 +278,16 @@ public:
> >      virtual void removeAutofillSuggestions(const WebString& name,
> >                                             const WebString& value) { }
> >  
> > +    // Geolocation ---------------------------------------------------------
> 
> Newline (since most of the other sections do this)

Done.

> 
> > +    // Geolocation related functions. This is the API used by
> > +    // WebKit::GeolocationServiceChromium and ChromeClientImpl to issue IPC
> > +    // requests to the browser process.
> 
> Once again, make it vague and not Chromium specific.

Done.

> 
> > +
> > +    // Requests permission for this frame.
> > +    virtual WebKit::GeolocationServiceInterface* getGeolocationService() {
> > +      return NULL;
> > +    }
> 
> Put on one line.  If you put on multiple, use webkit style.

Done.

> 
> > +
> >  protected:
> >      ~WebViewClient() { }
> >  };
> > Index: src/ChromeClientImpl.cpp
> > ===================================================================
> > --- src/ChromeClientImpl.cpp	(revision 54653)
> > +++ src/ChromeClientImpl.cpp	(working copy)
> > @@ -43,6 +43,10 @@
> >  #include "FloatRect.h"
> >  #include "FrameLoadRequest.h"
> >  #include "FrameView.h"
> > +#include "Geolocation.h"
> > +#include "GeolocationService.h"
> > +#include "GeolocationServiceBridgeChromium.h"
> > +#include "GeolocationServiceChromium.h"
> >  #include "HitTestResult.h"
> >  #include "IntRect.h"
> >  #include "Node.h"
> > @@ -674,4 +678,14 @@ NotificationPresenter* ChromeClientImpl:
> >  }
> >  #endif
> >  
> > +void ChromeClientImpl::requestGeolocationPermissionForFrame(
> > +        Frame* frame, Geolocation* geolocation) {
> 
> No 80 col limit.  Unwrap most of these unless it's super long.

Done

> 
> > +  GeolocationServiceChromium* geolocation_service =
> > +      reinterpret_cast<GeolocationServiceChromium*>(
> > +          geolocation->getGeolocationService());
> > +  m_webView->client()->getGeolocationService()->
> > +      requestPermissionForFrame(geolocation_service->geolocationServiceBridge()->getBridgeId(),
> > +                                frame->document()->url());
> > +}
> > +
> >  } // namespace WebKit
> > Index: src/ChromeClientImpl.h
> > ===================================================================
> > --- src/ChromeClientImpl.h	(revision 54653)
> > +++ src/ChromeClientImpl.h	(working copy)
> > @@ -122,7 +122,7 @@ public:
> >      virtual WebCore::NotificationPresenter* notificationPresenter() const;
> >  #endif
> >      virtual void requestGeolocationPermissionForFrame(
> > -        WebCore::Frame*, WebCore::Geolocation*) { }
> > +        WebCore::Frame*, WebCore::Geolocation*);
> 
> Don't wrap.  Why did you take out the {}?  Not all the clients will implement
> it.

hmm, this is the ChromeClientImpl.h, which now implements this function on ChromeClientImpl.cc..

> 
> >      virtual void runOpenPanel(WebCore::Frame*, PassRefPtr<WebCore::FileChooser>);
> >      virtual bool setCursor(WebCore::PlatformCursorHandle) { return false; }
> >      virtual void formStateDidChange(const WebCore::Node*);
> > Index: src/GeolocationServiceBridgeChromium.cpp
> > ===================================================================
> > --- src/GeolocationServiceBridgeChromium.cpp	(revision 0)
> > +++ src/GeolocationServiceBridgeChromium.cpp	(revision 0)
> > @@ -0,0 +1,174 @@
> > +/*
> > + * Copyright (c) 2009, Google Inc. All rights reserved.
> 
> 2010

done

> 
> > +
> > +#include "GeolocationServiceChromium.h"
> > +#include "GeolocationServiceBridgeChromium.h"
> > +#include "Chrome.h"
> > +#include "ChromeClientImpl.h"
> > +#include "Frame.h"
> > +#include "Geolocation.h"
> > +#include "Geoposition.h"
> > +#include "Page.h"
> > +#include "PositionError.h"
> > +#include "PositionOptions.h"
> > +#include "WebFrame.h"
> > +#include "WebFrameImpl.h"
> > +#include "WebViewClient.h"
> > +#include "WebViewImpl.h"
> 
> Alpha order.
done

> 
> > +
> > +#if ENABLE(GEOLOCATION)
> > +
> > +using WebCore::Coordinates;
> > +using WebCore::Frame;
> > +using WebCore::Geolocation;
> > +using WebCore::GeolocationServiceChromium;
> 
> Alpha order.
done

> 
> > +using WebCore::GeolocationServiceBridge;
> > +using WebCore::GeolocationServiceClient;
> > +using WebCore::Geoposition;
> > +using WebCore::PositionError;
> > +using WebCore::PositionOptions;
> > +using WebCore::String;
> > +
> > +namespace WebKit {
> > +
> > +class GeolocationServiceBridgeImpl
> > +    : public GeolocationServiceBridge, public WebGeolocationServiceBridge {
> 
> Combine those lines.
done

> 
> > +public:
> > +    explicit GeolocationServiceBridgeImpl(GeolocationServiceChromium* c);
> 
> Only put parameter names when it's necessary to understand what the parameter
> is.
done


> 
> > +    virtual ~GeolocationServiceBridgeImpl();
> > +
> > +    // GeolocationServiceBridge
> > +    virtual bool startUpdating(PositionOptions*);
> > +    virtual void stopUpdating();
> > +    virtual void suspend();
> > +    virtual void resume();
> > +    virtual int getBridgeId() const;
> > +
> > +    // WebGeolocationServiceBridge
> > +    virtual void setIsAllowed(bool allowed);
> > +    virtual void setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp);
> > +    virtual void setLastError(int error_code, const WebString& message);
> > +
> > +private:
> > +    WebViewClient* getWebViewClient();
> > +
> > +    GeolocationServiceChromium* m_GeolocationServiceChromium;
> 
> Should this be deleted automatically at te end?  If so, this should be an
> OwnPtr.

added a comment clarifying why it's not an OwnPtr.

> 
> > +    int m_BridgeId;
> 
> m_bridgeId.
done

> 
> > +};
> > +
> > +static GeolocationServiceBridge* createGeolocationServiceBridgeImpl(GeolocationServiceChromium* c)
> > +{
> > +    return new GeolocationServiceBridgeImpl(c);
> > +}
> > +
> > +GeolocationServiceBridgeImpl::GeolocationServiceBridgeImpl(GeolocationServiceChromium* c)
> 
> c is a bad name.
Renamed

> 
> > +    : m_GeolocationServiceChromium(c)
> > +{
> > +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> > +    m_BridgeId = webViewClient->getGeolocationService()->attachBridge(this);
> 
> ASSERT(m_bridgeId) since later you do an if (m_bridgeId) to see if it's valid.
> 
> But why even do this?  Can't you do it lazily on start?  (Or is start implicit
> when it's created?)

as we chatted, changed this a bit and added a few comments to clarify.

> 
> > +}
> > +
> > +GeolocationServiceBridgeImpl::~GeolocationServiceBridgeImpl()
> > +{
> > +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> > +    // webViewClient might be NULL at this point if the frame has been disconnected.
> > +    // Note that in this case, stopUpdating() has been called, and we have already
> > +    // dettached ourselves.
> 
> I assume that you've seen this condition or are 99% sure it's possible?  We
> like to avoid error checking code "just in case".

as above, this seems to be necessary.

> 
> > +    if (m_BridgeId && webViewClient)
> > +        webViewClient->getGeolocationService()->dettachBridge(m_BridgeId);
> > +}
> > +
> > +bool GeolocationServiceBridgeImpl::startUpdating(PositionOptions* positionOptions)
> > +{
> > +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> > +    if (!m_BridgeId) m_BridgeId = webViewClient->getGeolocationService()->attachBridge(this);
> 
> Avoid putting these on the same lines.
done


> 
> > +    webViewClient->getGeolocationService()->startUpdating(
> > +        m_BridgeId, positionOptions->enableHighAccuracy());
> 
> Combine these lines.

done

> 
> > +    //// TODO(bulach): this will trigger a permission request regardless.
> 
> // FIXME: This will trigger a permission request regardless. Is it correct?
> Confirm with andreip. 

done

> 
> > +    //// Is it correct? confirm with andreip.
> > +    //positionChanged();
> > +    return true;
> > +}
> > +
> > +void GeolocationServiceBridgeImpl::stopUpdating()
> > +{
> > +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> > +    webViewClient->getGeolocationService()->stopUpdating(m_BridgeId);
> > +    m_BridgeId = 0;
> 
> // Do you need to disconnect it?

ahn, good catch! fixed.

> 
> > +}
> > +
> > +void GeolocationServiceBridgeImpl::suspend()
> > +{
> > +    getWebViewClient()->getGeolocationService()->suspend(m_BridgeId);
> 
> Use this style everywhere instead of saving te webViewClient to a var.
done


> 
> > +}
> > +
> > +void GeolocationServiceBridgeImpl::resume()
> > +{
> > +    getWebViewClient()->getGeolocationService()->resume(m_BridgeId);
> > +}
> > +
> > +int GeolocationServiceBridgeImpl::getBridgeId() const
> > +{
> > +    return m_BridgeId;
> > +}
> > +
> > +void GeolocationServiceBridgeImpl::setIsAllowed(bool allowed) {
> > +    m_GeolocationServiceChromium->setIsAllowed(allowed);
> > +}
> > +
> > +void GeolocationServiceBridgeImpl::setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp) {
> > +    m_GeolocationServiceChromium->setLastPosition(latitude, longitude, providesAltitude, altitude, accuracy, providesAltitudeAccuracy, altitudeAccuracy, providesHeading, heading, providesSpeed, speed, timestamp);
> > +}
> > +
> > +void GeolocationServiceBridgeImpl::setLastError(int error_code, const WebString& message) {
> 
> { on newline here and elsewhere.

done

> 
> > +    m_GeolocationServiceChromium->setLastError(error_code, message);
> > +}
> > +
> > +WebViewClient* GeolocationServiceBridgeImpl::getWebViewClient() {
> > +    Frame* frame = m_GeolocationServiceChromium->frame();
> > +    if (!frame || !frame->page())
> > +        return NULL;
> 
> When can this happen?  Handling these conditions is a pain, so lets only do it
> if we know we need to.

as above, this is needed for the case where Geolocation itself has been disconnected from the frame, and after that it'll destroy its GeolocationService.


> 
> > +    WebKit::ChromeClientImpl* chromeClientImpl = static_cast<WebKit::ChromeClientImpl*>(frame->page()->chrome()->client());
> > +    WebKit::WebViewClient* webViewClient = chromeClientImpl->webView()->client();
> > +    return webViewClient;
> > +}
> > +
> > +} // namespace WebKit
> > +
> > +
> > +namespace WebCore {
> > +
> > +// Sets up the factory function for GeolocationService.
> > +GeolocationServiceChromium::BridgeFactoryFunction* GeolocationServiceChromium::s_bridgeFactoryFunction = &WebKit::createGeolocationServiceBridgeImpl;
> > +
> > +}  // namespace WebCore
> 
> Put this at the beginning if you must...  Is there precedent for something like
> this elsewhere?  If not, maybe there's a better way to do this.

using ChromiumBridge as suggested. however, instead of publishing the GeolocationServiceBridgeImpl, there's a factory function createGeolocationServiceBridgeImpl which is used by ChromiumBridge, and implemented here..

> 
> > +
> > +
> > +#endif // ENABLE(GEOLOCATION)
> > 
> > Property changes on: src/GeolocationServiceBridgeChromium.cpp
> > ___________________________________________________________________
> > Added: svn:executable
> >    + *
> > Added: svn:eol-style
> >    + LF
> > 
> > Index: src/WebViewImpl.cpp
> > ===================================================================
> > --- src/WebViewImpl.cpp	(revision 54653)
> > +++ src/WebViewImpl.cpp	(working copy)
> > @@ -426,7 +426,7 @@ void WebViewImpl::mouseUp(const WebMouse
> >          IntPoint contentPoint = view->windowToContents(clickPoint);
> >          HitTestResult hitTestResult = focused->eventHandler()->hitTestResultAtPoint(contentPoint, false, false, ShouldHitTestScrollbars);
> >          // We don't want to send a paste when middle clicking a scroll bar or a
> > -        // link (which will navigate later in the code).  The main scrollbars 
> > +        // link (which will navigate later in the code).  The main scrollbars
> >          // have to be handled separately.
> >          if (!hitTestResult.scrollbar() && !hitTestResult.isLiveLink() && focused && !view->scrollbarAtPoint(clickPoint)) {
> >              Editor* editor = focused->editor();
> 
> Whitespace cleanup here or there is ok, but changes where the majority (or
> entirety) are white space are looked down upon.  Please remove this file.
removed.

[Marcus Bulach]

(In reply to comment #7)
> (From update of attachment 48591 [details])
> > Index: platform/chromium/GeolocationServiceChromium.cpp
> > ===================================================================
> > --- platform/chromium/GeolocationServiceChromium.cpp	(revision 54653)
> > +++ platform/chromium/GeolocationServiceChromium.cpp	(working copy)
> > @@ -29,27 +29,81 @@
> >   */
> >  
> >  #include "config.h"
> > -#include "GeolocationService.h"
> > +#include "GeolocationServiceChromium.h"
> >  
> >  namespace WebCore {
> >  
> > -class GeolocationServiceChromium : public GeolocationService {
> > -public:
> > -    GeolocationServiceChromium(GeolocationServiceClient* c)
> > -        : GeolocationService(c)
> > -    {
> > -    }
> > -    // FIXME: Implement. https://bugs.webkit.org/show_bug.cgi?id=32068
> > -};
> > -
> > -// This guard is the counterpart of the one in WebCore/platform/GeolocationService.cpp
> > -#if ENABLE(GEOLOCATION)
> > -static GeolocationService* createGeolocationService(GeolocationServiceClient* c)
> > +GeolocationServiceChromium::GeolocationServiceChromium(GeolocationServiceClient* c)
> > +        : GeolocationService(c),
> > +          m_Geolocation(reinterpret_cast<Geolocation*>(c))
> > +{
> > +    m_LastPosition = Geoposition::create(
> > +        Coordinates::create(0.0, 0.0, false, 0.0, 0.0, false, 0.0, false, 0.0, false, 0.0),
> > +        0);
> 
> All on one line.

done

> 
> > +    m_LastError = PositionError::create(PositionError::POSITION_UNAVAILABLE, "");
> > +    m_geolocationServiceBridge.set((*s_bridgeFactoryFunction)(this));
> 
> Just use ChromiumBridge if you're going to use linking tricks anyway.  You can
> probably also do this in the initialization section as well.

using ChromiumBridge, thanks!


> 
> > +}
> > +
> > +void GeolocationServiceChromium::setIsAllowed(bool allowed)
> > +{
> > +    m_Geolocation->setIsAllowed(allowed);  
> > +}
> > +
> > +void GeolocationServiceChromium::setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp)
> > +{
> > +    m_LastPosition = Geoposition::create(
> > +        Coordinates::create(latitude, longitude, providesAltitude, altitude, accuracy, providesAltitudeAccuracy, altitudeAccuracy, providesHeading, heading, providesSpeed, speed),
> > +        timestamp);
> 
> One line.

done

> 
> > +    positionChanged();
> > +}
> > +
> > +void GeolocationServiceChromium::setLastError(int error_code, const String& message)
> > +{
> > +    m_LastError = PositionError::create(static_cast<PositionError::ErrorCode>(error_code), message);
> > +    errorOccurred();
> > +}
> > +
> > +Frame* GeolocationServiceChromium::frame()
> > +{
> > +    return m_Geolocation->frame();
> > +}
> > +
> > +bool GeolocationServiceChromium::startUpdating(PositionOptions* options)
> > +{
> > +    return m_geolocationServiceBridge->startUpdating(options);
> > +}
> > +
> > +void GeolocationServiceChromium::stopUpdating()
> > +{
> > +    return m_geolocationServiceBridge->stopUpdating();
> > +}
> > +
> > +void GeolocationServiceChromium::suspend()
> > +{
> > +    return m_geolocationServiceBridge->suspend();
> > +}
> > +
> > +void GeolocationServiceChromium::resume()
> > +{
> > +    return m_geolocationServiceBridge->resume();
> > +}
> > +
> > +Geoposition* GeolocationServiceChromium::lastPosition() const
> > +{
> > +    return m_LastPosition.get();
> > +}
> > +
> > +PositionError* GeolocationServiceChromium::lastError() const
> > +{
> > +    return m_LastError.get();
> 
> This doesn't seem particularly safe.  You probably should be passing a
> PassRefPtr.
> 
> Read: http://webkit.org/coding/RefPtr.html

this is already defined by GeolocationService interface, this class just implements it..

ownership is not transferred (in Geolocation.cpp, it just uses the values to create new objects), so it seems to be fine.

> 
> > +}
> > +
> > +static GeolocationService* createGeolocationServiceChromium(GeolocationServiceClient* c)
> >  {
> >      return new GeolocationServiceChromium(c);
> >  }
> >  
> > -GeolocationService::FactoryFunction* GeolocationService::s_factoryFunction = &createGeolocationService;
> > -#endif
> > +// Sets up the factory function for GeolocationService.
> > +GeolocationService::FactoryFunction* GeolocationService::s_factoryFunction = &createGeolocationServiceChromium;
> >  
> >  } // namespace WebCore
> > Index: platform/chromium/GeolocationServiceChromium.h
> > ===================================================================
> > --- platform/chromium/GeolocationServiceChromium.h	(revision 0)
> > +++ platform/chromium/GeolocationServiceChromium.h	(revision 0)
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Copyright (c) 2009, Google Inc. All rights reserved.
> 
> 2010
done

> 
> > + * 
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions are
> > + * met:
> > + * 
> > + *     * Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> > + * copyright notice, this list of conditions and the following disclaimer
> > + * in the documentation and/or other materials provided with the
> > + * distribution.
> > + *     * Neither the name of Google Inc. nor the names of its
> > + * contributors may be used to endorse or promote products derived from
> > + * this software without specific prior written permission.
> > + * 
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#include "config.h"
> 
> This only goes in the .cpp files.
> 
> > +#include "PlatformString.h"
> 
> Alpha order.
done

> 
> > +#include "Geolocation.h"
> > +#include "GeolocationService.h"
> > +#include "Geoposition.h"
> > +#include "PositionError.h"
> > +
> > +namespace WebCore {
> > +
> > +// Provides an interface for GeolocationServiceChromium to call into chromium.
> > +class GeolocationServiceBridge {
> > +public:
> > +    // Called by GeolocationServiceChromium.
> > +    virtual bool startUpdating(PositionOptions*) = 0;
> > +    virtual void stopUpdating() = 0;
> > +    virtual void suspend() = 0;
> > +    virtual void resume() = 0;
> > +
> > +    // Called by the Chromium side, to identify this bridge..
> > +    virtual int getBridgeId() const = 0;
> > +};
> > +
> > +// This class extends GeolocationService, and uses GeolocationServiceBridge to
> > +// call into chromium, as well as provides a few extra methods so that we can
> > +// be notified of permission, position, error, etc, from chromium.
> > +class GeolocationServiceChromium : public GeolocationService {
> > +public:
> > +    explicit GeolocationServiceChromium(GeolocationServiceClient* c);
> 
> Delete c.
done

> 
> > +
> > +    GeolocationServiceBridge* geolocationServiceBridge() const { return m_geolocationServiceBridge.get(); }
> > +    void setIsAllowed(bool allowed);
> > +    void setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp);
> > +    void setLastError(int error_code, const String& message);
> > +    Frame* frame();
> > +
> > +    // From GeolocationService.
> > +    virtual bool startUpdating(PositionOptions*);
> > +    virtual void stopUpdating();
> > +    virtual void suspend();
> > +    virtual void resume();
> > +    virtual Geoposition* lastPosition() const;
> > +    virtual PositionError* lastError() const;
> > +
> > +private:
> > +    Geolocation* m_Geolocation;
> > +    OwnPtr<GeolocationServiceBridge*> m_geolocationServiceBridge;    
> 
> Why is this an OwnPtr to a pointer?
my bad, removed.

> 
> > +    RefPtr<Geoposition> m_LastPosition;
> > +    RefPtr<PositionError> m_LastError;
> > +
> > +    typedef GeolocationServiceBridge* (BridgeFactoryFunction)(GeolocationServiceChromium*);
> > +    static BridgeFactoryFunction* s_bridgeFactoryFunction;
> 
> Get rid of.  Use ChromiumBridge.
done.
> 
> > +};
> > +
> > +} // namespace WebCore
> > 
> > Property changes on: platform/chromium/GeolocationServiceChromium.h
> > ___________________________________________________________________
> > Added: svn:eol-style
> >    + LF
> >

[Jeremy Orlow]

Comment on attachment 48649 [details]
Geolocation implementation

r=me

[WebKit Commit Bot]

Comment on attachment 48649 [details]
Geolocation implementation

Clearing flags on attachment: 48649

Committed r54883: <http://trac.webkit.org/changeset/54883>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48649 [details]
Geolocation implementation

> Index: WebKit/chromium/public/GeolocationServiceBridgeChromium.h

This file is incorrectly named.  The files in the public directory
should be prefixed with Web*.  Why is there a Chromium suffix?


> +#ifndef GeolocationServiceBridgeChromium_h
> +#define GeolocationServiceBridgeChromium_h
> +
> +namespace WebCore {
> +class GeolocationServiceBridge;
> +class GeolocationServiceChromium;
> +}
> +
> +namespace WebKit {
> +
> +class WebString;
> +class WebURL;
> +
> +// Provides a WebKit API called by the embedder.
> +class WebGeolocationServiceBridge {
> +public:
> +    virtual void setIsAllowed(bool allowed) = 0;
> +    virtual void setLastPosition(double latitude, double longitude, bool providesAltitude, double altitude, double accuracy, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, long long timestamp) = 0;

This function is begging to have its parameters expressed using a struct.
It is bad form to write functions with so many named arguments since it
is really hard to tell at the call site if you get the order of the
arguments wrong.


> +    virtual void setLastError(int errorCode, const WebString& message) = 0;
> +};
> +
> +// Provides an embedder API called by WebKit.
> +class WebGeolocationServiceInterface {
> +public:
> +    virtual void requestPermissionForFrame(int bridgeId, const WebURL& url) = 0;
> +    virtual void startUpdating(int bridgeId, bool hasHighAccuracy) = 0;
> +    virtual void stopUpdating(int bridgeId) = 0;
> +    virtual void suspend(int bridgeId) = 0;
> +    virtual void resume(int bridgeId) = 0;
> +
> +    // Attaches the GeolocationBridge to the embedder and returns its id, which
> +    // should be used on subsequent calls for the methods above.
> +    virtual int attachBridge(WebKit::WebGeolocationServiceBridge* geolocationServiceBridge) = 0;
> +
> +    // Dettaches the GeolocationService from the embedder.
> +    virtual void dettachBridge(int bridgeId) = 0;
> +};

There should be a separate header file for each type.


> +WebCore::GeolocationServiceBridge* createGeolocationServiceBridgeImpl(WebCore::GeolocationServiceChromium*);

Why is the public WebKit API exposing functions to create
WebCore types?  At the very least this should be wrapped
with a WEBKIT_IMPLEMENTATION macro.


> Index: WebKit/chromium/public/WebViewClient.h
> ===================================================================
> --- WebKit/chromium/public/WebViewClient.h	(revision 54724)
> +++ WebKit/chromium/public/WebViewClient.h	(working copy)
> @@ -46,6 +46,7 @@ class WebAccessibilityObject;
>  class WebDragData;
>  class WebFileChooserCompletion;
>  class WebFrame;
> +class WebGeolocationServiceInterface;
>  class WebNode;
>  class WebNotificationPresenter;
>  class WebRange;
> @@ -277,6 +278,11 @@ public:
>      virtual void removeAutofillSuggestions(const WebString& name,
>                                             const WebString& value) { }
>  
> +    // Geolocation ---------------------------------------------------------
> +
> +    // Access the embedder API for geolocation services.
> +    virtual WebKit::WebGeolocationServiceInterface* getGeolocationService() { return 0; }

Normally, we do not use the suffix Interface.  Also, simple getter
functions should not be prefixed with "get", and you do not need
to use the WebKit:: prefix when you are inside the WebKit namespace.

This should probably be:

       virtual WebGeolocationService* geolocationService() { return 0; }


> Index: WebKit/chromium/src/ChromeClientImpl.cpp
...
> +void ChromeClientImpl::requestGeolocationPermissionForFrame(Frame* frame, Geolocation* geolocation)
> +{
> +    GeolocationServiceChromium* geolocationService = reinterpret_cast<GeolocationServiceChromium*>(geolocation->getGeolocationService());
> +    m_webView->client()->getGeolocationService()->requestPermissionForFrame(geolocationService->geolocationServiceBridge()->getBridgeId(), frame->document()->url());

Note: the WebViewClient can be null!


> Index: WebKit/chromium/src/GeolocationServiceBridgeChromium.cpp
...
> +GeolocationServiceBridgeImpl::~GeolocationServiceBridgeImpl()
> +{
> +    WebKit::WebViewClient* webViewClient = getWebViewClient();
> +    // Geolocation has an OwnPtr to us, and it's destroyed after the frame has
> +    // been potentially disconnected. In this case, it calls stopUpdating()
> +    // has been called and we have already dettached ourselves.
> +    if (!webViewClient) {
> +        ASSERT(!m_bridgeId);
> +    } else if (m_bridgeId)

nit: no brackets around single line statements


> +WebViewClient* GeolocationServiceBridgeImpl::getWebViewClient()
> +{
> +    Frame* frame = m_GeolocationServiceChromium->frame();
> +    if (!frame || !frame->page())
> +        return 0;
> +    WebKit::ChromeClientImpl* chromeClientImpl = static_cast<WebKit::ChromeClientImpl*>(frame->page()->chrome()->client());
> +    WebKit::WebViewClient* webViewClient = chromeClientImpl->webView()->client();
> +    return webViewClient;
> +}

nit: no need for WebKit:: prefix when you are in the WebKit namespace

[Marcus Bulach]

Darin, thanks for the comments!

I addressed all of them at https://bugs.webkit.org/show_bug.cgi?id=36535
would you mind taking a look please?

Thanks,
Marcus