Bug 167201 – Add Link Preload as an off-by-default experimental feature menu item.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Add Link Preload as an off-by-default experimental feature menu item.

[Yoav Weiss]

Created attachment 299245 [details]
Patch

[Yoav Weiss]

Created attachment 299584 [details]
Patch

[Yoav Weiss]

Rebased. PTAL?

[Yoav Weiss]

Created attachment 299690 [details]
Patch

[Joseph Pecoraro]

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

[Yoav Weiss]

Created attachment 299802 [details]
Patch

[Yoav Weiss]

Created attachment 299803 [details]
Patch

[Yoav Weiss]

(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

[Joseph Pecoraro]

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".

[Joseph Pecoraro]

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.

[Yoav Weiss]

Created attachment 299831 [details]
Patch

[Yoav Weiss]

Created attachment 299832 [details]
Patch

[Yoav Weiss]

Thanks! Reordered both

[WebKit Commit Bot]

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

[Yoav Weiss]

Created attachment 300040 [details]
Patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

> I think you're just letting the buts run on it

bots*

[Yoav Weiss]

(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

[Yoav Weiss]

Created attachment 300044 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 300044 [details]
Patch

Clearing flags on attachment: 300044

Committed r211341: <http://trac.webkit.org/changeset/211341>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.