Bug 49069 – WebViewHost::reset() uses placement new.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Knottenbelt]

WebViewHost::reset() uses placement new.

[John Knottenbelt]

WebViewHost is using placement destruction / new to simulate a fresh WebViewHost
object at the same address. This is because the WebView remains open across
tests and maintains a pointer to the WebViewHost. 

The use of placement new makes it difficult to add new mocks such as
DeviceOrientationClientMock (see bug ...)  to the WebViewHost, because the
WebView's page controllers maintain pointers to their respective clients, which
indirectly point back to the mocks. For this reason, the mock clients must
keep their addresses across calls to WebViewHost::reset(). 

If we keep the placement new dance, we will have to add code such as:
PassOwnPtr deviceOrientationClientMock = m_deviceOrientationClientMock.release();
...
this->WebViewHost();
new (this) WebViewHost(shell)
...
m_deviceOrientationClientMock = deviceOrientationClientMock;

This seems quite cumbersome, so I propose resetting the member variables that
need it instead (patch attached). Alternatively, we could completely destroy
both the WebView and the WebViewHost and create fresh ones for each
test. However, doing that might slow down the test suite, as well as change the
way that tests can interfere with each other, possibly reducing the number of
bugs that we can discover.

[John Knottenbelt]

Created attachment 73064 [details]
Patch

[Kent Tamura]

Comment on attachment 73064 [details]
Patch

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

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:1111
> +    m_policyDelegateEnabled = false;
> +    m_policyDelegateIsPermissive = false;
> +    m_policyDelegateShouldNotifyDone = false;
> +    m_topLoadingFrame = 0;
> +    m_pageId = -1;
> +    m_lastPageIdUpdated = -1;
> +    m_hasWindow = false;
> +    m_inModalLoop = false;
> +    m_smartInsertDeleteEnabled = true;
> +#if OS(WINDOWS)
> +    m_selectTrailingWhitespaceEnabled = true;
> +#else
> +    m_selectTrailingWhitespaceEnabled = false;
> +#endif
> +    m_blocksRedirects = false;
> +    m_requestReturnNull = false;
> +    m_isPainting = false;
> +    m_canvas.clear();

We had better reset the followings too:
m_pendingExtraData
m_resourceIdentifierMap
m_currentCursor
m_windowRect
m_clearHeaders
m_editCommandName
m_editCommandvalue
m_paintRect

Some of them don't need to be reset.  But we should be consistent.

[John Knottenbelt]

Created attachment 73231 [details]
Patch

[Kent Tamura]

Comment on attachment 73231 [details]
Patch

LGTM.  Thank you!

[WebKit Commit Bot]

Comment on attachment 73231 [details]
Patch

Clearing flags on attachment: 73231

Committed r71597: <http://trac.webkit.org/changeset/71597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.