[webkit-reviews] review denied: [Bug 49735] [Chromium] Introduce wrapper types for WebCore::GeolocationError, WebCore::GeolocationPosition : [Attachment 74558] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 22 16:15:33 PST 2010
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Knottenbelt
<jknotten at chromium.org>'s request for review:
Bug 49735: [Chromium] Introduce wrapper types for WebCore::GeolocationError,
WebCore::GeolocationPosition
https://bugs.webkit.org/show_bug.cgi?id=49735
Attachment 74558: Patch
https://bugs.webkit.org/attachment.cgi?id=74558&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74558&action=review
> WebKit/chromium/public/WebGeolocationError.h:43
> + enum ErrorCode {
Sorry, I should have pointed this out in the previous review, but in the
WebKit API enum names should be formatted like this:
enum Foo {
FooA,
FooB
};
In this case, I would use:
enum Error {
ErrorPermissionDenied,
ErrorPositionUnavailable
};
I'd recommend asserting with a compile-time assert that these enum values
match the corresponding WebCore types by adding to AssertMatchingEnums.cpp.
That way, in the implementation code, you can just static_cast between
the WebKit and WebCore enum types.
> WebKit/chromium/public/WebGeolocationPosition.h:41
> + WebGeolocationPosition() {};
nit: no trailing ";"
> WebKit/chromium/public/WebGeolocationPosition.h:58
> +
nit: delete extra new line, leave only one between #endif and "private:" label.
> WebKit/chromium/src/WebGeolocationError.cpp:51
>
+WebGeolocationError::WebGeolocationError(WTF::PassRefPtr<WebCore::GeolocationE
rror> error)
nit: no need for WTF:: or WebCore:: prefixes in this file.
> WebKit/chromium/src/WebGeolocationError.cpp:56
> +WebGeolocationError&
WebGeolocationError::operator=(WTF::PassRefPtr<WebCore::GeolocationError>
error)
ditto
> WebKit/chromium/src/WebGeolocationError.cpp:62
> +WebGeolocationError::operator WTF::PassRefPtr<WebCore::GeolocationError>()
const
ditto
> WebKit/chromium/src/WebGeolocationPosition.cpp:50
> +WebGeolocationPosition&
WebGeolocationPosition::operator=(WTF::PassRefPtr<WebCore::GeolocationPosition>
position)
same nit about dropping WTF:: and WebCore::
More information about the webkit-reviews
mailing list