[Webkit-unassigned] [Bug 113499] WebCore refactoring of device motion/orientation and fixing of webkit ports

[bugzilla-daemon at webkit.org]

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





--- Comment #41 from Alexey Proskuryakov <ap at webkit.org>  2013-03-28 09:29:57 PST ---
(From update of attachment 195564)
View in context: https://bugs.webkit.org/attachment.cgi?id=195564&action=review

What is the goal of this refactoring?

I realize that this is not marked for review yet, but too much in this patch looks questionable.

> Source/WebCore/dom/DeviceMotionController.cpp:43
> +DeviceMotionController* DeviceMotionController::getInstance()

This is wrong style - getXXX functions in WebKit return their results by reference as out arguments.

The common name for this is shared(), and the common way to implement is through DEFINE_STATIC_LOCAL macro.

> Source/WebCore/dom/DeviceMotionController.cpp:51
> +void DeviceMotionController::didChangeDeviceMotion(PassRefPtr<DeviceMotionData> deviceMotionData)

There is no ownership passing here, so it's incorrect to use PassRefPtr. A plain pointer should be used.

The purpose of PassRefPtr is to avoid reference count thrashing. It also gives a little protection against leaks when returning results from functions. It's not the goal to use it everywhere.

> Source/WebCore/dom/DeviceMotionController.cpp:60
> +    // TODO(timvolodine): check how to implement this properly

Are you planning to get rid of this before sending the patch for review? This is not a proper style for FIXME comments in WebKit.

> Source/WebCore/dom/DeviceOrientationController.cpp:54
> +void DeviceOrientationController::didChangeDeviceOrientation(PassRefPtr<DeviceOrientationData> orientation)

Ditto about PassRefPtr.

> Source/WebCore/dom/DeviceOrientationController.cpp:57
> +    LOG_ERROR(WTF::String::number(orientation.get()->alpha()).ascii().data());
> +    LOG_ERROR(WTF::String::number(orientation.get()->canProvideGamma()).ascii().data());

Please don't prefix WTF::.

> Source/WebCore/dom/DeviceOrientationController.cpp:59
> +    // orientation = InspectorInstrumentation::overrideDeviceOrientation(m_page, orientation);

Please remove commented out code before sending a patch for review.

> Source/WebCore/dom/DeviceOrientationController.cpp:66
> +    // TODO(timvolodine): figure out how to implement this
> +    // maybe : (m_lastOrientation.get() != 0);

...

> Source/WebCore/dom/DeviceOrientationController.cpp:70
>  PassRefPtr<Event> DeviceOrientationController::getLastEvent()

Existing code, but this is very wrong, as explained in above comments.

> Source/WebCore/page/DOMWindow.h:411
> +        Page* page();

Did you check why this function was private? I suspect that another idiom could be preferred for getting the page.

> Source/WebCore/page/DeviceController.h:41
> +    explicit DeviceController();

"Explicit" is meaningless with this constructor, now that it has no arguments.

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