[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