[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 #9 from hans at chromium.org  2010-08-03 08:12:06 PST ---
Fixing style issues first, will then provide a code example of how this would tie in with the orientation plumbing as a whole to make this easier to discuss.

(In reply to comment #7)
> (From update of attachment 63312 [details])
> WebKit/chromium/src/WebDeviceOrientationClientDummy.cpp:37
>  +      DeviceOrientationClientDummy() : orientation(WebCore::DeviceOrientation::create()) { }
> usually we split onto 2 lines right before the :
Done.

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

> 
> 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.
Happy to change this if you decide it needn't be inline.

> 
> 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
Done. (I assume you mean assignment operator and copy ctor.)

> 
> WebKit/chromium/public/WebDeviceOrientationClient.h:50
>  +      WebDeviceOrientationClient() { }
> This works??  I'm pretty sure this will need to be public and virtual.
AFAIK, constructors can't be virtual. Why would it need to be public? It's only called by subclass constructors (and create(), when that is implemented).

> 
> 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.
client will be null during some UI tests. This check is analogue to Satish's check in Bug 43240. Either we handle it, or we require it to be non-NULL.

We can't let device orientation be NULL when the feature is enabled, as Page will then crash upon construction.

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

Will scrap the dummy and use the mock as fall-back for now. Willing to change later.

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