[chromium] Add test for ScrollingCoordinatorChromium
Created attachment 163887 [details] Patch
Comment on attachment 163887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163887&action=review I really like the idea but this won't quite build as is - gonna need to do some more plumbing or expose this at a different level. > Source/WebKit/chromium/tests/ScrollingCoordinatorChromiumTest.cpp:32 > +#include "LayerChromium.h" you can't make a test that depends on WebCore stuff and LayerChromium any more. You gotta plumb what you want through the public Web*Layer* headers if you want to access it from a webkit_unit_test > Source/WebKit/chromium/tests/ScrollingCoordinatorChromiumTest.cpp:38 > +#include "WebLayerImpl.h" Can't see this either - now that WebLayerImpl is implemented in chromium the WebLayer type is completely opaque even to this unit test. You can either write the whole test on the chromium side or plumb it through the WebKit API
Ah, foiled by GTFO it seems :) I think I'll extend the WebLayer API to return the bits I need since ScrollingCoordinator isn't exposed to chromium at all. If I understood right I need to land the implementation on the chromium side first and only then expand WebLayer here.
Created attachment 164138 [details] Patch No more peeking into WebLayerImpl/LayerChromium. Requires https://codereview.chromium.org/10909233.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 164138 [details] Patch Attachment 164138 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13859181
FYI the cq build failures are because https://codereview.chromium.org/10909233 isn't in place yet.
Comment on attachment 164138 [details] Patch Attachment 164138 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13859205
Created attachment 164697 [details] Patch Rebased.
Comment on attachment 164697 [details] Patch 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)
Thanks, I'll clean up the nits. (In reply to comment #10) > > 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 Agreed. This ended up a little odd as I was trying to figure out what bits are needed to make the WebView tick. Setting things up in the constructor is definitely cleaner. > > 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 Thanks for the tip. > > 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 Done. > > 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 Good idea, done. > > 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) True.
Created attachment 165127 [details] Patch
Comment on attachment 165127 [details] Patch Clearing flags on attachment: 165127 Committed r129225: <http://trac.webkit.org/changeset/129225>
All reviewed patches have been landed. Closing bug.