[webkit-reviews] review denied: [Bug 32068] Implement Chromium Geolocation Sevice (GeolocationServiceChromium) : [Attachment 48592] WebKit bits
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 12 03:07:17 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 48592: WebKit bits
https://bugs.webkit.org/attachment.cgi?id=48592&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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()->getB
ridgeId(),
> + 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(GeolocationServiceC
hromium* 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.
More information about the webkit-reviews
mailing list