[webkit-reviews] review denied: [Bug 32068] Implement Chromium Geolocation Sevice (GeolocationServiceChromium) : [Attachment 48591] WebCore bits
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 12 03:36:52 PST 2010
Jeremy Orlow <jorlow at chromium.org> has denied review:
Bug 32068: Implement Chromium Geolocation Sevice (GeolocationServiceChromium)
https://bugs.webkit.org/show_bug.cgi?id=32068
Attachment 48591: WebCore bits
https://bugs.webkit.org/attachment.cgi?id=48591&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
> 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(GeolocationServiceClien
t* 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
>
More information about the webkit-reviews
mailing list