Bug 143743 – WebKitTestRunner should use modern API on OS X and iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Enrica Casucci]

We should adopt the modern API on Apple platforms.

[Enrica Casucci]

Created attachment 250763 [details]
Patch

[Enrica Casucci]

Created attachment 250841 [details]
Patch2

Fixes build failures.

[Build Bot]

Comment on attachment 250841 [details]
Patch2

Attachment 250841 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4685786897186816

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 250844 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Darin Adler]

Regressions: Unexpected timeouts (7)
  animations/3d/change-transform-in-end-event.html [ Timeout ]
  compositing/animation/animation-compositing.html [ Timeout ]
  compositing/backing/backface-visibility-flip.html [ Timeout ]
  compositing/checkerboard.html [ Timeout ]
  compositing/contents-scale/animating.html [ Timeout ]
  compositing/layer-creation/animation-overlap-with-children.html [ Timeout ]
  compositing/reflections/animation-inside-reflection.html [ Timeout ]

[Darin Adler]

Comment on attachment 250841 [details]
Patch2

Looks like there are 30 tests failing. I suggest fixing those before we get this reviewed.

[Tim Horton]

Timers are getting throttled to 1 second intervals. Going to look into this now.

[Tim Horton]

That's probably because we're not disabling occlusion detection when we're using WKWebView.

There's a second mistake here; the patch as it stands uses WKWebView by default, even though it purports to have it off by default with a command line argument to enable it. That's why this broke tests without actually enabling it. This happened because Options::useModernAPI was not initialized!

[Tim Horton]

Next big problem seems to be that plugins don't work. I remembered that WKWebView has plugins off by default, but flipping the preference on doesn't seem to be sufficient.

Anyway, I think we should be able to land this as it is now (with WKWebView off by default) like Enrica originally planned, and continue to improve the missing things and then eventually turn it on.

I'll post a fixed up patch shortly for EWS to play with.

[Tim Horton]

Created attachment 252144 [details]
Patch

[Tim Horton]

Plugins are broken because the patch is quite wrong, and ends up making two different WKContext/WKProcessPools and using them for different things. No good!

Hacky fix seems to work... maybe this will get us far enough that we can avoid landing the switchable-API version of this patch?

[Tim Horton]

Only four failures in fast/ now, and none in compositing/! That helped a lot.

[Tim Horton]

Created attachment 252146 [details]
Patch

[Tim Horton]

Next failure is because BackgroundShouldExtendBeyondPage defaults to NO in the antique API and YES in the modern API. Easy fix, WKTR can maintain the old default.

[Tim Horton]

Next up: teaching EventSenderProxy::mouseUp to hit the inner WKView instead of the WKWebView when the mouse moves outside of the window, to make fast/events/mouseup-outside-document.html pass.

[Tim Horton]

Created attachment 252148 [details]
Patch

[Tim Horton]

37 failures remain, all but one in http/ or userscripts/. The http/tests/contentextensions tests seem pretty completely hosed, and it looks like the userscripts tests don't work at all either. Might need some help on those tomorrow.

[Tim Horton]

It seems like content extensions and user scripts are broken for the same reason: WKTR is using WKPageGroup SPI that doesn't play nicely with the Modern API (I guess?).

We end up with two UserContentControllers (one owned by WebPage, one owned by WebPageGroupProxy), and install extensions/userscripts/etc. on one of them (the WebPageGroupProxy one, I think) while evaluating loads against the other one (the WebPage one, I think).

Maybe Sam has some idea of how we should resolve this.

[Tim Horton]

