[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 #19 from hans at chromium.org  2010-08-09 04:28:23 PST ---
(In reply to comment #18)
> (From update of attachment 63717 [details])
> 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.  
Done. Always initializing m_deviceOrientationClientProxy, but only passing it on to Page is the runtime flag is set.

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

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

> 
> 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
Setting it to 0 after delete. Not sure why I'd need an if before delete? delete 0 just does nothing.

> 
> and maybe you should just make a class much like PrivatePtr...make that PrivateRefPtr and then add PrivateOwnPtr?  Just a thought.
Filed Bug 43709 to do that, but I don't think this patch needs to block on it?

> 
> 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.
OK.

> 
> 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.
Adding null state to WebDeviceOrientation and having WebDOClient::lastOrientation() return an object rather than a pointer.

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

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

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

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

> 
> WebKit/chromium/src/DeviceOrientationClientProxy.cpp:68
>  +      OwnPtr<WebDeviceOrientation> lastWebOrientation(m_client->lastOrientation());
> Hmmm...this probably needs to be a refPtr in WebCore.
This is obsolete now that WebDOClient::lastOrientation() returns an object instead of a pointer.

> 
> 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.
That's an unpleasant detail. DeviceOrientation::lastOrientation() is expected to return a raw pointer to a DeviceOrientation object, which is reference counted.
The problem is we create the DeviceOrientation object from the WebDeviceOrientation object, and if we don't hold a reference to it ourselves, it will be destructed before our caller gets the result and puts it in a RefPtr. That's why we cache it: to hold an extra reference to it.

If this is too ugly, we should talk to Steve about changing DeviceOrientation::lastOrientation() to return a PassRefPtr.

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

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

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