[webkit-reviews] review granted: [Bug 43258] Implement chromium WebDeviceOrientationClient wrapper and have WebViewImpl get it from WebViewClient. : [Attachment 63884] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 9 07:21:35 PDT 2010
Jeremy Orlow <jorlow at chromium.org> has granted hans at chromium.org's request for
review:
Bug 43258: Implement chromium WebDeviceOrientationClient wrapper and have
WebViewImpl get it from WebViewClient.
https://bugs.webkit.org/show_bug.cgi?id=43258
Attachment 63884: Patch
https://bugs.webkit.org/attachment.cgi?id=63884&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebKit/chromium/public/WebDeviceOrientation.h:49
+ static WebDeviceOrientation nullOrientation() { return
WebDeviceOrientation(); }
Since you're using (), C++ guarantees us that everything is 0'ed out?
WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
+ void setController(WebDeviceOrientationController*);
Either virtual of WEBKIT_API
WebKit/chromium/public/WebDeviceOrientationClientMock.h:49
+ void initialize();
WEBKIT_API
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).
WebKit/chromium/src/WebDeviceOrientation.cpp:70
+ return PassRefPtr<WebCore::DeviceOrientation>(0);
Can't you just say "return 0;" ?
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.
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.
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.
More information about the webkit-reviews
mailing list