Bug 80613 – [chromium] Clean up culling tests and templatize to test impl constructs

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Clean up culling tests and templatize to test impl constructs

[Dana Jansens]

Created attachment 130854 [details]
Patch

This is a large unit test cleanup, plus templatization so that we can run all the CCOcclusionTracker tests with both main and impl thread constructs.

While cleaning up, I made the test contain fewer LOC that are not relevant to the particular test, with helper methods in the parent test class.

Coverage remains the same.

[Dana Jansens]

This is built on top of bug #80173 so EWS should wait for it to land.

[Dana Jansens]

Created attachment 130855 [details]
Patch

[Dana Jansens]

Created attachment 131053 [details]
Patch

[Dana Jansens]

http://p.prom.corp.google.com/2124004

[Adrienne Walker]

Comment on attachment 131053 [details]
Patch

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

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:275
> +    PassOwnPtr<LayerChromium> create(LayerChromium* parent)
> +    {
> +        RefPtr<LayerChromium> layer = LayerChromium::create();
> +        return adoptPtr(layer.release().leakRef());
> +    }

This is super super sketchy.  I know that this is "just" test code, but I don't like mixing RefPtr and OwnPtr like this, since the potential for accidentally freeing something with a RefPtr that some OwnPtr is still pointing at seems really huge.  And, even if ASAN doesn't get any indigestion from the current code, I don't trust future code to behave properly.

Can CCOcclusionTrackerTest typedef LayerPtr and PassLayerPtr to be the right OwnPtr or RefPtr, avoiding this problem? I think this would also allow you to merge some functions and m_layerChromium/m_layerImpl.

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:317
> +        CCProxy::setCurrentThreadIsImplThread(true); \

I think you can use DebugScopedSetImplThread instead of calling setCurrentThreadIsImplThread anywhere in this patch.

[Dana Jansens]

(In reply to comment #6)
> (From update of attachment 131053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131053&action=review
> 
> > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:275
> > +    PassOwnPtr<LayerChromium> create(LayerChromium* parent)
> > +    {
> > +        RefPtr<LayerChromium> layer = LayerChromium::create();
> > +        return adoptPtr(layer.release().leakRef());
> > +    }
> 
> This is super super sketchy.  I know that this is "just" test code, but I don't like mixing RefPtr and OwnPtr like this, since the potential for accidentally freeing something with a RefPtr that some OwnPtr is still pointing at seems really huge.  And, even if ASAN doesn't get any indigestion from the current code, I don't trust future code to behave properly.
>
> Can CCOcclusionTrackerTest typedef LayerPtr and PassLayerPtr to be the right OwnPtr or RefPtr, avoiding this problem? I think this would also allow you to merge some functions and m_layerChromium/m_layerImpl.

Thanks for the brilliant idea to dump all the types into two classes and make the template on them. That really helped make this possible. Got rid of all the leakPtr weirdness.
 
> > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:317
> > +        CCProxy::setCurrentThreadIsImplThread(true); \
> 
> I think you can use DebugScopedSetImplThread instead of calling setCurrentThreadIsImplThread anywhere in this patch.

Done! Thanks for pointing it out.

[Dana Jansens]

Created attachment 131100 [details]
Patch

[Dana Jansens]

go/pastebin/2123005

[Adrienne Walker]

Comment on attachment 131100 [details]
Patch

Looks way better.  Thanks!  R=me

[WebKit Review Bot]

Comment on attachment 131100 [details]
Patch

Clearing flags on attachment: 131100

Committed r110353: <http://trac.webkit.org/changeset/110353>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

Reopening to fix compile on other platforms.

[Dana Jansens]

Created attachment 131179 [details]
Patch

Fix build on mac. Missing this-> on calcDrawEtc() calls.

[WebKit Review Bot]

Comment on attachment 131179 [details]
Patch

Clearing flags on attachment: 131179

Committed r110384: <http://trac.webkit.org/changeset/110384>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

Reopening to fix win build!

[Dana Jansens]

Created attachment 131261 [details]
Patch

Fix build on win. Missing a default constructor in each of the test classes. Ran a win & mac try job.

[WebKit Review Bot]

Comment on attachment 131261 [details]
Patch

Clearing flags on attachment: 131261

Committed r110433: <http://trac.webkit.org/changeset/110433>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.