We should adopt the modern API on Apple platforms.
Created attachment 250763 [details] Patch
Created attachment 250841 [details] Patch2 Fixes build failures.
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.
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
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 ]
Comment on attachment 250841 [details] Patch2 Looks like there are 30 tests failing. I suggest fixing those before we get this reviewed.
Timers are getting throttled to 1 second intervals. Going to look into this now.
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!
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.
Created attachment 252144 [details] Patch
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?
Only four failures in fast/ now, and none in compositing/! That helped a lot.
Created attachment 252146 [details] Patch
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.
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.
Created attachment 252148 [details] Patch
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.
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.
(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.
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.
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.
(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.
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.
(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.
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.
(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.
Created attachment 252218 [details] Patch
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.
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
Created attachment 252220 [details] Patch
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
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
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).
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.
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)
Created attachment 259088 [details] patch
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.
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
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
> 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.
(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.
(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!).
Created attachment 259315 [details] Patch
Created attachment 259330 [details] Patch
Created attachment 259364 [details] Patch
Created attachment 259365 [details] Patch
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.
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.
Created attachment 259555 [details] Patch
Created attachment 259563 [details] Patch
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.
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
Split one WebKit2 part out into https://bugs.webkit.org/show_bug.cgi?id=148325
Created attachment 259678 [details] Patch
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.
http://trac.webkit.org/changeset/188807
http://trac.webkit.org/changeset/188813
Re-opened since this is blocked by bug 148349
Trying again in http://trac.webkit.org/changeset/188828
(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.
(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.
Additional buildfixes landed in https://trac.webkit.org/changeset/188842 and https://trac.webkit.org/changeset/188843
This caused bug 148512.