[Webkit-unassigned] [Bug 89197] [Chromium] Implemented DeviceMotion

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 15 07:35:11 PDT 2012


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





--- Comment #5 from Hans Wennborg <hans at chromium.org>  2012-06-15 07:35:09 PST ---
(From update of attachment 147796)
View in context: https://bugs.webkit.org/attachment.cgi?id=147796&action=review

> Source/WebCore/ChangeLog:3
> +        [Chromium] Implemented DeviceMotion

s/Implemented/Implement/

I wonder if we could break this patch up into smaller peaces. Perhaps we could do the mock object separately? But I guess we need all the wrapper stuff to actually use it, though :/

> Source/WebCore/platform/mock/DeviceMotionClientMock.h:48
> +    virtual void setController(DeviceMotionController*);

a good idea when adding new code is to add the OVERRIDE specifier for virtual functions that are supposed to override functions in the base class. That way, the compiler will give an error if the base class function changes and the function doesn't override anymore.

> Source/WebKit/chromium/public/WebDeviceMotion.h:23
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

this copyright blurb is slightly different than the previous ones in the patch. we should figure out which one is the best one and use the same throughout the patch

> Source/WebKit/chromium/public/WebDeviceMotion.h:36
> +class WebDeviceMotion {

why not WebDeviceMotionData?

> Source/WebKit/chromium/public/WebDeviceMotion.h:66
> +    bool isNull() { return m_isNull; }

this, and all other getters in the function should be const-qualified

> Source/WebKit/chromium/public/WebDeviceMotion.h:98
> +    {

this function could be on a single line; this goes for the other one-statement functions below as well. WebKit doesn't have an 80-col limit.

> Source/WebKit/chromium/public/WebDeviceMotion.h:167
> +    WebDeviceMotion(const WTF::PassRefPtr<WebCore::DeviceMotionData>&);

i wonder if this should be just WebDeviceMotion(const WebCore::DeviceMotionData&);

> Source/WebKit/chromium/public/WebDeviceMotion.h:168
> +    WebDeviceMotion& operator=(const WTF::PassRefPtr<WebCore::DeviceMotionData>&);

ditto. const-ref to a PassRef seems unusual; i don't remember if there's a good reason for it

> Source/WebKit/chromium/public/WebDeviceMotionClient.h:38
> +    virtual void setController(WebDeviceMotionController*) = 0;

for interfaces which will be implemented by chromium-side classes, we don't use pure virtual functions anymore, they should be defined like this:

virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); }

This makes it easier to make changes to the classes in the future, without getting compile errors about pure virtuals not being implemented.

> Source/WebKit/chromium/public/WebDeviceMotionClientMock.h:42
> +    virtual void setController(WebDeviceMotionController*);

these could be marked OVERRIDE

> Source/WebKit/chromium/public/WebViewClient.h:332
> +    virtual WebDeviceMotionClient* deviceMotionClient() { return 0; }

we could probably put this in the Device Orientation section below, since they're part of the same API

> Source/WebKit/chromium/src/DeviceMotionClientProxy.h:47
> +    void setController(WebCore::DeviceMotionController*);

since these are overriding DeviceMotionClient, they should be marked "virtual" and "OVERRIDE"

> Source/WebKit/chromium/src/WebDeviceMotion.cpp:80
> +    const WebCore::DeviceMotionData::Acceleration* accelerationIncludingGravity =

no need for linebreak

> Source/WebKit/chromium/src/WebDeviceMotionController.cpp:38
> +    m_controller->didChangeDeviceMotion(static_cast<PassRefPtr<WebCore::DeviceMotionData> >(motion).get());

another way of avoiding the static_cast would be

RefPtr<WebCore::DeviceMotionData> deviceMotionData = PassRefPtr<WebCore::DeviceMotionData>(motion);
m_controller->didChangeDeviceMotion(deviceMotionData.get());

> Source/WebKit/chromium/src/WebDeviceOrientationController.cpp:38
> +    m_controller->didChangeDeviceOrientation(static_cast<PassRefPtr<WebCore::DeviceOrientation> >(orientation).get());

same here, the trick with a local RefPtr is nicer than the static_cast (sorry, I should have mentioned that earlier). Also, this one could be a separate patch.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:45
> +#include "WebDeviceMotionClientMock.h"

yeah, we looked at this and couldn't figure out any ordering that the style script would accept :/

> Tools/DumpRenderTree/chromium/LayoutTestController.h:360
> +    void setMockDeviceMotion(const CppArgumentList&, CppVariant*);

maybe just put it in device orientation's section below

> Tools/DumpRenderTree/chromium/WebViewHost.h:190
> +    virtual WebKit::WebDeviceMotionClient* deviceMotionClient();

let's mark it OVERRIDE while we're here

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