Bug 49069 - WebViewHost::reset() uses placement new.
Summary: WebViewHost::reset() uses placement new.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 48506
  Show dependency treegraph
 
Reported: 2010-11-05 07:28 PDT by John Knottenbelt
Modified: 2010-11-08 19:04 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.36 KB, patch)
2010-11-05 07:36 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2010-11-08 03:53 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2010-11-05 07:28:26 PDT
WebViewHost::reset() uses placement new.
Comment 1 John Knottenbelt 2010-11-05 07:30:56 PDT
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.
Comment 2 John Knottenbelt 2010-11-05 07:36:43 PDT
Created attachment 73064 [details]
Patch
Comment 3 Kent Tamura 2010-11-06 06:55:58 PDT
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.
Comment 4 John Knottenbelt 2010-11-08 03:53:16 PST
Created attachment 73231 [details]
Patch
Comment 5 Kent Tamura 2010-11-08 04:33:09 PST
Comment on attachment 73231 [details]
Patch

LGTM.  Thank you!
Comment 6 WebKit Commit Bot 2010-11-08 19:04:46 PST
Comment on attachment 73231 [details]
Patch

Clearing flags on attachment: 73231

Committed r71597: <http://trac.webkit.org/changeset/71597>
Comment 7 WebKit Commit Bot 2010-11-08 19:04:51 PST
All reviewed patches have been landed.  Closing bug.