[chromium] Clean up culling tests and templatize to test impl constructs
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.
This is built on top of bug #80173 so EWS should wait for it to land.
Created attachment 130855 [details] Patch
Created attachment 131053 [details] Patch
http://p.prom.corp.google.com/2124004
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.
(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.
Created attachment 131100 [details] Patch
go/pastebin/2123005
Comment on attachment 131100 [details] Patch Looks way better. Thanks! R=me
Comment on attachment 131100 [details] Patch Clearing flags on attachment: 131100 Committed r110353: <http://trac.webkit.org/changeset/110353>
All reviewed patches have been landed. Closing bug.
Reopening to fix compile on other platforms.
Created attachment 131179 [details] Patch Fix build on mac. Missing this-> on calcDrawEtc() calls.
Comment on attachment 131179 [details] Patch Clearing flags on attachment: 131179 Committed r110384: <http://trac.webkit.org/changeset/110384>
Reopening to fix win build!
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.
Comment on attachment 131261 [details] Patch Clearing flags on attachment: 131261 Committed r110433: <http://trac.webkit.org/changeset/110433>