[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