Bug 215566 – Add setting for lazy iframe loading

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Rob Buis]

Add runtime flag for lazy iframe loading

[Rob Buis]

Created attachment 406716 [details]
Patch

[Sam Weinig]

Comment on attachment 406716 [details]
Patch

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

> Source/WebKit/Shared/WebPreferences.yaml:2038
> +  webcoreBinding: RuntimeEnabledFeatures

What's the reason behind using RuntimeEnabledFeatures rather than the default, Settings? In general, I think we should always prefer Settings over RuntimeEnabledFeatures as they are not global singletons and are automatically accessible in layout tests via InternalSettingsGenerated.

[Rob Buis]

Comment on attachment 406716 [details]
Patch

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

>> Source/WebKit/Shared/WebPreferences.yaml:2038
>> +  webcoreBinding: RuntimeEnabledFeatures
> 
> What's the reason behind using RuntimeEnabledFeatures rather than the default, Settings? In general, I think we should always prefer Settings over RuntimeEnabledFeatures as they are not global singletons and are automatically accessible in layout tests via InternalSettingsGenerated.

This will be used though to enable the loading attribute in HTMLIFrameElement.idl, AFAIK there is not a way to make that work using Settings.

[Sam Weinig]

(In reply to Rob Buis from comment #3)
> Comment on attachment 406716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406716&action=review
> 
> >> Source/WebKit/Shared/WebPreferences.yaml:2038
> >> +  webcoreBinding: RuntimeEnabledFeatures
> > 
> > What's the reason behind using RuntimeEnabledFeatures rather than the default, Settings? In general, I think we should always prefer Settings over RuntimeEnabledFeatures as they are not global singletons and are automatically accessible in layout tests via InternalSettingsGenerated.
> 
> This will be used though to enable the loading attribute in
> HTMLIFrameElement.idl, AFAIK there is not a way to make that work using
> Settings.

I believe `[EnabledBySetting=Foo] attribute DOMString loading;` would work for this.

[Rob Buis]

Comment on attachment 406716 [details]
Patch

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

>>>> Source/WebKit/Shared/WebPreferences.yaml:2038
>>>> +  webcoreBinding: RuntimeEnabledFeatures
>>> 
>>> What's the reason behind using RuntimeEnabledFeatures rather than the default, Settings? In general, I think we should always prefer Settings over RuntimeEnabledFeatures as they are not global singletons and are automatically accessible in layout tests via InternalSettingsGenerated.
>> 
>> This will be used though to enable the loading attribute in HTMLIFrameElement.idl, AFAIK there is not a way to make that work using Settings.
> 
> I believe `[EnabledBySetting=Foo] attribute DOMString loading;` would work for this.

Thanks, I was not aware of that. Does this mean LazyImageLoading runtime flag will need the same treatment?

[Sam Weinig]

(In reply to Rob Buis from comment #5)
> Comment on attachment 406716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406716&action=review
> 
> >>>> Source/WebKit/Shared/WebPreferences.yaml:2038
> >>>> +  webcoreBinding: RuntimeEnabledFeatures
> >>> 
> >>> What's the reason behind using RuntimeEnabledFeatures rather than the default, Settings? In general, I think we should always prefer Settings over RuntimeEnabledFeatures as they are not global singletons and are automatically accessible in layout tests via InternalSettingsGenerated.
> >> 
> >> This will be used though to enable the loading attribute in HTMLIFrameElement.idl, AFAIK there is not a way to make that work using Settings.
> > 
> > I believe `[EnabledBySetting=Foo] attribute DOMString loading;` would work for this.
> 
> Thanks, I was not aware of that. Does this mean LazyImageLoading runtime
> flag will need the same treatment?

Ideally, yes.

[Rob Buis]

Created attachment 406731 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 406732 [details]
Patch

[Rob Buis]

Comment on attachment 406716 [details]
Patch

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

>>>>>> Source/WebKit/Shared/WebPreferences.yaml:2038
>>>>>> +  webcoreBinding: RuntimeEnabledFeatures
>>>>> 
>>>>> What's the reason behind using RuntimeEnabledFeatures rather than the default, Settings? In general, I think we should always prefer Settings over RuntimeEnabledFeatures as they are not global singletons and are automatically accessible in layout tests via InternalSettingsGenerated.
>>>> 
>>>> This will be used though to enable the loading attribute in HTMLIFrameElement.idl, AFAIK there is not a way to make that work using Settings.
>>> 
>>> I believe `[EnabledBySetting=Foo] attribute DOMString loading;` would work for this.
>> 
>> Thanks, I was not aware of that. Does this mean LazyImageLoading runtime flag will need the same treatment?
> 
> Ideally, yes.

Thanks, will try to make a patch for that after this one.

[youenn fablet]

Comment on attachment 406732 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-base-url-2.tentative.html:14
> +    internals.settings.setLazyIframeLoadingEnabled(true);

It would not be great to upstream these changes to WPT.
Either you could update testharnessreport.js to enable the setting or you could use <!-- webkit-test-runner -->

[Rob Buis]

Created attachment 406842 [details]
Patch

[Rob Buis]

Comment on attachment 406732 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-base-url-2.tentative.html:14
>> +    internals.settings.setLazyIframeLoadingEnabled(true);
> 
> It would not be great to upstream these changes to WPT.
> Either you could update testharnessreport.js to enable the setting or you could use <!-- webkit-test-runner -->

Thanks, I attempted the testharnessreport.js approach, let me know if you had that in mind. If yes I suppose I should put it up on wpt first?
I don't think settings are set using <!-- webkit-test-runner --> before, so I think that one would need a lot of plumbing....

[Radar WebKit Bug Importer]

<rdar://problem/67678075>

[youenn fablet]

Comment on attachment 406842 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/resources/testharness.js:3674
> +    expose(enable_internal_setting, 'enable_internal_setting');

We do not want to touch testharness.js, this change should go in testharnessreport.js.
And we do not want to change the html/semantics/embedded-content/the-iframe-element/iframe-loading-xx files as well.

Something like the following in testharnessreport.js:
if (location.port == 8800 || location.port == 9443)
    internals.settings.setLazyIframeLoadingEnabled(true)

If you want to only enable lazy loading for these tests, you will need to write something like the following in testharnessreport.js:
if (location.port == 8800 || location.port == 9443)
    internals.settings.setLazyIframeLoadingEnabled(location.pathname.indexOf('iframe-loading-lazy') !== -1)

You can look at what we are doing for AudioContext there as an example.

[Rob Buis]

Created attachment 407184 [details]
Patch

[Rob Buis]

Comment on attachment 406842 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/resources/testharness.js:3674
>> +    expose(enable_internal_setting, 'enable_internal_setting');
> 
> We do not want to touch testharness.js, this change should go in testharnessreport.js.
> And we do not want to change the html/semantics/embedded-content/the-iframe-element/iframe-loading-xx files as well.
> 
> Something like the following in testharnessreport.js:
> if (location.port == 8800 || location.port == 9443)
>     internals.settings.setLazyIframeLoadingEnabled(true)
> 
> If you want to only enable lazy loading for these tests, you will need to write something like the following in testharnessreport.js:
> if (location.port == 8800 || location.port == 9443)
>     internals.settings.setLazyIframeLoadingEnabled(location.pathname.indexOf('iframe-loading-lazy') !== -1)
> 
> You can look at what we are doing for AudioContext there as an example.

Thanks for the hints! I put up a new patch. I am not sure for the AudioContext example, maybe the patch is still under development?

[youenn fablet]

Comment on attachment 407184 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/resources/testharnessreport.js:58
> +    }

This should go in LayoutTests/resources/testharnessreport.js which should override this file.
There is already a section with if (location.port == 8800 || location.port == 9443)

[Rob Buis]

Created attachment 407194 [details]
Patch

[EWS]

Committed r266127: <https://trac.webkit.org/changeset/266127>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407194 [details].