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

Mon Jun 18 08:52:15 PDT 2012

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

Amy Ousterhout <aousterh at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[Chromium] Implemented      |[Chromium] Implements
                   |DeviceMotion                |DeviceMotion

--- Comment #8 from Amy Ousterhout <aousterh at chromium.org>  2012-06-18 08:52:13 PST ---

(In reply to comment #5)
> (From update of attachment 147796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147796&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [Chromium] Implemented DeviceMotion
> 
> s/Implemented/Implement/

Done.

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

Done. See https://bugs.webkit.org/show_bug.cgi?id=89220 for the Client Mock in WebCore, and bugs 89337 and 89339 for other small changes that can be done separately.

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

Done (see bug 89220).

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

It looks like several different copyright blurbs are commonly used within WebKit. For DeviceOrientation, one copyright blurb is always used in Source/WebCore/dom and another in Source/WebKit/chromium, so I did the same for DeviceMotion. The main difference between the blurbs is that in WebCore, it says the software is provided by "apple and its contributors" whereas in chromium it says by "the copyright holders". Is it possible that this distinction was intentional?

> > Source/WebKit/chromium/public/WebDeviceMotion.h:36
> > +class WebDeviceMotion {
> 
> why not WebDeviceMotionData?

I wanted the name to be consistent with WebDeviceOrientation, but I guess it's more important for it to be consistent with the rest of the DeviceMotion implementation than with the corresponding part of the DeviceOrientation implementation. I changed it to 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

Done.

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

Done.

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

It does seem overly-complicated in its current state. However the parameter does need to be some sort of pointer, because WebDeviceMotionData has an "isNull" field, whereas DeviceMotionData does not, and a null pointer is the easiest way to represent a null DeviceMotionData. Thus I changed it to WebDeviceMotion(const WebCore::DeviceMotionData *).

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

Done.

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

Done.

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

Done.

> > Source/WebKit/chromium/src/DeviceMotionClientProxy.h:47
> > +    void setController(WebCore::DeviceMotionController*);
> 
> since these are overriding DeviceMotionClient, they should be marked "virtual" and "OVERRIDE"

Done.

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

Done.

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

Done. See bug 89337 for the change in WebDeviceOrientationController.

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

Done.

> > Tools/DumpRenderTree/chromium/WebViewHost.h:190
> > +    virtual WebKit::WebDeviceMotionClient* deviceMotionClient();
> 
> let's mark it OVERRIDE while we're here

Done.

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