[webkit-reviews] review denied: [Bug 43258] Implement chromium WebDeviceOrientationClient wrapper and have WebViewImpl get it from WebViewClient. : [Attachment 63717] 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 63717: Patch
https://bugs.webkit.org/attachment.cgi?id=63717&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebKit/chromium/src/WebViewImpl.cpp:290
 +	    m_deviceOrientationClientProxy.set(new
DeviceOrientationClientProxy(client ? client->deviceOrientationClient() : 0));
See if you can clean up this logic a bit.  It's kinda weird how we have this
matrix of possibilities between whether the flag is on and whether there's a
client.  

WebKit/chromium/src/WebDeviceOrientationController.cpp:31
 +  #include "PassRefPtr.h"
<wtf/PassRefPtr.h>

WebKit/chromium/src/WebDeviceOrientationController.cpp:36
 +  void WebDeviceOrientationController::didChangeDeviceOrientation(const
WebDeviceOrientation& o)
use a better variable name

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:68
 +	delete m_clientMock;
and set it to 0...and first do an if to see if it's already 0

and maybe you should just make a class much like PrivatePtr...make that
PrivateRefPtr and then add PrivateOwnPtr?  Just a thought.

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:63
 +	m_clientMock = new WebCore::DeviceOrientationClientMock();
Hmmm...i'm tempted to have this call reset() but I guess the constructor
doesn't set it to 0 before calling it...so that's not possible.  I guess leave
it as is.

WebKit/chromium/src/WebDeviceOrientationClientMock.cpp:53
 +	return lastOrientation ? new WebDeviceOrientation(lastOrientation) : 0;

Hmmm....this should probably just pass back the object, not a pointer to it. 
But the problem is that there might not be a last orientation.	Hmmmm....maybe
you shoudl add an invalid/null state to the WebDO class?  This is what we've
done for serializedScriptValue and a couple others where a null refPtr has
semantic meaning.  Not sure about this, but I think it's better than passing
around pointers when we don't absolutely have to.

WebKit/chromium/src/WebDeviceOrientation.cpp:49
 +	m_canProvideGamma = o->canProvideGamma();
what about the actual gamma value?

WebKit/chromium/src/WebDeviceOrientation.cpp:39
 +	      m_canProvideGamma(o->canProvideGamma())
ditto

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:32
 +  #include "RefPtr.h"
ditto ditto ditto

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:44
 +	if (!m_client)
add fixme about getting rid of these null checks once it's enabled by default

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:68
 +	OwnPtr<WebDeviceOrientation>
lastWebOrientation(m_client->lastOrientation());
Hmmm...this probably needs to be a refPtr in WebCore.

WebKit/chromium/src/DeviceOrientationClientProxy.cpp:72
 +	m_lastOrientation = *lastWebOrientation;
Why are we caching it here?  I guess because it was an own ptr?  Well, we
should def make it ref.

WebKit/chromium/public/WebDeviceOrientation.h:39
 +		: m_canProvideAlpha(canProvideAlpha),
too many spaces in

WebKit/chromium/public/WebDeviceOrientation.h:46
 +	}
You probably should add methods to this for getting the values.