[Webkit-unassigned] [Bug 113499] WebCore refactoring of device motion/orientation and fixing of webkit ports
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 28 09:31:48 PDT 2013
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.
More information about the webkit-unassigned
mailing list