Bug 191394 - Deprecate WKBundlePageSetDefersLoading
Summary: Deprecate WKBundlePageSetDefersLoading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-07 12:53 PST by Alex Christensen
Modified: 2019-02-11 11:32 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.67 KB, patch)
2018-11-07 12:58 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (4.42 KB, patch)
2019-02-05 15:41 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.95 MB, application/zip)
2019-02-05 16:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.80 MB, application/zip)
2019-02-05 17:28 PST, Build Bot
no flags Details
Patch (6.89 KB, patch)
2019-02-07 10:52 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2018-11-07 12:53:37 PST
Deprecate WKBundlePageSetDefersLoading
Comment 1 Alex Christensen 2018-11-07 12:58:41 PST
Created attachment 354133 [details]
Patch
Comment 2 Dean Jackson 2018-11-12 10:14:22 PST
Comment on attachment 354133 [details]
Patch

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

r=me except for the build errors.

> Source/WebKit/ChangeLog:8
> +        The last use of it was removed in <rdar://45212964>

Nit: .
Comment 3 Dean Jackson 2018-11-12 10:15:25 PST
Comment on attachment 354133 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextControllerPrivate.h:48
> +@property (nonatomic, setter=_setDefersLoading:) BOOL _defersLoading WK_API_DEPRECATED(_message, ...);

WKWebProcessPlugInBrowserContextControllerPrivate.h:48:88: error: use of undeclared identifier '_message'
Comment 4 Alex Christensen 2019-02-05 15:41:17 PST
Created attachment 361231 [details]
Patch
Comment 5 Build Bot 2019-02-05 16:53:48 PST
Comment on attachment 361231 [details]
Patch

Attachment 361231 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11045604

New failing tests:
loader/load-defer.html
Comment 6 Build Bot 2019-02-05 16:53:50 PST
Created attachment 361250 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 7 Build Bot 2019-02-05 17:28:01 PST
Comment on attachment 361231 [details]
Patch

Attachment 361231 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11045711

New failing tests:
loader/load-defer.html
Comment 8 Build Bot 2019-02-05 17:28:03 PST
Created attachment 361256 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 9 Alex Christensen 2019-02-07 10:52:24 PST
Created attachment 361416 [details]
Patch
Comment 10 Alex Christensen 2019-02-07 10:53:10 PST
http://trac.webkit.org/r241127
Comment 11 Radar WebKit Bug Importer 2019-02-07 10:54:31 PST
<rdar://problem/47889372>
Comment 12 Alex Christensen 2019-02-07 11:21:26 PST
http://trac.webkit.org/r241133
Comment 13 Darin Adler 2019-02-09 11:47:38 PST
Comment on attachment 361416 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:746
> -void TestRunner::setDefersLoading(bool shouldDeferLoading)
> +void TestRunner::setDefersLoading(bool)
>  {
> -    WKBundlePageSetDefersLoading(InjectedBundle::singleton().page()->page(), shouldDeferLoading);
>  }

This change isn’t quite right. We should be removing this function instead of hollowing it out and leaving it. A setDefersLoading function that does nothing isn’t valuable. It’s dangerous because it lets us run tests that won’t be testing what they are trying to test.

We don’t need to keep the function to keep running the loader/load-defer.html test, because we are no longer running that test under modern WebKit, only legacy WebKit. But it’s not good that we keep running the two other tests that use it, loader/load-defer-resume-crash.html and loader/navigation-while-deferring-loads.html, because they aren’t testing what they claim to be testing any more.

I suggest we change all of those tests into legacy-WebKit-only tests and remove this function entirely.

Is that right, or am I missing something?
Comment 14 Alex Christensen 2019-02-11 11:32:03 PST
You're absolutely right, Darin.  I did so in my next patch in https://bugs.webkit.org/show_bug.cgi?id=194506