Add Link Preload as an off-by-default experimental feature menu item.
Created attachment 299245 [details] Patch
Created attachment 299584 [details] Patch
Rebased. PTAL?
Created attachment 299690 [details] Patch
Comment on attachment 299690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299690&action=review Neat! r- only because I think there is some additional cleanup you should do when switching to an experimental feature. 1. Remove the legacy way to enable the feature for tests (since test runners will auto-enable it now!) Source/WebCore/testing/InternalSettings.cpp 187: RuntimeEnabledFeatures::sharedFeatures().setLinkPreloadEnabled(m_linkPreloadEnabled); 674:void InternalSettings::setLinkPreloadEnabled(bool enabled) 676: RuntimeEnabledFeatures::sharedFeatures().setLinkPreloadEnabled(enabled); Source/WebCore/testing/InternalSettings.h 113: static void setLinkPreloadEnabled(bool); 191: bool m_linkPreloadEnabled; Source/WebCore/testing/InternalSettings.idl 86: void setLinkPreloadEnabled(boolean enabled); 2. Remove uses of `internal.settings.setLinkPreloadEnabled` in tests: http/tests/fetch/redirectmode-and-preload.html http/tests/preload/delaying_onload_link_preload_after_discovery.html http/tests/preload/delaying_onload_link_preload_after_discovery_image.html http/tests/preload/download_resources.html http/tests/preload/download_resources_from_header_iframe.html http/tests/preload/download_resources_from_invalid_headers.html http/tests/preload/dynamic_adding_preload.html http/tests/preload/dynamic_remove_preload_href.html http/tests/preload/not_delaying_window_onload_before_discovery.html http/tests/preload/onerror_event.html http/tests/preload/onload_event.html http/tests/preload/resources/download_resources_from_header.php http/tests/preload/resources/invalid_resources_from_header.php http/tests/preload/single_download_preload_runner.html http/tests/security/cached-cross-origin-preloaded-css-stylesheet.html http/tests/security/cached-cross-origin-preloading-css-stylesheet.html Some tests might need updates based on line numbers once you simplify. 3. Add to the experimental features test page a way to quick test the experimental features menu (can be tested in MiniBrowser). Websites/webkit.org/experimental-features.html Hopefully there is a way to feature check this. Maybe checking for the attribute being null instead of undefined. > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:172 > +- (void)setLinkPreloadEnabled:(BOOL)flag; > +- (BOOL)linkPreloadEnabled; At the bottom we have been just using @property syntax. For example: @property (nonatomic) BOOL intersectionObserverEnabled; > Source/WebKit/mac/WebView/WebView.mm:2901 > + RuntimeEnabledFeatures::sharedFeatures().setLinkPreloadEnabled([preferences linkPreloadEnabled]); This could be moved to dot syntax: preferences.linkPreloadEnabled
Created attachment 299802 [details] Patch
Created attachment 299803 [details] Patch
(In reply to comment #5) > Comment on attachment 299690 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299690&action=review > > Neat! Thanks! :) > > r- only because I think there is some additional cleanup you should do when > switching to an experimental feature. > > 1. Remove the legacy way to enable the feature for tests (since test runners > will auto-enable it now!) > > Source/WebCore/testing/InternalSettings.cpp > 187: > RuntimeEnabledFeatures::sharedFeatures(). > setLinkPreloadEnabled(m_linkPreloadEnabled); > 674:void InternalSettings::setLinkPreloadEnabled(bool enabled) > 676: > RuntimeEnabledFeatures::sharedFeatures().setLinkPreloadEnabled(enabled); > > Source/WebCore/testing/InternalSettings.h > 113: static void setLinkPreloadEnabled(bool); > 191: bool m_linkPreloadEnabled; > > Source/WebCore/testing/InternalSettings.idl > 86: void setLinkPreloadEnabled(boolean enabled); Removed > > 2. Remove uses of `internal.settings.setLinkPreloadEnabled` in tests: > > http/tests/fetch/redirectmode-and-preload.html > http/tests/preload/delaying_onload_link_preload_after_discovery.html > > http/tests/preload/delaying_onload_link_preload_after_discovery_image.html > http/tests/preload/download_resources.html > http/tests/preload/download_resources_from_header_iframe.html > http/tests/preload/download_resources_from_invalid_headers.html > http/tests/preload/dynamic_adding_preload.html > http/tests/preload/dynamic_remove_preload_href.html > http/tests/preload/not_delaying_window_onload_before_discovery.html > http/tests/preload/onerror_event.html > http/tests/preload/onload_event.html > http/tests/preload/resources/download_resources_from_header.php > http/tests/preload/resources/invalid_resources_from_header.php > http/tests/preload/single_download_preload_runner.html > http/tests/security/cached-cross-origin-preloaded-css-stylesheet.html > http/tests/security/cached-cross-origin-preloading-css-stylesheet.html > Removed > Some tests might need updates based on line numbers once you simplify. > Updated > 3. Add to the experimental features test page a way to quick test the > experimental features menu (can be tested in MiniBrowser). > > Websites/webkit.org/experimental-features.html Added > > Hopefully there is a way to feature check this. Maybe checking for the > attribute being null instead of undefined. Of course there's a way to feature detect :) (Forcing new features through this demo page is a great way to make sure of that. I really like that) > > > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:172 > > +- (void)setLinkPreloadEnabled:(BOOL)flag; > > +- (BOOL)linkPreloadEnabled; > > At the bottom we have been just using @property syntax. For example: > > @property (nonatomic) BOOL intersectionObserverEnabled; Changed > > > Source/WebKit/mac/WebView/WebView.mm:2901 > > + RuntimeEnabledFeatures::sharedFeatures().setLinkPreloadEnabled([preferences linkPreloadEnabled]); > > This could be moved to dot syntax: > > preferences.linkPreloadEnabled Moved
Comment on attachment 299803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299803&action=review Looks good to me. I'll let someone actually familiar with the feature review =) > Source/WebKit2/Shared/WebPreferencesDefinitions.h:322 > + macro(LinkPreloadEnabled, linkPreloadEnabled, Bool, bool, DEFAULT_EXPERIMENTAL_FEATURES_ENABLED, "Link Preload", "Link preload support") \ The comment above says this should be alphabetically ordered. "[L]ink Preload" should go between "[G]amepads" and "[M]odern Media Controls".
Comment on attachment 299803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299803&action=review > Websites/webkit.org/experimental-features.html:148 > + <div class="test" id="LinkPreload"><p>Link Preload</p></div> I think we should be alphabetically sorted in here too, to match the menu.
Created attachment 299831 [details] Patch
Created attachment 299832 [details] Patch
Thanks! Reordered both
Comment on attachment 299832 [details] Patch Rejecting attachment 299832 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 299832, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ing file LayoutTests/http/tests/preload/single_download_preload-expected.txt patching file LayoutTests/http/tests/preload/single_download_preload.html patching file LayoutTests/http/tests/security/cached-cross-origin-preloaded-css-stylesheet.html patching file LayoutTests/http/tests/security/cached-cross-origin-preloading-css-stylesheet.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/2963454
Created attachment 300040 [details] Patch
Comment on attachment 300040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300040&action=review I see you have r? on this but I believe an earlier patch was reviewed already. I think you're just letting the buts run on it > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1014 > + [preferences setLinkPreloadEnabled:YES]; This should go up around line 900 in `static void enableExperimentalFeatures` with other experimental features.
> I think you're just letting the buts run on it bots*
(In reply to comment #16) > Comment on attachment 300040 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=300040&action=review > > I see you have r? on this but I believe an earlier patch was reviewed > already. I think you're just letting the buts run on it Yeah, forgot to turn off the r?... > > > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1014 > > + [preferences setLinkPreloadEnabled:YES]; > > This should go up around line 900 in `static void > enableExperimentalFeatures` with other experimental features. OK
Created attachment 300044 [details] Patch
Comment on attachment 300044 [details] Patch Clearing flags on attachment: 300044 Committed r211341: <http://trac.webkit.org/changeset/211341>
All reviewed patches have been landed. Closing bug.