(In reply to comment #18)
> Maybe Sam has some idea of how we should resolve this.

Maybe WKTR just needs to use the ObjC WKUserContentController API instead of WKPageGroup if we're using WKWebView?

I wonder if we can make this API misuse a harder failure. As it is, it's pretty mysterious.

[Simon Fraser (smfr)]

Comment on attachment 252148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252148&action=review

> Tools/ChangeLog:9
> +        This patch add support for the modern API on OS X and iOS.

adds

> Tools/ChangeLog:14
> +        run with the --use-modern-api option on the command line.
> +        The code is slightly more complex than it should, because
> +        I wanted to keep supporting the C API for Apple platforms,
> +        since some of the tests are still a bit flakey.

Using the term 'modern' though out this patch will quickly become an anachronism. Maybe "native API" or "objective-C API"?

Or rename the old one to "legacy".

> Tools/WebKitTestRunner/Options.cpp:139
> +    optionList.append(Option("--use-modern-api", "Uses WKWebView on Mac.", handleOptionUseModernAPI, false));

Not iOS?

> Tools/WebKitTestRunner/TestController.cpp:371
> +    // FIXME: This seems backwards. Is there any reason we can't always make a WKContextRef here and put it in the
> +    // WKWebViewConfiguration in platformInitializeConfiguration by casting to WKProcessPool?

Seems confusing. Should we fix this now?

> Tools/WebKitTestRunner/TestController.h:138
> +    static PlatformWebView* platformCreateOtherPage(PlatformWebView* parentView, WKContextRef, WKPageGroupRef, WKPageRef otherPage, WKDictionaryRef options);

'other' is confusing here.

> Tools/WebKitTestRunner/TestController.h:281
> +    bool m_useModernAPI;

C++11 initializer?

> Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:90
> +    _useTiledDrawing = useTiledDrawing;
> +    return [super initWithFrame:frame configuration:configuration];

Weird to assign to a member var before you initialize super.

> Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:163
> +    WKPreferencesSetCompositingBordersVisible(preferences, YES);
> +    WKPreferencesSetCompositingRepaintCountersVisible(preferences, YES);

I don't think we should hardcode those to be on. The fact that iOS did this was leftover debugging.

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:39
> +- (WKPageRef)_pageForTesting;

Gross.

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:95
> +    _useThreadedScrolling = useThreadedScrolling;

Ditto.

[Tim Horton]

Comment on attachment 252148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252148&action=review

>> Tools/ChangeLog:14
>> +        since some of the tests are still a bit flakey.
> 
> Using the term 'modern' though out this patch will quickly become an anachronism. Maybe "native API" or "objective-C API"?
> 
> Or rename the old one to "legacy".

If all goes well later today we won't need the switch and this comment won't be necessary. The patch is verrrrry close.

>> Tools/WebKitTestRunner/TestController.cpp:371
>> +    // WKWebViewConfiguration in platformInitializeConfiguration by casting to WKProcessPool?
> 
> Seems confusing. Should we fix this now?

Want to talk to Sam. But maybe!

>> Tools/WebKitTestRunner/TestController.h:138
>> +    static PlatformWebView* platformCreateOtherPage(PlatformWebView* parentView, WKContextRef, WKPageGroupRef, WKPageRef otherPage, WKDictionaryRef options);
> 
> 'other' is confusing here.

Additional? Secondary? Isn't it called other in the API?

>> Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:90
>> +    return [super initWithFrame:frame configuration:configuration];
> 
> Weird to assign to a member var before you initialize super.

That is definitely weird.

>> Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:163
>> +    WKPreferencesSetCompositingRepaintCountersVisible(preferences, YES);
> 
> I don't think we should hardcode those to be on. The fact that iOS did this was leftover debugging.

Oh god no. Did not notice that.

Also did you notice that iOS doesn't have snapshotting implemented? :|

>> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:39
>> +- (WKPageRef)_pageForTesting;
> 
> Gross.

This is the leadership-approved way of doing this. Didn't want it to be in any headers, not even Private.

[Tim Horton]

(In reply to comment #19)
> (In reply to comment #18)
> > Maybe Sam has some idea of how we should resolve this.
> 
> Maybe WKTR just needs to use the ObjC WKUserContentController API instead of
> WKPageGroup if we're using WKWebView?

This ^ seems to work. Content extension tests pass now.

[Alexey Proskuryakov]

Is it feasible to make an overview of what old code becomes unused when using the modern API path? I'd like to look over the dying code to verify that there aren't any non-obvious important hacks that could be forgotten.

[Tim Horton]

(In reply to comment #23)
> Is it feasible to make an overview of what old code becomes unused when
> using the modern API path? I'd like to look over the dying code to verify
> that there aren't any non-obvious important hacks that could be forgotten.

I'm going to get rid of the switch and delete the old code, so it will show up as red and we can look through it then.

[Tim Horton]

User scripts are harder. The user scripts are injected by JS in the page, and the asynchronicity of pushing them to the UI process and back (to go through WKUserContentController, which seems to be the only modern API way to add user scripts) causes the user script to be installed *after* at-load user scripts are evaluated.

So, probably we need to add some bundle-side SPI for WKTR to synchronously inject user scripts. Or something.

[Tim Horton]

(In reply to comment #25)
> User scripts are harder. The user scripts are injected by JS in the page,
> and the asynchronicity of pushing them to the UI process and back (to go
> through WKUserContentController, which seems to be the only modern API way
> to add user scripts) causes the user script to be installed *after* at-load
> user scripts are evaluated.
> 
> So, probably we need to add some bundle-side SPI for WKTR to synchronously
> inject user scripts. Or something.

I added SPI to WKBundlePage. I don't know that it was a good idea, but it works, and now user scripts and user stylesheets work. Hopefully Anders or Sam can advise if this is OK. Going to do a full test run and see what's left.

[Tim Horton]

Created attachment 252218 [details]
Patch

[Build Bot]

Comment on attachment 252218 [details]
Patch

Attachment 252218 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5480560394764288

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 252219 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

Created attachment 252220 [details]
Patch

[Build Bot]

Comment on attachment 252220 [details]
Patch

Attachment 252220 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5327298781773824

New failing tests:
userscripts/user-script-audio-document.html
userscripts/user-script-plugin-document.html
userscripts/script-not-run-for-fragments.html
userscripts/document-element-available-at-start.html
userscripts/simple-stylesheet.html
userscripts/user-script-top-frame-only.html
userscripts/user-script-all-frames.html
userscripts/script-run-at-start.html
userscripts/script-run-at-end.html
userscripts/user-style-top-frame-only.html
userscripts/mixed-case-stylesheet.html
userscripts/user-script-image-document.html
userscripts/user-script-video-document.html
userscripts/user-stylesheet-invalidate.html
userscripts/user-style-all-frames.html
fast/css/user-stylesheet-crash.html

[Build Bot]

Created attachment 252221 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

Made some more progress today, adding some code to clear content extensions the right way (maybe) with the modern API, and setting a setting (storageBlockingPolicy) whose default is different between the antique and modern APIs.

Next up is getting us to use one StorageManager or set the backing directory on the right one, or something (but currently LocalStorage leaks between runs).

[Tim Horton]

Now I've added WKWebsiteDataStoreConfiguration and a way to make a WKWebsiteDataStore given a configuration, adopted it in WKTR, and added a way to disable the legacy data store in the process pool, and we're ending up with just one data store, correctly configured. Yay.

[Tim Horton]

Eight tests remain (on Mac):

compositing/video/poster.html
    - weird 1px image diff

fast/css-generated-content/initial-letter-clearance.html
fast/css-generated-content/initial-letter-descender.html
fast/text/international/bold-bengali.html
fast/text/international/danda-space.html
fast/text/midword-break-before-surrogate-pair.html
    - these all have slight text metrics/margin differences (why?)

http/tests/media/video-buffered-range-contains-currentTime.html
    - this has an extra EVENT(seeked) in the output

http/tests/security/storage-blocking-strengthened-plugin.html
    - this says 'false' instead of 'true' in frame0 (haven't looked at what that means yet)

[Tim Horton]

Created attachment 259088 [details]
patch

[WebKit Commit Bot]

Attachment 259088 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/mac/TestControllerMac.mm:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/TestController.h:139:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Tools/WebKitTestRunner/TestController.h:140:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 259088 [details]
patch

Attachment 259088 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/61348

New failing tests:
userscripts/user-script-audio-document.html
userscripts/user-script-plugin-document.html
userscripts/script-not-run-for-fragments.html
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.html
userscripts/document-element-available-at-start.html
userscripts/simple-stylesheet.html
userscripts/user-script-all-frames.html
userscripts/user-script-top-frame-only.html
userscripts/script-run-at-start.html
userscripts/script-run-at-end.html
userscripts/user-style-top-frame-only.html
userscripts/mixed-case-stylesheet.html
userscripts/user-script-image-document.html
userscripts/user-script-video-document.html
userscripts/user-stylesheet-invalidate.html
userscripts/user-style-all-frames.html
fast/css/user-stylesheet-crash.html

[Build Bot]

Created attachment 259089 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Alexey Proskuryakov]

> http/tests/media/video-buffered-range-contains-currentTime.html
>    - this has an extra EVENT(seeked) in the output

This is unrelated to the patch, see bug 148042.

[Tim Horton]

(In reply to comment #40)
> > http/tests/media/video-buffered-range-contains-currentTime.html
> >    - this has an extra EVENT(seeked) in the output
> 
> This is unrelated to the patch, see bug 148042.

Nice :D

I guess I have to run on Mavericks and see why EWS has so many failures that I don't see.

[Tim Horton]

(In reply to comment #41)
> (In reply to comment #40)
> > > http/tests/media/video-buffered-range-contains-currentTime.html
> > >    - this has an extra EVENT(seeked) in the output
> > 
> > This is unrelated to the patch, see bug 148042.
> 
> Nice :D
> 
> I guess I have to run on Mavericks and see why EWS has so many failures that
> I don't see.

I just realized that these are with the flag (--use-modern-api) *off*, so I haven't actually tested that. Will do that now. Some people think we shouldn't bother having a flag, so I might just get rid of it (and a bunch of code!).

[Tim Horton]

Created attachment 259315 [details]
Patch

[Tim Horton]

Created attachment 259330 [details]
Patch

[Tim Horton]

Created attachment 259364 [details]
Patch

[Tim Horton]

Created attachment 259365 [details]
Patch

[Tim Horton]

Comment on attachment 259365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259365&action=review

> Tools/WebKitTestRunner/TestController.cpp:622
> +    WKPreferencesSetStorageBlockingPolicy(preferences, kWKAllowAllStorage);

Add fixmes for these.

> Tools/WebKitTestRunner/TestController.cpp:683
> +    WKPageGroupRemoveAllUserContentFilters(WKPageGetPageGroup(m_mainWebView->page()));

Sam thinks we might not need this.

> Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:73
> +    return WKPageGroupGetPreferences(m_pageGroup.get());

This should probably use page configuration (and we shouldn't use page group anywhere!)

> Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:37
> +- (WKPageRef)_pageForTesting;

I don't think this needs to be here anymore.

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:273
> +    // when using the modern API, so the level of fakery and horror increases.

Figure out a better "why" here.

[Tim Horton]

Comment on attachment 259365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259365&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:5062
> +void WebPage::addUserScript(const String& source, WebCore::UserContentInjectedFrames injectedFrames, WebCore::UserScriptInjectionTime injectionTime)

Talk to Anders about what happens here with the other ports, Sam thinks that using this unconditionally from WKTR might break them.

Possible plan is to adopt WKPageConfiguration and set a UserContentControllerRef on it, so we don't end up using the PageGroup's.

[Tim Horton]

Created attachment 259555 [details]
Patch

[Tim Horton]

Created attachment 259563 [details]
Patch

[Build Bot]

Comment on attachment 259563 [details]
Patch

Attachment 259563 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/83995

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 259574 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

Split one WebKit2 part out into https://bugs.webkit.org/show_bug.cgi?id=148325

[Tim Horton]

Created attachment 259678 [details]
Patch

[Anders Carlsson]

Comment on attachment 259678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259678&action=review

> Tools/WebKitTestRunner/PlatformWebView.h:44
> -@class WKView;
> +@class NSView;
> +@class UIView;
> +@class WKWebView;
> +@class WKWebViewConfiguration;
>  @class WebKitTestRunnerWindow;
>  #else
> -class WKView;
> +class NSView;
> +class UIView;
> +class WKWebView;
> +class WKWebViewConfiguration;
>  class WebKitTestRunnerWindow;

Please use OBJC_CLASS here.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:95
> +    {

Please remove \n here.

[Tim Horton]

http://trac.webkit.org/changeset/188807

[Tim Horton]

http://trac.webkit.org/changeset/188813

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148349

[Tim Horton]

Trying again in http://trac.webkit.org/changeset/188828

[Csaba Osztrogonác]

(In reply to comment #59)
> Trying again in http://trac.webkit.org/changeset/188828

It broke the EFL build. Including the Cocoa only WKFoundation.h 
header in common code without ifdef guard is incorrect and should 
have been fixed before/after landing.

[Csaba Osztrogonác]

(In reply to comment #60)
> (In reply to comment #59)
> > Trying again in http://trac.webkit.org/changeset/188828
> 
> It broke the EFL build. Including the Cocoa only WKFoundation.h 
> header in common code without ifdef guard is incorrect and should 
> have been fixed before/after landing.

Buildfix landed in https://trac.webkit.org/changeset/188840.

[Csaba Osztrogonác]

Additional buildfixes landed in https://trac.webkit.org/changeset/188842 and
https://trac.webkit.org/changeset/188843

[Alexey Proskuryakov]

This caused bug 148512.