https://bugs.webkit.org/show_bug.cgi?id=89197
Adam Barth <abarth at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #158834|review? |review-
Flag| |
--- Comment #50 from Adam Barth <abarth at webkit.org> 2012-08-16 11:41:34 PST ---
(From update of attachment 158834)
View in context: https://bugs.webkit.org/attachment.cgi?id=158834&action=review
I haven't reviewed the tests yet. It looks like you've also got a compile failure on apple-mac.
> Source/Platform/chromium/public/WebDeviceMotionData.h:38
> + WebDeviceMotionData();
Does this need WEBKIT_EXPORT ?
> Source/Platform/chromium/public/WebDeviceMotionData.h:46
> + static WebDeviceMotionData nullMotion() { return WebDeviceMotionData(); }
What's the point of this API? It seems redundant with the default constructor.
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:39
> +DeviceMotionDetectorInternal::DeviceMotionDetectorInternal()
DeviceMotionDetectorInternal -> What does "Internal" mean in this name? Is there a non-internal version? If not, we should probably just call this class DeviceMotionDetector....
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:54
> + m_controller = controller;
Four-space indent
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:59
> + m_detector = WebKit::Platform::current()->deviceMotionDetector();
Is this always non-0? If not, we should probably add a null check.
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:83
> + // Our lifetime is bound to the WebViewImpl.
There is no such thing as a WebViewImpl in WebCore. Do you mean the Page? Do other ports use deviceMotionControllerDestroyed? It seems like we should use the same mechanism for managing the lifetime of these classes across ports.
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:88
> + if (m_controller) {
Prefer early return.
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:91
> + if (!deviceMotionData)
> + deviceMotionData = DeviceMotionData::create();
Can this happen? Why can this function be called with a null pointer but m_controller->didChangeDeviceMotion cannot?
> Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.h:62
> + WebKit::WebDeviceMotionDetector* m_detector;
How is the lifetime of this object managed? Do we even need to store a pointer to this object? It seems like we can just get it from the Platform whenever we like.
> Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:50
> +void WebDeviceMotionData::initializeAcceleration(bool canProvideAccelerationX, double accelerationX, bool canProvideAccelerationY, double accelerationY, bool canProvideAccelerationZ, double accelerationZ)
Should this be called setAcceleration instead? Is it legal to call this function more than once? If not, we should ASSERT that we don't do that. If so, we should change the name to "set" rather than "initialize".
> Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:55
> + m_private->setAcceleration(acceleration.release());
Notice that the WebCore function name is "set". We like to align the API names with the WebCore names to cut down on confusion whenever possible.
> Source/WebKit/chromium/src/WebViewImpl.h:840
> + // If we attempt to fetch the on-screen GraphicsContext3D before
> + // the compositor has been turned on, we need to instantiate it
> + // early. This member holds on to the GC3D in this case.
> + OwnPtr<WebGraphicsContext3D> m_temporaryOnscreenGraphicsContext3D;
This seems unrelated to device motion.
> Source/WebKit/chromium/src/WebViewImpl.h:841
> + OwnPtr<WebCore::DeviceMotionDetectorInternal> m_deviceMotionDetectorInternal;
It seems strange to own a WebCore object here. Perhaps DeviceMotionDetectorInternal should be in the WebKit layer rather than WebCore? It implements DeviceMotionClient, which is a WebCore client. Typically, we implement WebCore clients in the WebKit layer, not in the WebCore layer. You can see there are a bunch of FooClientImpl objects here. Perhaps DeviceMotionDetectorInternal should be called DeviceMotionClientImpl? It's not entirely clear to me.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.