96657 – [chromium] Add test for ScrollingCoordinatorChromium

RESOLVED FIXED 96657

[chromium] Add test for ScrollingCoordinatorChromium

https://bugs.webkit.org/show_bug.cgi?id=96657

Summary [chromium] Add test for ScrollingCoordinatorChromium

Sami Kyöstilä

Reported 2012-09-13 08:56:36 PDT

[chromium] Add test for ScrollingCoordinatorChromium

Attachments
Patch (18.13 KB, patch) 2012-09-13 09:06 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (17.65 KB, patch) 2012-09-14 07:07 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (17.66 KB, patch) 2012-09-19 02:38 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (15.90 KB, patch) 2012-09-21 06:41 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Sami Kyöstilä

Comment 1 2012-09-13 09:06:36 PDT

Created attachment 163887 [details] Patch

James Robinson

Comment 2 2012-09-13 16:34:17 PDT

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

Sami Kyöstilä

Comment 3 2012-09-14 06:47:28 PDT

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.

Sami Kyöstilä

Comment 4 2012-09-14 07:07:30 PDT

Created attachment 164138 [details] Patch No more peeking into WebLayerImpl/LayerChromium. Requires https://codereview.chromium.org/10909233.

WebKit Review Bot

Comment 5 2012-09-14 07:12:20 PDT

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.

WebKit Review Bot

Comment 6 2012-09-14 07:13:17 PDT

Comment on attachment 164138 [details] Patch Attachment 164138 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13859181

Sami Kyöstilä

Comment 7 2012-09-14 07:17:50 PDT

FYI the cq build failures are because https://codereview.chromium.org/10909233 isn't in place yet.

Peter Beverloo (cr-android ews)

Comment 8 2012-09-14 08:09:07 PDT

Comment on attachment 164138 [details] Patch Attachment 164138 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13859205

Sami Kyöstilä

Comment 9 2012-09-19 02:38:41 PDT

Created attachment 164697 [details] Patch Rebased.

James Robinson

Comment 10 2012-09-19 10:22:25 PDT

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)

Sami Kyöstilä

Comment 11 2012-09-21 06:33:23 PDT

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.

Sami Kyöstilä

Comment 12 2012-09-21 06:41:28 PDT

Created attachment 165127 [details] Patch

WebKit Review Bot

Comment 13 2012-09-21 07:11:06 PDT

Comment on attachment 165127 [details] Patch Clearing flags on attachment: 165127 Committed r129225: <http://trac.webkit.org/changeset/129225>

WebKit Review Bot

Comment 14 2012-09-21 07:11:10 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Sami Kyöstilä

Reported

2012-09-13 08:56 PDT

Modified

2012-09-21 07:11 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks