[webkit-reviews] review denied: [Bug 43258] Implement chromium WebDeviceOrientationClient wrapper and have WebViewImpl get it from WebViewClient. : [Attachment 63312] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied 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 63312: Patch
https://bugs.webkit.org/attachment.cgi?id=63312&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebKit/chromium/src/WebDeviceOrientationClientDummy.cpp:37
 +	DeviceOrientationClientDummy() :
orientation(WebCore::DeviceOrientation::create()) { }
usually we split onto 2 lines right before the :

WebKit/chromium/public/WebDeviceOrientation.h:32
 +  #include <wtf/PassRefPtr.h>
You can just do "namespace WTF { template <typename T> class PassRefPtr; }"

WebKit/chromium/public/WebDeviceOrientation.h:43
 +	    initialize(canProvideAlpha, alpha, canProvideBeta, beta,
canProvideGamma, gamma);
Darin, in the past you've been pretty adamant that constructors be inline.  Do
you think that doing it this way is better than the alternative?  I'm not so
sure.

WebKit/chromium/public/WebDeviceOrientation.h:48
 +	operator PassRefPtr<WebCore::DeviceOrientation>() const;
add the other 2 operators we have in just about every other class like this

WebKit/chromium/public/WebDeviceOrientationClient.h:50
 +	WebDeviceOrientationClient() { }
This works??  I'm pretty sure this will need to be public and virtual.

WebKit/chromium/src/WebViewImpl.cpp:267
 +	,
m_webDeviceOrientationClient(WebRuntimeFeatures::isDeviceOrientationEnabled() ?
(client ? client->createWebDeviceOrientationClient() :
WebDeviceOrientationClientDummy::create()) : 0)
What is the dummy for?	I don't think client can ever be non-null.  And, if it
can be, then we should just assume the device orientation client is null too
then.

I don't see any point to the dummy.  I'd rather not add logic to WebKit to
handle the client and DOclient being null unless there's already precedent. 
But even if so, I think that's better than having some dummy client.

Another option, if all else fails, you could just allocate the mock.


hmmm...lets just talk in person