[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 #46 from timvolodine at chromium.org  2013-04-02 10:41:15 PST ---
(From update of attachment 195564)
View in context: https://bugs.webkit.org/attachment.cgi?id=195564&action=review

Thanks Alexey, I addressed most of your comments, so now the code should look better.

>> 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.

done

>> 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.

done

>> 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.

done

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

done

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

done

>> Source/WebCore/dom/DeviceOrientationController.cpp:59
>> +    // orientation = InspectorInstrumentation::overrideDeviceOrientation(m_page, orientation);
> 
> Please remove commented out code before sending a patch for review.

done

>> Source/WebCore/dom/DeviceOrientationController.cpp:66
>> +    // maybe : (m_lastOrientation.get() != 0);
> 
> ...

done

>> Source/WebCore/dom/DeviceOrientationController.cpp:70
>>  PassRefPtr<Event> DeviceOrientationController::getLastEvent()
> 
> Existing code, but this is very wrong, as explained in above comments.

don't really see why this is wrong now

>> 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.

hmm, at this point not sure how to get to the page from window in any other way.

>> Source/WebCore/page/DeviceController.h:41
>> +    explicit DeviceController();
> 
> "Explicit" is meaningless with this constructor, now that it has no arguments.

done

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