[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