Bug 48506 – Move DeviceOrientationClientMock from LayoutTestController to WebViewHost

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[John Knottenbelt]

The test fast/dom/DeviceOrientation/no-page-cache.html opens an additional
window which results in a secondary page with its own
DeviceOrientationController. Since DeviceOrientationClientMock maintains a back
pointer to the corresponding WebCore::Page's DeviceOrientationController, there
should be one DeviceOrientationClientMock per DeviceOrientationController. 

In DumpRenderTree (unlike test_shell) there is only one instance of
LayoutTestController. LayoutTestController owns WebDeviceOrientationClientMock
which owns DeviceOrientationClientMock, so there is only one instance of the
mock that must serve two controllers (in the above test).

There is exactly one WebViewHost instance per WebView (per page). I propose that
we move the mock to WebViewHost, and that
LayoutTestController::setMockOrientation delegates to the TestShell's primary
WebView.

[John Knottenbelt]

Created attachment 72178 [details]
Patch

[Hans Wennborg]

Comment on attachment 72178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72178&action=review

This looks fine (assuming the tests still pass of course :), I only have a couple of nits (also I'm not a real reviewer):

> WebCore/ChangeLog:5
> +        Add an assert to check that the DeviceOrientationClient is 

DeviceOrientationClient -> DeviceOrientationClientMock?

> WebKitTools/ChangeLog:6
> +        WebViewHost.

Maybe add a comment that this is to make sure that each Page gets its own client?

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1562
> +WebKit::WebDeviceOrientationClientMock* LayoutTestController::deviceOrientationClientMock()

There's a using directive for the WebKit namespace, so WebKit:: shouldn't be necessary.

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:1110
> +    PassOwnPtr<WebDeviceOrientationClientMock> deviceOrientationClientMock = m_deviceOrientationClientMock.release();

Should this be a PassOwnPtr? I'm always confused by these smart pointers, but since release() returns a raw pointer, isn't that what we should use? Steve would know this better than me, I think..

[John Knottenbelt]

Comment on attachment 72178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72178&action=review

>> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:1110
>> +    PassOwnPtr<WebDeviceOrientationClientMock> deviceOrientationClientMock = m_deviceOrientationClientMock.release();
> 
> Should this be a PassOwnPtr? I'm always confused by these smart pointers, but since release() returns a raw pointer, isn't that what we should use? Steve would know this better than me, I think..

PassOwnPtr will take ownership of the mock, and set the member variable m_deviceOrientationClientMock to 0. The reason for doing this is because the destructor will be invoked and we don't actually want the mock to be destroyed and reallocated because the WebView() still exists, meaning that the DeviceOrientationController still have a pointer back to the current (Web)DeviceOrientationControllerMock. This means that the address of the DeviceOrientationControllerMock should stay the same. 

I need to talk to Kent Tamura about whether the placement destruction / new is necessary, but in the meantime this should do the trick.

If it is important to reset the state of the mock between tests (the tests run fine without), we should add a reset() method to the mock and call it at the end of this method.

[John Knottenbelt]

Created attachment 73361 [details]
Patch

[John Knottenbelt]

WebViewHost::reset() refactored, so no longer need rather strange PassOwnPtr combination.

[Hans Wennborg]

(In reply to comment #5)
> WebViewHost::reset() refactored, so no longer need rather strange PassOwnPtr combination.

Great!

I believe my first three nits still apply, though ;)

[John Knottenbelt]

Created attachment 73372 [details]
Patch

[Hans Wennborg]

(In reply to comment #7)
> Created an attachment (id=73372) [details]
> Patch

LGTM.

[Steve Block]

Comment on attachment 73372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73372&action=review

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1569
> +    return m_shell->webViewHost()->deviceOrientationClientMock();

So webViewHost() returns the primary WebView, right? This means that JS calls in all pages will only configure the mock for the main page. Maybe add a comment?

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.h:446
> +    WebKit::WebDeviceOrientationClientMock* deviceOrientationClientMock();

Is there any need for this method if it's a one-liner and only called once?

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:584
> +WebDeviceOrientationClient* WebViewHost::deviceOrientationClient()

Is this method needed? Can we just use deviceOrientationClientMock()? Where is it called from?

[John Knottenbelt]

Created attachment 73904 [details]
Patch

[Jeremy Orlow]

Comment on attachment 73904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73904&action=review

r=me

> WebCore/platform/mock/DeviceOrientationClientMock.cpp:43
> +    ASSERT(controller);

It's probably better to keep the m_controler check after

> WebCore/platform/mock/DeviceOrientationClientMock.cpp:-43
> -    ASSERT(m_controller);

I think it's better to leave this one.

> WebKitTools/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=48506

"""
Reviewed by

Title
this url

description
"""

Also describe why it's a design constraint.

[John Knottenbelt]

Created attachment 73906 [details]
Patch

[Jeremy Orlow]

Comment on attachment 73906 [details]
Patch

lgtm

[WebKit Commit Bot]

Comment on attachment 73906 [details]
Patch

Clearing flags on attachment: 73906

Committed r72071: <http://trac.webkit.org/changeset/72071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.