Bug 215566

Created attachment 406716 [details] Patch

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.

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.

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

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?

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

Created attachment 406731 [details] Patch

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

Created attachment 406732 [details] Patch

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.

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

Created attachment 406842 [details] Patch

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

<rdar://problem/67678075>

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.

Created attachment 407184 [details] Patch

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?

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)

Created attachment 407194 [details] Patch

Committed r266127: <https://trac.webkit.org/changeset/266127> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407194 [details].