[webkit-reviews] review granted: [Bug 96657] [chromium] Add test for ScrollingCoordinatorChromium : [Attachment 164697] Patch

Wed Sep 19 10:21:54 PDT 2012

James Robinson <jamesr at chromium.org> has granted Sami Kyöstilä
<skyostil at chromium.org>'s request for review:
Bug 96657: [chromium] Add test for ScrollingCoordinatorChromium
https://bugs.webkit.org/show_bug.cgi?id=96657

Attachment 164697: Patch
https://bugs.webkit.org/attachment.cgi?id=164697&action=review

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

Looks great!  Have a few nits

> Source/WebKit/chromium/tests/ScrollingCoordinatorChromiumTest.cpp:71
> +    WebViewImpl* createCompositedWebViewImpl(const std::string& url)

I think this signature is pretty weird - it's called create..() and it returns
a pointer, but it doesn't transfer ownership. could you instead create the view
in the fixture's constructor and rename this something like navigate()?  you
can access protected member variables directly in the test instances.  a new
instance of the fixture is created + destroyed for each unit test

> Source/WebKit/chromium/tests/ScrollingCoordinatorChromiumTest.cpp:74
> +	   WebKit::Platform::current()->compositorSupport()->initialize(0);

there's a WebCompositorInitializer in namespace WebKitTests that can help with
this - make it a member var of the fixture

> Source/WebKit/chromium/tests/data/fixed_position.html:1
> +<!DOCTYPE html>

supernit: I think layout tests typically use dashes instead of underscores, so
fixed-position.html

> Source/WebKit/chromium/tests/data/fixed_position.html:7
> +	 /* Use a stacking context to enable composition. */

if you want you can also setFixedPositionCreatesStackingContext(true) in the
test setup - that's what we are actually shipping these days in chromium so it
makes sense to test

> Source/WebKit/chromium/tests/data/fixed_position_non_composited.html:5
> +    .fixed {

this isn't so useful of a test now that we ship
setFixedPositionCreatesStackingContext(true)