[webkit-reviews] review granted: [Bug 67417] [chromium] Make CCThreadProxy draw : [Attachment 107727] Merge win the tests.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 67417: [chromium] Make CCThreadProxy draw
https://bugs.webkit.org/show_bug.cgi?id=67417

Attachment 107727: Merge win the tests.
https://bugs.webkit.org/attachment.cgi?id=107727&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review


R=me.  I left some nits on the previous patch and this one which apply, but
none that are too serious.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:307
> +INSTANTIATE_TEST_CASE_P(

Very nifty - so we'll run the tests in threaded and non-threaded mode? Me likey


> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
> +	   CCSettings(false, false, false, false, false),
> +	   CCSettings(false, false, true, false, false)));

now I kind of wish CCSettings some sort of c'tor where we could see what on
earth these flags are at callsites.  I think I'm wishing for a different
language...

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:558
> +    CCSettings setings;

setings->settings ? What actually uses this, or do you just need to have
something on the stack that a magical macro picks up?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:564
> +#endif

looks like we have ourselves a comment fight going on!	I'm personally a fan of
the // USE(THREADED_COMPOSITING) comment on this #endif

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:136
> +    ScopedSetImplThread impl;

I'm actually slightly surprised this works.  LayerChromium creation and
destruction should always be a main thread thing, but I guess we don't have
proper ASSERT()s to verify that so just doing the whole thing on the impl
thread works just fine.

I think that the synchronizeTrees() function should have a ScopedSetImplThread
doohickey, since normally we call that from commitTo(), but the LayerChromium
setup should be main thread.  If this works for now, though, leave it this way
and we can clean up when we add more ASSERT()s for LayerChromium.