[Webkit-unassigned] [Bug 43258] Implement chromium WebDeviceOrientationClient wrapper and have WebViewImpl get it from WebViewClient.

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from hans at chromium.org  2010-08-09 08:43:38 PST ---
(In reply to comment #21)
> (From update of attachment 63884 [details])
> WebKit/chromium/public/WebDeviceOrientation.h:49
>  +      static WebDeviceOrientation nullOrientation() { return WebDeviceOrientation(); }
> Since you're using (), C++ guarantees us that everything is 0'ed out?
No, WebDeviceOrientation() is the private constructor that sets m_isNull=true and leaves the rest uninitialized. I figured the values of the other members doesn't matter when m_isNull is true.

> WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
>  +      void setController(WebDeviceOrientationController*);
> Either virtual of WEBKIT_API
It is virtual since it's inherited from WebDeviceOrientationClient.

> WebKit/chromium/public/WebDeviceOrientationClientMock.h:49
>  +      void initialize();
> WEBKIT_API
Done. (Also for reset()).

> WebKit/chromium/public/WebDeviceOrientationClientMock.h:52
>  +      WebCore::DeviceOrientationClientMock* m_clientMock;
> I can't remember if this is allowed or not.  We might prefer a void* and casting...see if you can find other examples (like in PrivatePtr).
I can't find any classes in WebKit/chromium/public that have a void* member, and WebPrivatePtr<T> uses a T* member so I hope it's ok.

> WebKit/chromium/src/WebDeviceOrientation.cpp:70
>  +          return PassRefPtr<WebCore::DeviceOrientation>(0);
> Can't you just say "return 0;" ?
Done.

> WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:53
>  +      return WebDeviceOrientation(lastOrientation);
> Why store it to the temp variable...doesn't seem to give you much....and why do you need to cast it explicitly either?  It should just work.
Removing the temp variable.
I need to use the WebDeviceOrientation constructor explicitly. C++ will not see that it first needs to convert DeviceOrientation* to PassRefPtr<DeviceOrientation> which it can then use to construct a WebDeviceOrientation.

> 
> WebKit/chromium/src/WebViewImpl.cpp:290
>  +      if (WebRuntimeFeatures::isDeviceOrientationEnabled())
> Can't you do this either way?  Worst case, it gets set to null which is what would have happened anyway.
You're right. Done.

> Minor comments so r=me, but you're not a committer so you'll need to post another rev.  Let me know if you only address stuff I mention so I can avoid yet another review of the whole thing.


Uploading rebased patch addressing the stuff above (and nothing else).

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