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.
Created attachment 72178 [details] Patch
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..
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.
Created attachment 73361 [details] Patch
WebViewHost::reset() refactored, so no longer need rather strange PassOwnPtr combination.
(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 ;)
Created attachment 73372 [details] Patch
(In reply to comment #7) > Created an attachment (id=73372) [details] > Patch LGTM.
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?
Created attachment 73904 [details] Patch
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.
Created attachment 73906 [details] Patch
Comment on attachment 73906 [details] Patch lgtm
Comment on attachment 73906 [details] Patch Clearing flags on attachment: 73906 Committed r72071: <http://trac.webkit.org/changeset/72071>
All reviewed patches have been landed. Closing bug.