[Webkit-unassigned] [Bug 43258] Implement WebDeviceOrientationClient wrapper and have WebViewImpl get it from WebViewClient.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43258





--- Comment #3 from hans at chromium.org  2010-08-02 02:15:27 PST ---
(In reply to comment #2)
> (From update of attachment 63069 [details])
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:41
>  +      // WEBKIT_API setOrientation(const WebDeviceOrientation&);
> These should be virtual thus WEBKIT_API is not needed.  All of these will also be in the WebDeviceOrientationClient.h (and be virtual).  The methods in the cpp should just call m_private->method_name(...)
But it's only the mock client that has a setOrientation function, so I don't see why it should be virtual?

> 
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:28
>  +  #include "WebCommon.h"
> not needed
OK.

> 
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:48
>  +      WebCore::DeviceOrientationClient* m_private;
> Use the private ptr class instead.
DeviceOrientationClient is not reference counted. As I understand, WebPrivatePtr is for reference counted types?

> 
> WebKit/chromium/public/WebDeviceOrientationClientMock.h:37
>  +      WEBKIT_API WebDeviceOrientationClientMock();
> Define these inline.  Have them both call initialize() and have it set itself to a new mock object in the src file.
Will do.

> Have the destructor call reset() and define it as {} in the src file.
Wait; both call reset() and define as {}?

> 
> WebDeviceOrientationCLient.cpp doesn't need to exist.  All the methods in the .h file should be implemented as calling WEBKIT_ASSERT_NOT_REACHED().
But isn't it a good defensive strategy to provide a dummy implementation for when WebViewClient fails to provide a WebDeviceOrientationClient?

> 
> WebKit/chromium/src/WebViewImpl.cpp:266
>  +      , m_webDeviceOrientationClient(createWebDeviceOrientationClient(client))
> m_webDeviceOrientationClient(webViewClient->createWebDeviceOrientationClient())
What if webViewClient is NULL? As Satish pointed out, it sometimes is.

> 
> WebKit/chromium/src/WebViewImpl.cpp:291
>  +          pageClients.deviceOrientationClient = m_webDeviceOrientationClient->client();
> not needed
> 
> WebKit/chromium/src/WebViewImpl.cpp:2211
>  +  WebDeviceOrientationClient* WebViewImpl::createWebDeviceOrientationClient(WebViewClient* webViewClient)
> not needed
> 
> 
> 
> 
> You're almost there.

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