[webkit-reviews] review granted: [Bug 46344] Update DeviceMotionEvent to spec : [Attachment 68508] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 46344: Update DeviceMotionEvent to spec
https://bugs.webkit.org/show_bug.cgi?id=46344

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68508&action=review

> WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:38
> +namespace {

Why not just make these static functions?

> WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:40
> +RefPtr<DeviceMotionData::Acceleration> readAccelerationArgument(JSValue
value, ExecState* exec)

This should return a PassRefPtr

> WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:52
> +    double x = static_cast<double>(xValue.toNumber(exec));

alphaValue.toNumber() returns a double. No need to do any casting here.

> WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:78
> +RefPtr<DeviceMotionData::RotationRate> readRotationRateArgument(JSValue
value, ExecState* exec)

This should return a PassRefPtr

> WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:119
> +    object->putDirect(Identifier(exec, stringToUString("x")),
acceleration->canProvideX() ? jsNumber(exec, acceleration->x()) : jsNull());

No need for stringToUString; see Identifier(ExecState* exec, const char* s)

> WebCore/dom/DeviceMotionData.cpp:85
> +    : m_acceleration()
> +    , m_accelerationIncludingGravity()
> +    , m_rotationRate()

No need to initialize these.

> WebCore/dom/DeviceMotionData.h:62
> +	   bool m_canProvideX;
> +	   bool m_canProvideY;
> +	   bool m_canProvideZ;
> +
> +	   double m_x;
> +	   double m_y;
> +	   double m_z;

Put the bools after the doubles to minimize padding.

> WebCore/dom/DeviceMotionData.h:90
> +	   bool m_canProvideAlpha;
> +	   bool m_canProvideBeta;
> +	   bool m_canProvideGamma;
> +
> +	   double m_alpha;
> +	   double m_beta;
> +	   double m_gamma;

Ditto