[Webkit-unassigned] [Bug 49735] [Chromium] Introduce wrapper types for WebCore::GeolocationError, WebCore::GeolocationPosition

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49735


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74558|review?                     |review-
               Flag|                            |




--- Comment #7 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-11-22 16:15:34 PST ---
(From update of attachment 74558)
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::GeolocationError> 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::

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.