[webkit-reviews] review denied: [Bug 46895] [Chromium] Implement mocks for client-based geolocation : [Attachment 74915] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 29 08:50:57 PST 2010


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Knottenbelt
<jknotten at chromium.org>'s request for review:
Bug 46895: [Chromium] Implement mocks for client-based geolocation
https://bugs.webkit.org/show_bug.cgi?id=46895

Attachment 74915: Patch
https://bugs.webkit.org/attachment.cgi?id=74915&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74915&action=review

What is the master plan for ENABLE_CLIENT_BASED_GEOLOCATION?  Will we be
deleting the "else" code in the near future?

> WebKit/chromium/public/WebGeolocationClientMock.h:70
> +    WEBKIT_API void initialize();

nit: looks like 'initialize' does not need to be exported from webkit.dll since

it is only called by a private constructor.  i'd probably just get rid of the
initialize method, and move its implementation into the constructor.

> WebKit/chromium/src/WebGeolocationClientMock.cpp:54
> +void WebGeolocationClientMock::initialize()

nit: please implement the methods in the same order in which they are declared
in the header file.

> WebKitTools/DumpRenderTree/chromium/WebViewHost.h:321
> +    OwnPtr<WebKit::WebGeolocationClientMock> m_webGeolocationClientMock;

nit: for consumers of the WebKit API, it is conventional to drop the "web"
prefix
on variable names.  that's why we have m_geolocationServiceMock and not
m_webGeolocationServiceMock.


More information about the webkit-reviews mailing list