Bug 200764

Created attachment 376371 [details] Patch

Created attachment 376381 [details] Patch

Created attachment 376383 [details] Patch

Created attachment 376400 [details] Patch

This patch has been kept simple: - no metadata fetch to get the image dimensions like in my earlier prototype. - 'auto' is treated as 'eager' for now, as otherwise too many things would break. - placeholder painting is pretty basic. Note that https://bugs.webkit.org/show_bug.cgi?id=200662 should probably go in before this patch, to make this one even simpler/smaller.

Created attachment 376872 [details] Patch

Comment on attachment 376872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376872&action=review > Source/WebCore/ChangeLog:30 > + * Sources.txt: > + * WebCore.xcodeproj/project.pbxproj: Does the WinCairo failure mean we also need to do something for CMake?

Created attachment 377113 [details] Patch

(In reply to Darin Adler from comment #7) > Comment on attachment 376872 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376872&action=review > > > Source/WebCore/ChangeLog:30 > > + * Sources.txt: > > + * WebCore.xcodeproj/project.pbxproj: > > Does the WinCairo failure mean we also need to do something for CMake? Thanks for the hint, I think the WinCairo bot must have been in an unclean state, since the same patch applied this time. Now everything shows up green and is ready for review.

Comment on attachment 377113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377113&action=review > LayoutTests/http/tests/lazyload/attribute.html:12 > + <img id="eager_attribute_img" src='../loading/resources/dup-image2.png' loading="eager"> It seems you could easily add tests for ASCII case-insensitiveness here? (loading is indeed an enumerated attribute so your code is correct: https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#keywords-and-enumerated-attributes )

Created attachment 377592 [details] Patch

(In reply to Frédéric Wang (:fredw) from comment #10) > Comment on attachment 377113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377113&action=review > > > LayoutTests/http/tests/lazyload/attribute.html:12 > > + <img id="eager_attribute_img" src='../loading/resources/dup-image2.png' loading="eager"> > > It seems you could easily add tests for ASCII case-insensitiveness here? Done!

Created attachment 377976 [details] Patch

Created attachment 377983 [details] Patch

Created attachment 377984 [details] Patch

Comment on attachment 377984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377984&action=review > Source/WebCore/ChangeLog:20 > + [1] https://github.com/whatwg/html/pull/3752/files Is there an actual spec for this? > Source/WebCore/dom/Document.cpp:8294 > + m_lazyLoadImageObserver = new LazyLoadImageObserver(); Where is m_lazyLoadImageObserver deleted? This should be m_lazyLoadImageObserver = makeUnique<LazyLoadImageObserver>(); > Source/WebCore/html/HTMLImageElement.cpp:850 > +const AtomString& HTMLImageElement::loading() const What is the purpose of this function and where it is used? I would expect this function to return an enum value instead of of AtomString. > Source/WebCore/html/HTMLImageElement.cpp:863 > +void HTMLImageElement::setLoading(const AtomString& value) Where is this function called? > Source/WebCore/html/HTMLImageElement.cpp:880 > +HTMLImageElement::LazyLoadDimensionType HTMLImageElement::getAttributeLazyLoadDimensionType(const String& attributeValue) > +{ > + auto optionalDimension = parseValidHTMLNonNegativeInteger(attributeValue); > + if (optionalDimension && !attributeValue.endsWith('%')) { > + return (optionalDimension.value() <= kMinDimensionToLazyLoad) > + ? LazyLoadDimensionType::AbsoluteSmall > + : LazyLoadDimensionType::AbsoluteNotSmall; > + } > + return LazyLoadDimensionType::NotAbsolute; > +} Usually we do not use get...() in the function names. Also the code will be clearer if we use something like this: Optional<IntSize> lazyLoadImageSize() { // return { } if width or height attribute is missing. } And the caller can use it like this: if (auto size = lazyLoadImageSize()) { if (size->area() < MinLazyLoadImageSize) return; } Also should not we try reading the first few bytes from the image to read its actual size? > Source/WebCore/html/HTMLImageElement.cpp:882 > +HTMLImageElement::LazyLoadDimensionType HTMLImageElement::getInlineStyleDimensionsType(const StyleProperties* propertySet) Why do we care about the style dimension when deciding to load the image eagerly or lazily? > Source/WebCore/html/HTMLImageElement.cpp:893 > + if (!widthPrim.isPx() || !heightPrim.isPx()) > + return LazyLoadDimensionType::NotAbsolute; Why do we consider the point units only and ignore the rest? > Source/WebCore/html/HTMLImageElement.cpp:895 > + return (heightPrim.doubleValue() <= kMinDimensionToLazyLoad) && (widthPrim.doubleValue() <= kMinDimensionToLazyLoad) > + ? LazyLoadDimensionType::AbsoluteSmall : LazyLoadDimensionType::AbsoluteNotSmall; You can make this function returns Optional<FloatSize> and check its area. > Source/WebCore/html/LazyLoadImageObserver.cpp:38 > +class IntersectionObserverCallbackImpl final : public IntersectionObserverCallback { This is very generic name. Should it be LazyLoadImageIntersectionObserverCallback? > Source/WebCore/html/LazyLoadImageObserver.cpp:47 > + IntersectionObserverCallbackImpl(Document* document) > + : IntersectionObserverCallback(document), m_document(document) > + { > + } Why do we need to pass the Document to this class? It deals with Elements. From the element you get element->document(). > Source/WebCore/html/LazyLoadImageObserver.cpp:59 > + m_document->ensureLazyLoadImageObserver().stopMonitoring(*element); If the element is not an HTMLImageElement, why do we have to call stopMonitoring() for it? > Source/WebCore/html/LazyLoadImageObserver.cpp:69 > +Document* getRootDocumentOrNull(const Element& element) Please do not use "get..()" in the function name. Also there is no need to add "OrNull" if the function returns a pointer. Why do not we use element.document().topDocument()? > Source/WebCore/html/LazyLoadImageObserver.cpp:81 > + if (Document* document = getRootDocumentOrNull(element)) > + document->ensureLazyLoadImageObserver().startMonitoringNearViewport(document, element); I would suggest using the name "observe" for this function since it is going to call IntersectionObserver:: observe(). void LazyLoadImageObserver:: observe(Element& element) { Document* document = getRootDocumentOrNull(element); if (!document) return; auto& observer = document->ensureLazyLoadImageObserver(); auto* intersectionObserver = observer.ensureIntersectionObserver(); if (!intersectionObserver) return; intersectionObserver->observe(element); } > Source/WebCore/html/LazyLoadImageObserver.cpp:88 > +void LazyLoadImageObserver::stopMonitoring(Element& element) > +{ > + if (Document* document = getRootDocumentOrNull(element)) > + document->ensureLazyLoadImageObserver().m_lazyLoadIntersectionObserver->unobserve(element); > +} Ditto. > Source/WebCore/html/LazyLoadImageObserver.cpp:90 > +LazyLoadImageObserver::LazyLoadImageObserver() = default; You can move this to the header file. > Source/WebCore/html/LazyLoadImageObserver.cpp:103 > +void LazyLoadImageObserver::startMonitoringNearViewport(Document* rootDocument, Element& element) > +{ > + if (!m_lazyLoadIntersectionObserver) { > + auto callback = IntersectionObserverCallbackImpl::create(rootDocument); > + auto options = IntersectionObserver::Init { nullptr, "", { } }; > + auto observer = IntersectionObserver::create(*rootDocument, WTFMove(callback), WTFMove(options)); > + if (observer.hasException()) > + return; > + m_lazyLoadIntersectionObserver = observer.returnValue().ptr(); > + } > + m_lazyLoadIntersectionObserver->observe(element); > +} This should be called ensureIntersectionObserver(). > Source/WebCore/html/LazyLoadImageObserver.h:40 > + static void startMonitoring(Element&); > + static void stopMonitoring(Element&); I think these functions should live the document object instead of being static since we have to call Document::ensureLazyLoadImageObserver(). > Source/WebCore/loader/ImageLoader.cpp:71 > +static bool isLazyLoadableImage(const HTMLImageElement& htmlImage, const URL& url) Why is this function static? And why does it live in the ImageLoader source file? I think this should live in HTMLImageElement. If we do so, we will not need to define these functions as static also: HTMLImageElement::getAttributeLazyLoadDimensionType() HTMLImageElement::getInlineStyleDimensionsType() > Source/WebCore/loader/ImageLoader.cpp:77 > + if (!htmlImage.createdByParser()) > + return false; > + if (!url.protocolIsInHTTPFamily()) > + return false; Why do we need to check for these conditions to decide the loading strategy? Are not the loadingAttr value and the image size enough and sufficient to make this decision? > Source/WebCore/loader/ImageLoader.cpp:85 > + // Avoid lazyloading if width and height attributes are small. This > + // heuristic helps avoid double fetching tracking pixels. > + if (HTMLImageElement::getAttributeLazyLoadDimensionType(htmlImage.attributeWithoutSynchronization(HTMLNames::widthAttr)) == HTMLImageElement::LazyLoadDimensionType::AbsoluteSmall > + && HTMLImageElement::getAttributeLazyLoadDimensionType(htmlImage.attributeWithoutSynchronization(HTMLNames::heightAttr)) == HTMLImageElement::LazyLoadDimensionType::AbsoluteSmall) { > + return false; > + } I think the image size is a better check. > Source/WebCore/loader/ImageLoader.cpp:601 > +void ImageLoader::loadDeferredImage() > +{ > + if (m_lazyImageLoadState != LazyImageLoadState::Deferred) > + return; > + m_lazyImageLoadState = LazyImageLoadState::FullImage; > + updateFromElement(); > +} I am confused by this function. updateFromElement() will request the image loading. When the image finishes loading, it will call ImageLoader::notifyFinished() which will see that m_lazyImageLoadState == FullImage so it not call LazyLoadImageObserver::stopMonitoring() for it. > Source/WebCore/loader/ImageLoader.h:119 > + LazyImageLoadState m_lazyImageLoadState { LazyImageLoadState::None }; I think there is no need for this member. You can rely on m_imageComplete and a new function in LazyLoadImageObserver called bool isObserved(Element& element) const { return m_lazyLoadIntersectionObserver && m_lazyLoadIntersectionObserver.observationTargets().contains(*element); } None => !m_imageComplete && !isObserved() Deferred => !m_imageComplete && isObserved() FullImage => m_imageComplete > Source/WebCore/loader/cache/CachedImage.h:96 > + void setDeferred() { m_isDeferred = true; } > + bool isDeferred() const { return m_isDeferred && stillNeedsLoad(); } I think this is the wrong place to put these functions. The Lazy Image loading is only enabled for HTMLImageElement. So I think HTMLImageElement is the right place to ask these question from. HTMLImageElement also has access to the ImageLoader so you can get its LazyImageLoadState. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:200 > +ResourceErrorOr<CachedResourceHandle<CachedImage>> CachedResourceLoader::requestImage(CachedResourceRequest&& request, bool defer) Should not we use DeferOption instead of using bool? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:214 > + defer = clientDefersImage(request.resourceRequest().url()); I think clientDefersImage() should return DeferOption. It is confusing to use enum and bool for the same purpose and you keep switching between them. So let's stick with the enum only. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:217 > + if (cachedImage) > + cachedImage.value()->setDeferred(); No need for this call because ImageLoader::updateFromElement() will call LazyLoadImageObserver::startMonitoring(). Knowing the element is observed is the same calling setDeferred() here. > Source/WebCore/rendering/RenderImage.cpp:434 > + if (!imageResource().cachedImage() || imageResource().cachedImage()->isDeferred() || imageResource().errorOccurred()) { We should ask the element not the cachedImage() this question. > LayoutTests/http/tests/lazyload/attribute-expected.txt:3 > +CONSOLE MESSAGE: line 2451: Error: assert_equals: expected (object) null but got (string) "eager" > +CONSOLE MESSAGE: line 2451: Error: assert_equals: expected "auto" but got "eager" > +CONSOLE MESSAGE: line 2451: Error: assert_equals: expected "invalid-value-default" but got "eager" Why these assertions are failing.

Created attachment 378079 [details] Patch

Created attachment 378085 [details] Patch

Comment on attachment 378085 [details] Patch Attachment 378085 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13000539 Number of test failures exceeded the failure limit.

Created attachment 378090 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 378095 [details] Patch

Comment on attachment 378095 [details] Patch Attachment 378095 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13001129 New failing tests: fast/images/object-image-hide-show.html fast/css/image-object-hover-inherit.html svg/as-image/svg-object-intrinsic-size.html fast/images/exif-orientation-image-object.html fast/css/object-position/object-position-input-image.html fast/css/object-fit/object-fit-input-image.html fast/images/exif-orientation-content.html fast/images/object-data-url-case-insensitivity.html fast/replaced/outline-replaced-elements-offset.html

Created attachment 378103 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 378095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378095&action=review > Source/WebCore/html/HTMLImageElement.cpp:880 > +HTMLImageElement::LazyLoadDimensionType HTMLImageElement::getAttributeLazyLoadDimensionType(const String& attributeValue) > +{ > + auto optionalDimension = parseValidHTMLNonNegativeInteger(attributeValue); > + if (optionalDimension && !attributeValue.endsWith('%')) { > + return (optionalDimension.value() <= kMinDimensionToLazyLoad) > + ? LazyLoadDimensionType::AbsoluteSmall > + : LazyLoadDimensionType::AbsoluteNotSmall; > + } > + return LazyLoadDimensionType::NotAbsolute; > +} Is this dependency on image size described by the spec? I don't see it. > Source/WebCore/html/HTMLImageElement.cpp:882 > +HTMLImageElement::LazyLoadDimensionType HTMLImageElement::getInlineStyleDimensionsType(const StyleProperties* propertySet) Why only consult inline style, and not just using computed style? Are you trying to avoid forced layouts? > Source/WebCore/html/HTMLImageElement.cpp:892 > + if (!widthPrim.isPx() || !heightPrim.isPx()) What about other absolute units like pt, in, cm and font-size relative units like em, rem? > Source/WebCore/html/LazyLoadImageObserver.cpp:85 > + Document* document = rootDocument(element); > + if (!document) > + return; > + auto& observer = document->ensureLazyLoadImageObserver(); > + auto* intersectionObserver = observer.ensureIntersectionObserver(document); > + if (!intersectionObserver) > + return; > + intersectionObserver->observe(element); This seems wrong. The top document may have a different origin than the document, so an observer on the root won't be able to observe images in cross-origin subframes. Does this code do the right thing when Elements are moved between documents? > Source/WebCore/html/LazyLoadImageObserver.cpp:108 > + if (!m_lazyLoadIntersectionObserver) { > + auto callback = LazyImageLoadIntersectionObserverCallback::create(rootDocument); > + auto options = IntersectionObserver::Init { nullptr, "", { } }; > + auto observer = IntersectionObserver::create(*rootDocument, WTFMove(callback), WTFMove(options)); > + if (observer.hasException()) > + return nullptr; > + m_lazyLoadIntersectionObserver = observer.returnValue().ptr(); The spec talks about initiating loads when an image enters the viewport, but I would think we'd want some additional coverage area so there's a change the image can start loading earlier. Ideally we'd kick off image loads at the same time we make tiles for a region. > Source/WebCore/loader/ImageLoader.cpp:76 > + if (!url.protocolIsInHTTPFamily()) Is this in the spec? > Source/WebCore/loader/ImageLoader.cpp:81 > + // Avoid lazyloading if width and height attributes are small. This > + // heuristic helps avoid double fetching tracking pixels. Is the intention to avoid changing behavior for tracking pixels? > Source/WebCore/loader/ImageLoader.cpp:87 > + // small enough. This heuristic helps avoid double fetching tracking pixels. Why would double-fetching ever happen? > Source/WebCore/loader/ImageLoader.cpp:215 > + if (isLazyLoadableImage(imageElement, resourceRequest.url()) > + && RuntimeEnabledFeatures::sharedFeatures().lazyImageLoadingEnabled()) { You can unwrap these lines. > Source/WebCore/loader/ImageLoader.cpp:337 > + // A placeholder was requested, but the result was an error or a full image. Who's requesting the placeholder? > Source/WebCore/loader/ImageLoader.h:85 > + enum class LazyImageLoadState { None, Deferred, FullImage }; : uint8_t > Source/WebCore/loader/cache/CachedResourceLoader.cpp:201 > +ResourceErrorOr<CachedResourceHandle<CachedImage>> CachedResourceLoader::requestImage(CachedResourceRequest&& request, DeferOption defer) defer -> deferOption > Source/WebCore/loader/cache/CachedResourceLoader.h:65 > +enum class DeferOption { NoDefer, DeferredByClient }; Defer what? This should be LoadingDeferral or something.

Comment on attachment 378095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378095&action=review > Source/WebCore/html/LazyLoadImageObserver.cpp:94 > + auto* intersectionObserver = observer.ensureIntersectionObserver(document); You do not need to ensureIntersectionObserver() here. I think you should assert m_lazyLoadIntersectionObserver is not null and element is already observed. >> Source/WebCore/html/LazyLoadImageObserver.cpp:108 >> + m_lazyLoadIntersectionObserver = observer.returnValue().ptr(); > > The spec talks about initiating loads when an image enters the viewport, but I would think we'd want some additional coverage area so there's a change the image can start loading earlier. Ideally we'd kick off image loads at the same time we make tiles for a region. Do we want to do something similar to async image decoding but for loading? Or do we want to include loadDeferredImage() inside the async image decoding? > Source/WebCore/loader/ImageLoader.cpp:221 > + DeferOption defer = DeferOption::NoDefer; > + if (m_lazyImageLoadState == LazyImageLoadState::None) { > + if (is<HTMLImageElement>(element())) { > + auto& imageElement = downcast<HTMLImageElement>(element()); > + if (isLazyLoadableImage(imageElement, resourceRequest.url()) > + && RuntimeEnabledFeatures::sharedFeatures().lazyImageLoadingEnabled()) { > + m_lazyImageLoadState = LazyImageLoadState::Deferred; > + defer = DeferOption::DeferredByClient; > + imageElement.setDeferred(); > + } > + } > + } I think these calculations should be moved down under the else of if (m_loadManually). setLoadManual() is only called for ImageDocument which, I think, should not defer loading the images. > Source/WebCore/loader/ImageLoader.cpp:236 > + newImage = document.cachedResourceLoader().requestImage(WTFMove(request), defer).value_or(nullptr); There is no need for the local member 'defer' since its value is equivalent to: m_lazyImageLoadState == LazyImageLoadState::Deferred ? DeferOption::DeferredByClient : DeferOption::NoDefer; Or even you can pass m_lazyImageLoadState to CachedResourceLoader::requestImage() and get rid of the enum DeferOption. >> Source/WebCore/loader/cache/CachedResourceLoader.h:65 >> +enum class DeferOption { NoDefer, DeferredByClient }; > > Defer what? This should be LoadingDeferral or something. enum class ImageRequestOption : uint8_t { Now, Defer }; ?

Created attachment 378164 [details] Patch

Created attachment 378172 [details] Patch

Created attachment 378175 [details] Patch

Created attachment 378186 [details] Patch

Created attachment 378187 [details] Patch

Comment on attachment 378187 [details] Patch Attachment 378187 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13005232 New failing tests: http/tests/lazyload/lazy.html http/tests/lazyload/attribute.html

Created attachment 378188 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 378189 [details] Patch

Comment on attachment 378189 [details] Patch Attachment 378189 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13005547 New failing tests: http/tests/lazyload/lazy.html http/tests/lazyload/attribute.html

Created attachment 378200 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review A few comments; I had a little time to review but didn’t get through everything. > Source/WebCore/dom/Document.h:1536 > + LazyLoadImageObserver& ensureLazyLoadImageObserver(); WebKit coding style says you don’t use the word "ensure" in the name of a function like this. > Source/WebCore/html/HTMLImageElement.cpp:247 > + else if (name == loadingAttr && equalLettersIgnoringASCIICase(value, "eager")) > + loadDeferredImage(); This typically isn’t the correct pattern for an attribute. It implements a "latching" behavior that remember "was the loading attribute ever set to the value 'eager' now or in the past". Or you could say it supports only the transition from loading not being "eager" (no attribute present, a different value) to loading being "eager". To correctly support an attribute typically we also have to support a transition in the opposite direction, removing the attribute or changing its value. We don’t want an image element to be permanently in "eager mode" just because for one moment in time the attribute had this value. This is typically done more correctly by writing something like this instead: else if (name == loadingAttr) setLoadsDeferredImage(equalLettersIgnoringASCIICase(value, "eager")); This handles the case of the attribute being added, removed, or its value changed. It's important to write the setter code in a way that avoids doing work when the state isn’t changed. But perhaps there is some reason this value needs to be latched? > Source/WebCore/html/HTMLImageElement.cpp:861 > +const AtomString& HTMLImageElement::loadingForBindings() const > +{ > + static NeverDestroyed<AtomString> autoValue("auto", AtomString::ConstructFromLiteral); > + static NeverDestroyed<AtomString> eager("eager", AtomString::ConstructFromLiteral); > + static NeverDestroyed<AtomString> lazy("lazy", AtomString::ConstructFromLiteral); > + auto attributeValue = attributeWithoutSynchronization(HTMLNames::loadingAttr); > + if (equalLettersIgnoringASCIICase(attributeValue, "eager")) > + return eager; > + if (equalLettersIgnoringASCIICase(attributeValue, "lazy")) > + return lazy; > + return autoValue; > +} Normally in modern DOM attributes this would be done with reflection and an enumeration. I don’t have a source tree handy at the moment to search, but there would be [Reflect] in the IDL file and enumeration values in there too. There may be some reason we can’t do it in this case, but I’d like to know why. > Source/WebCore/html/HTMLImageElement.cpp:880 > + // Do not lazyload image elements created from javascript. I don’t think we should coin a word "lazy load". I would write: // Never do lazy loading for image elements created from JavaScript. But also I am really confused by that rule! > Source/WebCore/html/HTMLImageElement.cpp:885 > + if (!createdByParser()) > + return false; > + if (!equalLettersIgnoringASCIICase(attributeWithoutSynchronization(HTMLNames::loadingAttr), "lazy")) > + return false; > + return true; I would find this easier to read as a boolean expression rather than a pair of if statements: return createdByParser() && equalLettersIgnoringASCIICase(attributeWithoutSynchronization(HTMLNames::loadingAttr), "lazy"); Even if broken over two lines I would find this easier to read. > Source/WebCore/html/LazyLoadImageObserver.cpp:40 > + static Ref<LazyImageLoadIntersectionObserverCallback> create(Document* document) Should take Document&, not Document*. > Source/WebCore/html/LazyLoadImageObserver.cpp:44 > + LazyImageLoadIntersectionObserverCallback(Document* document) Should be private, and should be explicit and should take Document&. > Source/WebCore/html/LazyLoadImageObserver.cpp:48 > + CallbackResult<void> handleEvent(const Vector<Ref<IntersectionObserverEntry>>& entries, IntersectionObserver& intersectionObserver) Since this overrides a virtual function it should be marked final in WebKit coding style even though the class is final too. Also should be private. > Source/WebCore/html/LazyLoadImageObserver.cpp:98 > +IntersectionObserver* LazyLoadImageObserver::ensureIntersectionObserver(Document* rootDocument) I know Said suggested this, but I don’t understand why this has the word "ensure" in it. Normally we just would call this "intersectionObserver" in WebKit coding style. Also should take a Document&. > Source/WebCore/html/LazyLoadImageObserver.cpp:102 > + auto options = IntersectionObserver::Init { nullptr, "", { } }; emptyString() is more efficient than "". But also, maybe there is a cleaner way to write this. Perhaps we can leave out the options entirely? Change the IntersectionObserver::create to overload for a case like this with no options? > Source/WebCore/html/LazyLoadImageObserver.h:44 > + IntersectionObserver* ensureIntersectionObserver(Document*); This argument should be Document& instead of Document*. > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:218 > + if (match(attributeName, loadingAttr) && m_lazyloadAttribute.isNull()) { > + m_lazyloadAttribute = attributeValue; > + break; > + } Again, why the one-time latching behavior. Can’t we change the lazy load attribute more than once? Should this forever remember the first value it had? > Source/WebCore/loader/ImageLoader.h:84 > + // LazyImages: Defer the image load until the image is near the viewport. Should not merge the words "lazy images" into a since LazyImages word unless that’s an established term of art. > Source/WebCore/rendering/RenderImage.cpp:425 > + if (!is<HTMLImageElement>(element)) > + return false; > + return downcast<HTMLImageElement>(element)->isDeferred(); return is<HTMLImageElement>(element) && downcast<HTMLImageElement>(*element)->isDeferred();

Comment on attachment 377984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377984&action=review Thanks for the review! Sorry that the patch was bigger than needed and not all reviewing was necessary. >> Source/WebCore/ChangeLog:20 >> + [1] https://github.com/whatwg/html/pull/3752/files > > Is there an actual spec for this? No, that is the spec, but it is not merged yet since some things still need to be addressed. >> Source/WebCore/dom/Document.cpp:8294 >> + m_lazyLoadImageObserver = new LazyLoadImageObserver(); > > Where is m_lazyLoadImageObserver deleted? This should be > > m_lazyLoadImageObserver = makeUnique<LazyLoadImageObserver>(); Fixed. >> Source/WebCore/html/HTMLImageElement.cpp:850 >> +const AtomString& HTMLImageElement::loading() const > > What is the purpose of this function and where it is used? I would expect this function to return an enum value instead of of AtomString. This is for the bindings, I changed the method name to reflect that. >> Source/WebCore/html/HTMLImageElement.cpp:863 >> +void HTMLImageElement::setLoading(const AtomString& value) > > Where is this function called? See above. >> Source/WebCore/html/HTMLImageElement.cpp:880 >> +} > > Usually we do not use get...() in the function names. Also the code will be clearer if we use something like this: > > Optional<IntSize> lazyLoadImageSize() > { > // return { } if width or height attribute is missing. > } > > And the caller can use it like this: > > if (auto size = lazyLoadImageSize()) { > if (size->area() < MinLazyLoadImageSize) > return; > } > > Also should not we try reading the first few bytes from the image to read its actual size? Sorry, I did not do a good job of splitting up my original patch, this part is only important when doing range requests, which I think you also mean by "reading the first few bytes"? It adds complexity and for instance chromium is thinking of removing it, so I left it out. >> Source/WebCore/html/HTMLImageElement.cpp:882 >> +HTMLImageElement::LazyLoadDimensionType HTMLImageElement::getInlineStyleDimensionsType(const StyleProperties* propertySet) > > Why do we care about the style dimension when deciding to load the image eagerly or lazily? Sorry, as above, I should not have put this part up. >> Source/WebCore/html/HTMLImageElement.cpp:893 >> + return LazyLoadDimensionType::NotAbsolute; > > Why do we consider the point units only and ignore the rest? Ditto. >> Source/WebCore/html/HTMLImageElement.cpp:895 >> + ? LazyLoadDimensionType::AbsoluteSmall : LazyLoadDimensionType::AbsoluteNotSmall; > > You can make this function returns Optional<FloatSize> and check its area. Ditto. >> Source/WebCore/html/LazyLoadImageObserver.cpp:38 >> +class IntersectionObserverCallbackImpl final : public IntersectionObserverCallback { > > This is very generic name. Should it be LazyLoadImageIntersectionObserverCallback? Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:47 >> + } > > Why do we need to pass the Document to this class? It deals with Elements. From the element you get element->document(). It is needed for ActiveDOMCallback. >> Source/WebCore/html/LazyLoadImageObserver.cpp:59 >> + m_document->ensureLazyLoadImageObserver().stopMonitoring(*element); > > If the element is not an HTMLImageElement, why do we have to call stopMonitoring() for it? Well spotted, I moved the line into the if section. >> Source/WebCore/html/LazyLoadImageObserver.cpp:69 >> +Document* getRootDocumentOrNull(const Element& element) > > Please do not use "get..()" in the function name. Also there is no need to add "OrNull" if the function returns a pointer. > > Why do not we use element.document().topDocument()? Done (I think). >> Source/WebCore/html/LazyLoadImageObserver.cpp:81 >> + document->ensureLazyLoadImageObserver().startMonitoringNearViewport(document, element); > > I would suggest using the name "observe" for this function since it is going to call IntersectionObserver:: observe(). > > void LazyLoadImageObserver:: observe(Element& element) > { > Document* document = getRootDocumentOrNull(element); > if (!document) > return; > auto& observer = document->ensureLazyLoadImageObserver(); > auto* intersectionObserver = observer.ensureIntersectionObserver(); > if (!intersectionObserver) > return; > intersectionObserver->observe(element); > } Nice! done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:88 >> +} > > Ditto. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:90 >> +LazyLoadImageObserver::LazyLoadImageObserver() = default; > > You can move this to the header file. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:103 >> +} > > This should be called ensureIntersectionObserver(). Done. >> Source/WebCore/html/LazyLoadImageObserver.h:40 >> + static void stopMonitoring(Element&); > > I think these functions should live the document object instead of being static since we have to call Document::ensureLazyLoadImageObserver(). I'll try it. >> Source/WebCore/loader/ImageLoader.cpp:71 >> +static bool isLazyLoadableImage(const HTMLImageElement& htmlImage, const URL& url) > > Why is this function static? And why does it live in the ImageLoader source file? I think this should live in HTMLImageElement. If we do so, we will not need to define these functions as static also: > HTMLImageElement::getAttributeLazyLoadDimensionType() > HTMLImageElement::getInlineStyleDimensionsType() I did end up moving it there, done. >> Source/WebCore/loader/ImageLoader.cpp:77 >> + return false; > > Why do we need to check for these conditions to decide the loading strategy? Are not the loadingAttr value and the image size enough and sufficient to make this decision? The first check is there to prevent a situation where a script waits for the image to finish loading, but the image can't finish loading because it is deferred and not added to the page yet, so the user can't scroll to it. But I have to test a bit if this is actually possible. The second check was for data URI images but I think they would be okay to lazy load, will remove the check. >> Source/WebCore/loader/ImageLoader.cpp:85 >> + } > > I think the image size is a better check. Code was removed. >> Source/WebCore/loader/ImageLoader.cpp:601 >> +} > > I am confused by this function. updateFromElement() will request the image loading. When the image finishes loading, it will call ImageLoader::notifyFinished() which will see that m_lazyImageLoadState == FullImage so it not call LazyLoadImageObserver::stopMonitoring() for it. I agree that it is confusing. I'll see if the setting to LazyImageLoadState::FullImage can be removed here. >> Source/WebCore/loader/ImageLoader.h:119 >> + LazyImageLoadState m_lazyImageLoadState { LazyImageLoadState::None }; > > I think there is no need for this member. You can rely on m_imageComplete and a new function in LazyLoadImageObserver called > > bool isObserved(Element& element) const > { > return m_lazyLoadIntersectionObserver && m_lazyLoadIntersectionObserver.observationTargets().contains(*element); > } > > None => !m_imageComplete && !isObserved() > Deferred => !m_imageComplete && isObserved() > FullImage => m_imageComplete I tried this but calling isObserved is cumbersome, and it would need to be called quite a few times. >> Source/WebCore/loader/cache/CachedImage.h:96 >> + bool isDeferred() const { return m_isDeferred && stillNeedsLoad(); } > > I think this is the wrong place to put these functions. The Lazy Image loading is only enabled for HTMLImageElement. So I think HTMLImageElement is the right place to ask these question from. HTMLImageElement also has access to the ImageLoader so you can get its LazyImageLoadState. Done. >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:200 >> +ResourceErrorOr<CachedResourceHandle<CachedImage>> CachedResourceLoader::requestImage(CachedResourceRequest&& request, bool defer) > > Should not we use DeferOption instead of using bool? Done. >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:214 >> + defer = clientDefersImage(request.resourceRequest().url()); > > I think clientDefersImage() should return DeferOption. It is confusing to use enum and bool for the same purpose and you keep switching between them. So let's stick with the enum only. Fixed. >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:217 >> + cachedImage.value()->setDeferred(); > > No need for this call because ImageLoader::updateFromElement() will call LazyLoadImageObserver::startMonitoring(). Knowing the element is observed is the same calling setDeferred() here. Removed. >> Source/WebCore/rendering/RenderImage.cpp:434 >> + if (!imageResource().cachedImage() || imageResource().cachedImage()->isDeferred() || imageResource().errorOccurred()) { > > We should ask the element not the cachedImage() this question. Done. >> LayoutTests/http/tests/lazyload/attribute-expected.txt:3 >> +CONSOLE MESSAGE: line 2451: Error: assert_equals: expected "invalid-value-default" but got "eager" > > Why these assertions are failing. Fixed. This is because our loading=auto behavior is different than what the test(s) assume.

Comment on attachment 378095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378095&action=review Simon, thanks for the review! In hindsight I left too much stuff in the patch, a lot of your comments were about the tracking pixels/range request part which should have been stripped out. >> Source/WebCore/html/HTMLImageElement.cpp:880 >> +} > > Is this dependency on image size described by the spec? I don't see it. The code was removed. >> Source/WebCore/html/HTMLImageElement.cpp:882 >> +HTMLImageElement::LazyLoadDimensionType HTMLImageElement::getInlineStyleDimensionsType(const StyleProperties* propertySet) > > Why only consult inline style, and not just using computed style? Are you trying to avoid forced layouts? The code was removed. >> Source/WebCore/html/HTMLImageElement.cpp:892 >> + if (!widthPrim.isPx() || !heightPrim.isPx()) > > What about other absolute units like pt, in, cm and font-size relative units like em, rem? Good point. The code was removed for now. >> Source/WebCore/html/LazyLoadImageObserver.cpp:85 >> + intersectionObserver->observe(element); > > This seems wrong. The top document may have a different origin than the document, so an observer on the root won't be able to observe images in cross-origin subframes. > > Does this code do the right thing when Elements are moved between documents? I'll have to look into it and test a bit. Do you think the situation was better before Said's topDocument suggestion or same? >>> Source/WebCore/html/LazyLoadImageObserver.cpp:108 >>> + m_lazyLoadIntersectionObserver = observer.returnValue().ptr(); >> >> The spec talks about initiating loads when an image enters the viewport, but I would think we'd want some additional coverage area so there's a change the image can start loading earlier. Ideally we'd kick off image loads at the same time we make tiles for a region. > > Do we want to do something similar to async image decoding but for loading? Or do we want to include loadDeferredImage() inside the async image decoding? Simon, you are referring to something other than threshold values (chromium uses different values for different devices IIRC), rather a synchronization hook? >> Source/WebCore/loader/ImageLoader.cpp:76 >> + if (!url.protocolIsInHTTPFamily()) > > Is this in the spec? No, I will try removing. >> Source/WebCore/loader/ImageLoader.cpp:81 >> + // heuristic helps avoid double fetching tracking pixels. > > Is the intention to avoid changing behavior for tracking pixels? This part was removed. >> Source/WebCore/loader/ImageLoader.cpp:87 >> + // small enough. This heuristic helps avoid double fetching tracking pixels. > > Why would double-fetching ever happen? You are right, this can only happen when combined with range requests. I had the range requests in my first patch but it adds complexity and for this patch I would like to leave it out. This part was also removed. >> Source/WebCore/loader/ImageLoader.cpp:215 >> + && RuntimeEnabledFeatures::sharedFeatures().lazyImageLoadingEnabled()) { > > You can unwrap these lines. Done. >> Source/WebCore/loader/ImageLoader.cpp:337 >> + // A placeholder was requested, but the result was an error or a full image. > > Who's requesting the placeholder? I will remove the comment, also only makes sense if there is a range request. >> Source/WebCore/loader/ImageLoader.h:85 >> + enum class LazyImageLoadState { None, Deferred, FullImage }; > > : uint8_t Done. >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:201 >> +ResourceErrorOr<CachedResourceHandle<CachedImage>> CachedResourceLoader::requestImage(CachedResourceRequest&& request, DeferOption defer) > > defer -> deferOption Will do.

Created attachment 378212 [details] Patch

(In reply to Rob Buis from comment #38) > >> Source/WebCore/html/LazyLoadImageObserver.cpp:85 > >> + intersectionObserver->observe(element); > > > > This seems wrong. The top document may have a different origin than the document, so an observer on the root won't be able to observe images in cross-origin subframes. > > > > Does this code do the right thing when Elements are moved between documents? > > I'll have to look into it and test a bit. Do you think the situation was > better before Said's topDocument suggestion or same? I didn't see that. I think it's best just to register the Intersection Observer(s) in the image's document. > >>> Source/WebCore/html/LazyLoadImageObserver.cpp:108 > >>> + m_lazyLoadIntersectionObserver = observer.returnValue().ptr(); > >> > >> The spec talks about initiating loads when an image enters the viewport, but I would think we'd want some additional coverage area so there's a change the image can start loading earlier. Ideally we'd kick off image loads at the same time we make tiles for a region. > > > > Do we want to do something similar to async image decoding but for loading? Or do we want to include loadDeferredImage() inside the async image decoding? async image decoding has logic about first tile paint, but that's not what we want here. I think it's nice to use Intersection Observer, but IO doesn't allow for heuristics like "look further ahead in the direction the user is scrolling". Maybe we can extend IO with some non-web-exposed behavior that could do that, and we could also use for things like animated GIF pausing. > Simon, you are referring to something other than threshold values (chromium > uses different values for different devices IIRC), rather a synchronization > hook? Thresholds, yes.

(In reply to Simon Fraser (smfr) from comment #40) > (In reply to Rob Buis from comment #38) > > Simon, you are referring to something other than threshold values (chromium > > uses different values for different devices IIRC), rather a synchronization > > hook? > > Thresholds, yes. Thanks. IntersectionObserver allows thresholds (in the Init structure), I think we can skip it like we do so far or estimate some sane default value (for all platforms) to keep this patch "small".

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review Thanks for the review, will try to address it soon. >> Source/WebCore/html/HTMLImageElement.cpp:861 >> +} > > Normally in modern DOM attributes this would be done with reflection and an enumeration. I don’t have a source tree handy at the moment to search, but there would be [Reflect] in the IDL file and enumeration values in there too. There may be some reason we can’t do it in this case, but I’d like to know why. I would like to see an example of this, happy to trade this in for some automated code solution. The only thing special about that code is it fullfills the demand of a content attribute "limited to only known values": https://github.com/whatwg/html/pull/3752/files

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review >>> Source/WebCore/html/HTMLImageElement.cpp:861 >>> +} >> >> Normally in modern DOM attributes this would be done with reflection and an enumeration. I don’t have a source tree handy at the moment to search, but there would be [Reflect] in the IDL file and enumeration values in there too. There may be some reason we can’t do it in this case, but I’d like to know why. > > I would like to see an example of this, happy to trade this in for some automated code solution. The only thing special about that code is it fullfills the demand of a content attribute "limited to only known values": > https://github.com/whatwg/html/pull/3752/files Darn, after searching I’m not sure we made enumerations work with reflection. There are plenty of examples of enumerations, like VideoPresentationMode in HTMLVideoElement and FetchResponseType in FetchResponse, and many others. And the code for parsing enumerations does the right things for us. But I guess they are not used with [Reflect] anywhere, or at least I didn’t find anywhere. IDL enumerations are almost a good fit for this type of thing, but I believe they parse case sensitive so that might be one thing we'd have to change, and I guess that IDL enumerations aren’t actually used for DOM attributes like this in the specifications. It sure would be nicer to have generated code for the "limited to only known values" rules rather than writing custom code in each case. But maybe WebKit doesn’t have the best machinery for this already "off the shelf", and it's something we'd have to make.

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review >>>> Source/WebCore/html/HTMLImageElement.cpp:861 >>>> +} >>> >>> Normally in modern DOM attributes this would be done with reflection and an enumeration. I don’t have a source tree handy at the moment to search, but there would be [Reflect] in the IDL file and enumeration values in there too. There may be some reason we can’t do it in this case, but I’d like to know why. >> >> I would like to see an example of this, happy to trade this in for some automated code solution. The only thing special about that code is it fullfills the demand of a content attribute "limited to only known values": >> https://github.com/whatwg/html/pull/3752/files > > Darn, after searching I’m not sure we made enumerations work with reflection. There are plenty of examples of enumerations, like VideoPresentationMode in HTMLVideoElement and FetchResponseType in FetchResponse, and many others. And the code for parsing enumerations does the right things for us. But I guess they are not used with [Reflect] anywhere, or at least I didn’t find anywhere. > > IDL enumerations are almost a good fit for this type of thing, but I believe they parse case sensitive so that might be one thing we'd have to change, and I guess that IDL enumerations aren’t actually used for DOM attributes like this in the specifications. It sure would be nicer to have generated code for the "limited to only known values" rules rather than writing custom code in each case. But maybe WebKit doesn’t have the best machinery for this already "off the shelf", and it's something we'd have to make. Thanks Darin. Making it a proper idl enum would prevent silly mistakes (e.g. if (img.loading == "eager ")). And it is not too late to change the spec. I'll check what Dominic (spec editor) thinks. FWIW chromium supports this through ReflectOnly. It seems nice to have rather than crucial at the moment to me. When we were working at the same company, Chris was interested in implementing these constructions, but I don't necessarily want to volunteer him :)

Created attachment 378295 [details] Patch

Created attachment 378323 [details] Patch

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review >> Source/WebCore/dom/Document.h:1536 >> + LazyLoadImageObserver& ensureLazyLoadImageObserver(); > > WebKit coding style says you don’t use the word "ensure" in the name of a function like this. Fixed. >> Source/WebCore/html/HTMLImageElement.cpp:247 >> + loadDeferredImage(); > > This typically isn’t the correct pattern for an attribute. It implements a "latching" behavior that remember "was the loading attribute ever set to the value 'eager' now or in the past". > > Or you could say it supports only the transition from loading not being "eager" (no attribute present, a different value) to loading being "eager". To correctly support an attribute typically we also have to support a transition in the opposite direction, removing the attribute or changing its value. We don’t want an image element to be permanently in "eager mode" just because for one moment in time the attribute had this value. This is typically done more correctly by writing something like this instead: > > else if (name == loadingAttr) > setLoadsDeferredImage(equalLettersIgnoringASCIICase(value, "eager")); > > This handles the case of the attribute being added, removed, or its value changed. It's important to write the setter code in a way that avoids doing work when the state isn’t changed. But perhaps there is some reason this value needs to be latched? I implemented setLoadsDeferredImage, but I don't think there is anything to do for eager -> lazy transition. >> Source/WebCore/html/HTMLImageElement.cpp:880 >> + // Do not lazyload image elements created from javascript. > > I don’t think we should coin a word "lazy load". I would write: > > // Never do lazy loading for image elements created from JavaScript. > > But also I am really confused by that rule! Done. Still need to find out more if the rule makes sense, will keep you posted. >> Source/WebCore/html/HTMLImageElement.cpp:885 >> + return true; > > I would find this easier to read as a boolean expression rather than a pair of if statements: > > return createdByParser() && equalLettersIgnoringASCIICase(attributeWithoutSynchronization(HTMLNames::loadingAttr), "lazy"); > > Even if broken over two lines I would find this easier to read. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:40 >> + static Ref<LazyImageLoadIntersectionObserverCallback> create(Document* document) > > Should take Document&, not Document*. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:44 >> + LazyImageLoadIntersectionObserverCallback(Document* document) > > Should be private, and should be explicit and should take Document&. Fixed. >> Source/WebCore/html/LazyLoadImageObserver.cpp:48 >> + CallbackResult<void> handleEvent(const Vector<Ref<IntersectionObserverEntry>>& entries, IntersectionObserver& intersectionObserver) > > Since this overrides a virtual function it should be marked final in WebKit coding style even though the class is final too. Also should be private. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:98 >> +IntersectionObserver* LazyLoadImageObserver::ensureIntersectionObserver(Document* rootDocument) > > I know Said suggested this, but I don’t understand why this has the word "ensure" in it. Normally we just would call this "intersectionObserver" in WebKit coding style. Also should take a Document&. Removed ensure. >> Source/WebCore/html/LazyLoadImageObserver.cpp:102 >> + auto options = IntersectionObserver::Init { nullptr, "", { } }; > > emptyString() is more efficient than "". > > But also, maybe there is a cleaner way to write this. Perhaps we can leave out the options entirely? Change the IntersectionObserver::create to overload for a case like this with no options? I use emptyString() for now. I think we need the options for thresholds/rootMargin. >> Source/WebCore/html/LazyLoadImageObserver.h:44 >> + IntersectionObserver* ensureIntersectionObserver(Document*); > > This argument should be Document& instead of Document*. Done. >> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:218 >> + } > > Again, why the one-time latching behavior. Can’t we change the lazy load attribute more than once? Should this forever remember the first value it had? The other attributes have the same behavior, could it be to prevent an attribut later on overriding an earlier (same name) attribute? >> Source/WebCore/loader/ImageLoader.h:84 >> + // LazyImages: Defer the image load until the image is near the viewport. > > Should not merge the words "lazy images" into a since LazyImages word unless that’s an established term of art. Removed.

Created attachment 378365 [details] Patch

Created attachment 378369 [details] Patch

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review >>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:218 >>> + } >> >> Again, why the one-time latching behavior. Can’t we change the lazy load attribute more than once? Should this forever remember the first value it had? > > The other attributes have the same behavior, could it be to prevent an attribut later on overriding an earlier (same name) attribute? I think it’s peculiar for the other attributes as well and I would like to see tests covering those dynamic cases. I remember a time when WebKit’s DOM had this mistake all over the place, assuming that attributes were only parsed and never dynamically changed or removed later. So it doesn’t surprise me to see this kind of error again. If intentional then I think it’s good to have test cases showing the behavior anyway.

Comment on attachment 378369 [details] Patch Attachment 378369 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13017055 New failing tests: js/dom/customAccessor-defineProperty.html

Created attachment 378382 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 378369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378369&action=review > Source/WebCore/html/HTMLImageElement.cpp:247 > + else if (name == loadingAttr) > + setLoadsDeferredImage(!equalLettersIgnoringASCIICase(value, "lazy")); To avoid creating the new function setLoadsDeferredImage(): else if (name == loadingAttr) { if (!equalLettersIgnoringASCIICase(value, "lazy")) loadDeferredImage(); } > Source/WebCore/html/HTMLImageElement.cpp:269 > + m_imageLoader.loadDeferredImage(); I think you need to check or assert isDeferred() is true. > Source/WebCore/html/HTMLImageElement.cpp:276 > +void HTMLImageElement::setLoadsDeferredImage(bool shouldForceLoad) > +{ > + if (shouldForceLoad && isDeferred()) > + loadDeferredImage(); > +} I think this function is not needed if you replace the single call to it by loadDeferredImage(). > Source/WebCore/html/HTMLImageElement.cpp:883 > +bool HTMLImageElement::isDeferred() const > +{ > + if (!m_isDeferred) > + return false; > + auto* image = cachedImage(); > + if (!image) > + return false; > + return image->stillNeedsLoad(); > +} What is the difference between calling this function and checking m_imageLoader.m_lazyImageLoadState == LazyImageLoadState::Deferred? ImageLoader::loadDeferredImage() bails out early if the second condition is false. I would expect isDeferred() to live in ImageLoader because the actual loading have to be carried out by ImageLoader and not the HTMLImageElement > Source/WebCore/html/LazyLoadImageObserver.cpp:64 > + LazyImageLoadIntersectionObserverCallback(Document& document) > + : IntersectionObserverCallback(&document) > + { > + } I think this constructor can be replaced by this line: using IntersectionObserverCallback::IntersectionObserverCallback; > Source/WebCore/loader/ImageLoader.cpp:207 > + m_lazyImageLoadState = LazyImageLoadState::Deferred; > + imageElement.setDeferred(); Can't we set the deferred state in one place instead of two, preferably the ImageLoader?

Comment on attachment 378095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378095&action=review >>> Source/WebCore/html/LazyLoadImageObserver.cpp:85 >>> + intersectionObserver->observe(element); >> >> This seems wrong. The top document may have a different origin than the document, so an observer on the root won't be able to observe images in cross-origin subframes. >> >> Does this code do the right thing when Elements are moved between documents? > > I'll have to look into it and test a bit. Do you think the situation was better before Said's topDocument suggestion or same? I now added a test for moving the element between documents to js-image.html. It depends on what we define as the right thing, but it should be according to spec, by clearing the "created by parser" flag when moving between documents and also sticking to the "no lazy loading for elements created from JS" rule.

Comment on attachment 378189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378189&action=review >>> Source/WebCore/html/HTMLImageElement.cpp:880 >>> + // Do not lazyload image elements created from javascript. >> >> I don’t think we should coin a word "lazy load". I would write: >> >> // Never do lazy loading for image elements created from JavaScript. >> >> But also I am really confused by that rule! > > Done. Still need to find out more if the rule makes sense, will keep you posted. I added a test for this to js-image.html that may be compelling: const img = new Image(); img.loading = "lazy"; img.src = '../loading/resources/base-image1.png'; img.decode().then(() => { assert_true(is_image_fully_loaded(img)); t.done(); }).catch((encodingError) => { assert_unreached(); }) If we would allow lazy loading for JS images, the decode() Promise would never resolve.

Created attachment 378397 [details] Patch

Comment on attachment 378369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378369&action=review >> Source/WebCore/html/HTMLImageElement.cpp:247 >> + setLoadsDeferredImage(!equalLettersIgnoringASCIICase(value, "lazy")); > > To avoid creating the new function setLoadsDeferredImage(): > > else if (name == loadingAttr) { > if (!equalLettersIgnoringASCIICase(value, "lazy")) > loadDeferredImage(); > } Done. >> Source/WebCore/html/HTMLImageElement.cpp:269 >> + m_imageLoader.loadDeferredImage(); > > I think you need to check or assert isDeferred() is true. I don't think it is needed since ImageLoader::loadDeferredImage does its own is deferred check. >> Source/WebCore/html/HTMLImageElement.cpp:276 >> +} > > I think this function is not needed if you replace the single call to it by loadDeferredImage(). Done. >> Source/WebCore/html/HTMLImageElement.cpp:883 >> +} > > What is the difference between calling this function and checking m_imageLoader.m_lazyImageLoadState == LazyImageLoadState::Deferred? ImageLoader::loadDeferredImage() bails out early if the second condition is false. > > I would expect isDeferred() to live in ImageLoader because the actual loading have to be carried out by ImageLoader and not the HTMLImageElement Good point, I kept this method but made it ask the ImageLoader. >> Source/WebCore/html/LazyLoadImageObserver.cpp:64 >> + } > > I think this constructor can be replaced by this line: > > using IntersectionObserverCallback::IntersectionObserverCallback; This did not work for me, possibly because IntersectionObserverCallback is abstract? >> Source/WebCore/loader/ImageLoader.cpp:207 >> + imageElement.setDeferred(); > > Can't we set the deferred state in one place instead of two, preferably the ImageLoader? Good point, see above, looks like we do not need two places.

Created attachment 378744 [details] Patch

Comment on attachment 378095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378095&action=review >>> Source/WebCore/loader/cache/CachedResourceLoader.h:65 >>> +enum class DeferOption { NoDefer, DeferredByClient }; >> >> Defer what? This should be LoadingDeferral or something. > > enum class ImageRequestOption : uint8_t { Now, Defer }; ? Done.

Comment on attachment 378744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378744&action=review > Source/WebCore/html/LazyLoadImageObserver.cpp:85 > + Document* document = rootDocument(element); > + if (!document) > + return; > + auto& observer = document->lazyLoadImageObserver(); > + auto* intersectionObserver = observer.intersectionObserver(*document); It still seems wrong to me to do cross-origin observing here, both from an implementation and a security perspective. What does the spec say about lazy loading in iframes adding a new way for cross-origin frames to observe scrolling in their ancestors?

Created attachment 378849 [details] Patch

(In reply to Simon Fraser (smfr) from comment #60) > Comment on attachment 378744 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378744&action=review > > > Source/WebCore/html/LazyLoadImageObserver.cpp:85 > > + Document* document = rootDocument(element); > > + if (!document) > > + return; > > + auto& observer = document->lazyLoadImageObserver(); > > + auto* intersectionObserver = observer.intersectionObserver(*document); > > It still seems wrong to me to do cross-origin observing here, both from an > implementation and a security perspective. > > What does the spec say about lazy loading in iframes adding a new way for > cross-origin frames to observe scrolling in their ancestors? It is in the "fingerprint" advice section in the spec: https://github.com/whatwg/html/pull/3752/files In my latest patch I disabled lazy image loading in cross-origin frames.

(In reply to Simon Fraser (smfr) from comment #60) > Comment on attachment 378744 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378744&action=review > > > Source/WebCore/html/LazyLoadImageObserver.cpp:85 > > + Document* document = rootDocument(element); > > + if (!document) > > + return; > > + auto& observer = document->lazyLoadImageObserver(); > > + auto* intersectionObserver = observer.intersectionObserver(*document); > > It still seems wrong to me to do cross-origin observing here, both from an > implementation and a security perspective. > > What does the spec say about lazy loading in iframes adding a new way for > cross-origin frames to observe scrolling in their ancestors? If it uses the normally-available IntersectionObserver then this implementation does not change security/privacy aspects that IntersectionObserver already allows.

Created attachment 379149 [details] Patch

I replaced the "disable lazy image loading in cross-origin frames" with "disable lazy image loading when scripting is off" behavior to match the specification more, and added tests for that behavior.

Comment on attachment 379149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379149&action=review > Source/WebCore/html/LazyLoadImageObserver.cpp:88 > + Document* document = rootDocument(element); > + if (!document) > + return; > + auto& observer = document->lazyLoadImageObserver(); > + auto* intersectionObserver = observer.intersectionObserver(*document); > + if (!intersectionObserver) > + return; > + intersectionObserver->observe(element); I still think you should have an IntersectionObserver per frame, not always use a single observer in the root document. Crossing document boundaries like this is a source of bugs.

Created attachment 380151 [details] Patch

(In reply to Simon Fraser (smfr) from comment #66) > Comment on attachment 379149 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379149&action=review > > > Source/WebCore/html/LazyLoadImageObserver.cpp:88 > > + Document* document = rootDocument(element); > > + if (!document) > > + return; > > + auto& observer = document->lazyLoadImageObserver(); > > + auto* intersectionObserver = observer.intersectionObserver(*document); > > + if (!intersectionObserver) > > + return; > > + intersectionObserver->observe(element); > > I still think you should have an IntersectionObserver per frame, not always > use a single observer in the root document. Crossing document boundaries > like this is a source of bugs. Fair enough, done.

Comment on attachment 380151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380151&action=review I think this needs a bit more consideration of the edge cases: removing an element before lazy loading completes. Moving an element to a new document before lazy loading completes (didMoveToNewDocument). Also, are you sure there aren't any new retain cycles that keep Documents alive with this patch? > Source/WebCore/html/LazyLoadImageObserver.cpp:56 > + intersectionObserver.trackingDocument()->lazyLoadImageObserver().unobserve(*element); In observer(), you get to the document via element.document(). Might as well do the same thing here. > Source/WebCore/html/LazyLoadImageObserver.cpp:84 > +IntersectionObserver* LazyLoadImageObserver::intersectionObserver(Document& rootDocument) This isn't rootDocument any more. > Source/WebCore/loader/ImageLoader.cpp:264 > + if (m_lazyImageLoadState == LazyImageLoadState::Deferred) > + LazyLoadImageObserver::observe(element()); If the image element is removed from the DOM, does this keep it alive? Is there anything that stops observation in that case?

Created attachment 381911 [details] Patch

Created attachment 381946 [details] Patch

Comment on attachment 380151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380151&action=review >> Source/WebCore/html/LazyLoadImageObserver.cpp:56 >> + intersectionObserver.trackingDocument()->lazyLoadImageObserver().unobserve(*element); > > In observer(), you get to the document via element.document(). Might as well do the same thing here. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:84 >> +IntersectionObserver* LazyLoadImageObserver::intersectionObserver(Document& rootDocument) > > This isn't rootDocument any more. Done. >> Source/WebCore/loader/ImageLoader.cpp:264 >> + LazyLoadImageObserver::observe(element()); > > If the image element is removed from the DOM, does this keep it alive? Is there anything that stops observation in that case? IntersectionObserver protects against this, when the image element is removed, it will not have a renderer, so it will not intersect and therefore no deferred load will be triggered.

(In reply to Simon Fraser (smfr) from comment #69) > Comment on attachment 380151 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380151&action=review > > I think this needs a bit more consideration of the edge cases: removing an > element before lazy loading completes. I added http/tests/lazyload/scroll-element-removed-from-document.html for this. > Moving an element to a new document before lazy loading completes (didMoveToNewDocument). I added http/tests/lazyload/scroll-element-moved-from-document.html for this. > Also, are you sure there aren't any new retain cycles that keep Documents alive with this patch? I can't see that happen. m_lazyLoadIntersectionObserver probably does not need to be a RefPtr but it should not be a problem. Is there some good way to test for this?

(In reply to Rob Buis from comment #73) > (In reply to Simon Fraser (smfr) from comment #69) > > Comment on attachment 380151 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=380151&action=review > > > > I think this needs a bit more consideration of the edge cases: removing an > > element before lazy loading completes. > > I added http/tests/lazyload/scroll-element-removed-from-document.html for > this. Good. > > > Moving an element to a new document before lazy loading completes (didMoveToNewDocument). > > I added http/tests/lazyload/scroll-element-moved-from-document.html for this. Nice. > > > Also, are you sure there aren't any new retain cycles that keep Documents alive with this patch? > > I can't see that happen. m_lazyLoadIntersectionObserver probably does not > need to be a RefPtr but it should not be a problem. Is there some good way > to test for this? run-webkit-tests --world-leaks

Comment on attachment 381946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381946&action=review > Source/WebCore/loader/cache/CachedResourceLoader.h:65 > +enum class ImageRequestOption : uint8_t { Now, Defer }; I think this could use a better name. Maybe ImageLoading { Immediate, Deferred } ? Or DeferredUntilVisible? > LayoutTests/ChangeLog:8 > + Import relevant tests into http/tests/lazyload. Do they have to be http tests? Should they be web platform tests?

Created attachment 382008 [details] Patch

Created attachment 382014 [details] Patch

Comment on attachment 382014 [details] Patch Clearing flags on attachment: 382014 Committed r251637: <https://trac.webkit.org/changeset/251637>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56646614>

(In reply to Simon Fraser (smfr) from comment #75) > Comment on attachment 381946 [details] > Patch > > > LayoutTests/ChangeLog:8 > > + Import relevant tests into http/tests/lazyload. > > Do they have to be http tests? Should they be web platform tests? You are right, I will try to push them to the wpt repo soon.

This change introduced a crashing test on Mac Debug wk2 http/tests/lazyload/scroll-element-removed-from-document.html History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Flazyload%2Fscroll-element-removed-from-document.html log: https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r251671%20(5494)/http/tests/lazyload/scroll-element-removed-from-document-crash-log.txt Can this be resolved quickly?

(In reply to Truitt Savell from comment #82) > This change > > introduced a crashing test on Mac Debug wk2 > http/tests/lazyload/scroll-element-removed-from-document.html > > Can this be resolved quickly? Yes, I am pretty sure I know what is wrong, I should be able to fix it today/Tuesday.

Reopening to attach new patch.

Created attachment 382167 [details] Patch

We rolled this out in https://trac.webkit.org/changeset/251708/webkit We decided to roll this out while you resolved the fix for your commit.

Created attachment 382342 [details] Patch

Created attachment 382345 [details] Patch

(In reply to Truitt Savell from comment #86) > We rolled this out in https://trac.webkit.org/changeset/251708/webkit > We decided to roll this out while you resolved the fix for your commit. Sorry, with travel preparations I was not able to dedicate enough time to fix it. Today I had some time and now it is fixed locally. The problem was that when moving elements to a new document the observation on the old document was never unobserved.

Comment on attachment 382345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382345&action=review > Source/WebCore/html/HTMLImageElement.cpp:679 > + if (m_imageLoader->isDeferred()) This is the same as calling isDeferred() > Source/WebCore/html/HTMLImageElement.cpp:683 > } Do we need to start observing in the new document? Or is that done elsewhere?

Created attachment 382495 [details] Patch

Created attachment 382500 [details] Patch

Created attachment 383144 [details] Patch

Comment on attachment 382345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382345&action=review >> Source/WebCore/html/HTMLImageElement.cpp:679 >> + if (m_imageLoader->isDeferred()) > > This is the same as calling isDeferred() Fixed. >> Source/WebCore/html/HTMLImageElement.cpp:683 >> } > > Do we need to start observing in the new document? Or is that done elsewhere? This is being done automatically when the element is inserted into newDocument. However I noticed the lazy load state was not being reset, so I fixed that in ImageLoader::elementDidMoveToNewDocument.

Comment on attachment 382345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382345&action=review >>> Source/WebCore/html/HTMLImageElement.cpp:683 >>> } >> >> Do we need to start observing in the new document? Or is that done elsewhere? > > This is being done automatically when the element is inserted into newDocument. However I noticed the lazy load state was not being reset, so I fixed that in ImageLoader::elementDidMoveToNewDocument. Could we add a test case that would have failed with that omission?

(In reply to Darin Adler from comment #98) > Comment on attachment 382345 [details] > Patch > > This is being done automatically when the element is inserted into newDocument. However I noticed the lazy load state was not being reset, so I fixed that in ImageLoader::elementDidMoveToNewDocument. > > Could we add a test case that would have failed with that omission? This is tested by http/tests/lazyload/scroll-element-moved-from-document.html.

Created attachment 384614 [details] Patch

Created attachment 385253 [details] Patch

Created attachment 385383 [details] Patch

Created attachment 385594 [details] Patch

Created attachment 386605 [details] Patch

Created attachment 386612 [details] Patch

It would be helpful if, when attaching a new patch, you said what was changed in it. I'm a bit lost now with the rollouts etc.

(In reply to Simon Fraser (smfr) from comment #106) > It would be helpful if, when attaching a new patch, you said what was > changed in it. I'm a bit lost now with the rollouts etc. Hi Simon, I usually only do that for patches that I put up for review. Lately it was just rebasing, and importing tests that we added to WPT late December. I also fixed a load event problem today because of the new tests. The spec change is supposed to land next week and I have to fix one more test failure, so I think next week is a good time to put up the patch for review again, with a detailed commit message. Sorry about the confusion!

Created attachment 390633 [details] Patch

Created attachment 390672 [details] Patch

Comment on attachment 390672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390672&action=review I know this wasn’t up for review, but have a few comments. > Source/WebCore/html/HTMLImageElement.cpp:267 > + if (!equalLettersIgnoringASCIICase(value, "lazy")) > + loadDeferredImage(); Since this isn’t usually the correct pattern, but we’ve determined in this case that it is, we should probably write a brief. clarifying comment explaining that. > Source/WebCore/html/HTMLImageElement.cpp:910 > + auto attributeValue = attributeWithoutSynchronization(HTMLNames::loadingAttr); > + if (equalLettersIgnoringASCIICase(attributeValue, "lazy")) > + return lazy; > + return eager; I think it’s better as a one-liner: return equalLettersIgnoringASCIICase(attributeWithoutSynchronization(HTMLNames::loadingAttr), "lazy") ? lazy : eager; > Source/WebCore/html/LazyLoadImageObserver.cpp:89 > + auto options = IntersectionObserver::Init { nullptr, emptyString(), { } }; > + auto observer = IntersectionObserver::create(document, WTFMove(callback), WTFMove(options)); Better with fewer lines? auto observer = IntersectionObserver::create(document, WTFMove(callback), { nullptr, emptyString(), { } }); Maybe not. > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:166 > + if (equalLettersIgnoringASCIICase(m_lazyloadAttribute, "lazy")) Annoying that this is repeated in 4 different places. Any way to share it more? > Source/WebCore/loader/ImageLoader.cpp:204 > + if (m_lazyImageLoadState == LazyImageLoadState::None) { > + if (is<HTMLImageElement>(element())) { Use && here instead of nested ifs? > Source/WebCore/loader/ImageLoader.cpp:212 > + newImage = document.cachedResourceLoader().requestImage(WTFMove(request), (m_lazyImageLoadState == LazyImageLoadState::Deferred) ? ImageLoading::DeferredUntilVisible : ImageLoading::Immediate).value_or(nullptr); I usually push for single lines but I’m thinking the ImageLoading policy enum expression should maybe go on a separate line here. > Source/WebCore/loader/ImageLoader.h:87 > + enum class LazyImageLoadState { None, Deferred, FullImage }; I suggest basing this on uint8_t instead of int. > Source/WebCore/loader/cache/CachedResourceLoader.h:65 > +enum class ImageLoading : uint8_t { Immediate, DeferredUntilVisible }; Not sure if bool or uint8_t is better in a case like this. > Source/WebCore/rendering/RenderImage.cpp:435 > + return is<HTMLImageElement>(element) && downcast<HTMLImageElement>(element)->isDeferred(); Would have done downcast<HTMLImageElement>(*element). but not sure the generated code will differ at all.

Created attachment 390742 [details] Patch

Created attachment 390765 [details] Patch

Created attachment 390791 [details] Patch

Comment on attachment 390672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390672&action=review Thanks for the review! >> Source/WebCore/html/HTMLImageElement.cpp:267 >> + loadDeferredImage(); > > Since this isn’t usually the correct pattern, but we’ve determined in this case that it is, we should probably write a brief. clarifying comment explaining that. I added a comment. >> Source/WebCore/html/HTMLImageElement.cpp:910 >> + return eager; > > I think it’s better as a one-liner: > > return equalLettersIgnoringASCIICase(attributeWithoutSynchronization(HTMLNames::loadingAttr), "lazy") ? lazy : eager; Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:89 >> + auto observer = IntersectionObserver::create(document, WTFMove(callback), WTFMove(options)); > > Better with fewer lines? > > auto observer = IntersectionObserver::create(document, WTFMove(callback), { nullptr, emptyString(), { } }); > > Maybe not. Heh :) Not sure, I kept it as-is for now. >> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:166 >> + if (equalLettersIgnoringASCIICase(m_lazyloadAttribute, "lazy")) > > Annoying that this is repeated in 4 different places. Any way to share it more? I added a static method on HTMLImageElement. The name may not be the best though. >> Source/WebCore/loader/ImageLoader.cpp:204 >> + if (is<HTMLImageElement>(element())) { > > Use && here instead of nested ifs? I think this was needed at some point, but you are right, now it makes more sense using &&. >> Source/WebCore/loader/ImageLoader.cpp:212 >> + newImage = document.cachedResourceLoader().requestImage(WTFMove(request), (m_lazyImageLoadState == LazyImageLoadState::Deferred) ? ImageLoading::DeferredUntilVisible : ImageLoading::Immediate).value_or(nullptr); > > I usually push for single lines but I’m thinking the ImageLoading policy enum expression should maybe go on a separate line here. Done. >> Source/WebCore/loader/ImageLoader.h:87 >> + enum class LazyImageLoadState { None, Deferred, FullImage }; > > I suggest basing this on uint8_t instead of int. Done. >> Source/WebCore/loader/cache/CachedResourceLoader.h:65 >> +enum class ImageLoading : uint8_t { Immediate, DeferredUntilVisible }; > > Not sure if bool or uint8_t is better in a case like this. Not sure either, but uint8_t makes it easy to add to the enum. I kept it as-is, hopefully it makes no difference. >> Source/WebCore/rendering/RenderImage.cpp:435 >> + return is<HTMLImageElement>(element) && downcast<HTMLImageElement>(element)->isDeferred(); > > Would have done downcast<HTMLImageElement>(*element). but not sure the generated code will differ at all. Ah I misread the suggestion initially. Yes that is a safe operation, indeed I think the generated code will be the same, kept as-is.

Comment on attachment 390791 [details] Patch I believe this can be reviewed again. Note that the spec has landed and it is supported in Chrome and Firefox.

Comment on attachment 390791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390791&action=review > Source/WebCore/dom/Document.h:1741 > + std::unique_ptr<LazyLoadImageObserver> m_lazyLoadImageObserver; Seems a tiny bit unfortunate that this leads to a memory block that just contains a single pointer in it. Maybe there’s some way to avoid that? > Source/WebCore/html/HTMLImageElement.cpp:707 > + if (isDeferred()) > + LazyLoadImageObserver::unobserve(*this, oldDocument); Seems like this might be wrong. If we move the same image element between documents twice, then we will call unobserve twice and the second time it will assert. Unless I am missing something that will do the observing or make isDeferred return false? > Source/WebCore/html/HTMLImageElement.cpp:910 > + auto attributeValue = attributeWithoutSynchronization(HTMLNames::loadingAttr); This should be auto& to avoid a tiny amount of reference count churn. > Source/WebCore/html/HTMLImageElement.h:-131 > - bool createdByParser() const { return m_createdByParser; } So we are removing this because it’s unused; seems like that was already true and this change could be done separately from the lazy loading work. I don’t see any removed uses of m_createdByParser or of createdByParser(). > Source/WebCore/html/LazyLoadImageObserver.cpp:53 > + Element* element = entry->target(); Tiny tiny code style point: I would have used auto or auto* here. If target() was known to be a more specific type than Element* we’d want to preserve that. > Source/WebCore/html/LazyLoadImageObserver.cpp:88 > + auto options = IntersectionObserver::Init { nullptr, emptyString(), { } }; Occurs to me that another way to write this is: IntersectionObserver::Init options { nullptr, emptyString(), { } }; Not sure I like it better. > Source/WebCore/html/LazyLoadImageObserver.h:38 > + LazyLoadImageObserver() = default; Maybe this should be private and Document should be a friend. Just to make sure nobody writes something like this by accident: LazyLoadImageObserver().unobserve(element, element.document()); It would be nice if they immediately got a compilation error. > Source/WebCore/html/LazyLoadImageObserver.h:48 > + // The intersection observer responsible for loading the image once it's near > + // the viewport. Gratuitious line break. Do it on one line!

Created attachment 390860 [details] Patch

Comment on attachment 390791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390791&action=review >> Source/WebCore/dom/Document.h:1741 >> + std::unique_ptr<LazyLoadImageObserver> m_lazyLoadImageObserver; > > Seems a tiny bit unfortunate that this leads to a memory block that just contains a single pointer in it. Maybe there’s some way to avoid that? I am not sure why this one would be singled out here, std::unique_ptr as member var is used in quite a few cases in Document, and Document does not seem to be used too frequently compared to other classes? If it is a problem than maybe a bigger patch needs to be done involving more members. The alternative seems to be plain old pointer which I would be ok with but obviously needs manual delettion. >> Source/WebCore/html/HTMLImageElement.cpp:707 >> + LazyLoadImageObserver::unobserve(*this, oldDocument); > > Seems like this might be wrong. If we move the same image element between documents twice, then we will call unobserve twice and the second time it will assert. Unless I am missing something that will do the observing or make isDeferred return false? I believe it is fine since after the first unobserve the elementDidMoveToNewDocument on the image loader will be called (a few lines below) and after that isDeferred will return false. >> Source/WebCore/html/HTMLImageElement.cpp:910 >> + auto attributeValue = attributeWithoutSynchronization(HTMLNames::loadingAttr); > > This should be auto& to avoid a tiny amount of reference count churn. Done. >> Source/WebCore/html/HTMLImageElement.h:-131 >> - bool createdByParser() const { return m_createdByParser; } > > So we are removing this because it’s unused; seems like that was already true and this change could be done separately from the lazy loading work. I don’t see any removed uses of m_createdByParser or of createdByParser(). Sorry, it was mainly used in HTMLImageElement::isLazyLoadable(), but the requirement for images created by JS to not be lazily loadable was dropped from the spec, so I was able to remove everything related to m_createdByParser. >> Source/WebCore/html/LazyLoadImageObserver.cpp:53 >> + Element* element = entry->target(); > > Tiny tiny code style point: I would have used auto or auto* here. If target() was known to be a more specific type than Element* we’d want to preserve that. Done. >> Source/WebCore/html/LazyLoadImageObserver.cpp:88 >> + auto options = IntersectionObserver::Init { nullptr, emptyString(), { } }; > > Occurs to me that another way to write this is: > > IntersectionObserver::Init options { nullptr, emptyString(), { } }; > > Not sure I like it better. I like it a bit better, done. >> Source/WebCore/html/LazyLoadImageObserver.h:38 >> + LazyLoadImageObserver() = default; > > Maybe this should be private and Document should be a friend. Just to make sure nobody writes something like this by accident: > > LazyLoadImageObserver().unobserve(element, element.document()); > > It would be nice if they immediately got a compilation error. Done. >> Source/WebCore/html/LazyLoadImageObserver.h:48 >> + // the viewport. > > Gratuitious line break. Do it on one line! Done.

Comment on attachment 390791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390791&action=review >>> Source/WebCore/html/HTMLImageElement.cpp:707 >>> + LazyLoadImageObserver::unobserve(*this, oldDocument); >> >> Seems like this might be wrong. If we move the same image element between documents twice, then we will call unobserve twice and the second time it will assert. Unless I am missing something that will do the observing or make isDeferred return false? > > I believe it is fine since after the first unobserve the elementDidMoveToNewDocument on the image loader will be called (a few lines below) and after that isDeferred will return false. Then why not do this work inside ImageLoader::elementDidMoveToNewDocument too?

Created attachment 390886 [details] Patch

Comment on attachment 390791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390791&action=review >>>> Source/WebCore/html/HTMLImageElement.cpp:707 >>>> + LazyLoadImageObserver::unobserve(*this, oldDocument); >>> >>> Seems like this might be wrong. If we move the same image element between documents twice, then we will call unobserve twice and the second time it will assert. Unless I am missing something that will do the observing or make isDeferred return false? >> >> I believe it is fine since after the first unobserve the elementDidMoveToNewDocument on the image loader will be called (a few lines below) and after that isDeferred will return false. > > Then why not do this work inside ImageLoader::elementDidMoveToNewDocument too? Done. It is a bit more logical, just a pity that we can't get rid of the isDefferer methods and elementDidMoveToNewDocument needs to gain a parameter.

Created attachment 390918 [details] Patch

Created attachment 390920 [details] Patch

Comment on attachment 390920 [details] Patch Rejecting attachment 390920 [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-01', 'apply-attachment', '--no-update', '--non-interactive', 390920, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: g file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied-expected.txt patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied-iframe-expected.txt patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied-iframe.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-crossorigin-applied.sub-expected.txt patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-crossorigin-applied.sub.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-referrer-policy-applied.sub-expected.txt patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-referrer-policy-applied.sub.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/picture-loading-lazy-expected.txt patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/picture-loading-lazy.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/remove-element-and-scroll-expected.txt patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/remove-element-and-scroll.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/resources/image-loading-lazy-below-viewport-iframe.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/resources/image-loading-lazy-in-viewport-iframe.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/resources/newwindow.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/resources/referrer-checker-img.py patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/resources/subframe.html patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/resources/w3c-import.log patching file LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/w3c-import.log patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/iframe-loading-eager.tentative-expected.txt rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/iframe-loading-eager.tentative-expected.txt' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/iframe-loading-eager.tentative.html rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/iframe-loading-eager.tentative.html' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/iframe-loading-lazy.tentative.html rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/iframe-loading-lazy.tentative.html' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/image-loading-eager.tentative-expected.txt rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/image-loading-eager.tentative-expected.txt' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/image-loading-eager.tentative.html rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/image-loading-eager.tentative.html' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/image-loading-lazy.tentative.html rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/image-loading-lazy.tentative.html' rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/resources/image.png' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/resources/subframe.html rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/resources/subframe.html' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/resources/w3c-import.log rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/resources/w3c-import.log' patching file LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/w3c-import.log rm 'LayoutTests/imported/w3c/web-platform-tests/loading/lazyload/w3c-import.log' patching file LayoutTests/platform/mac-wk1/TestExpectations Hunk #2 FAILED at 935. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej patching file LayoutTests/platform/win/TestExpectations Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/13324039

Created attachment 390963 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 390963 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 390963 [details] Patch Clearing flags on attachment: 390963 Committed r256786: <https://trac.webkit.org/changeset/256786>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 391041 [details] Patch

Created attachment 391043 [details] Patch

Comment on attachment 391043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391043&action=review > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied-expected.txt:4 > +FAIL Test that when deferred img is loaded, it uses the base URL computed at parse time. assert_unreached: The image load should not fail, trying to load with invalid base URL. Reached unreachable code Is this new failure expected? > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-referrer-policy-applied.sub-expected.txt:4 > +FAIL Test that when deferred img is loaded, it uses the referrer-policy specified at parse time. assert_unreached: The image load should not fail, by sending the wrong referer header. Reached unreachable code ditto?

Comment on attachment 391043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391043&action=review >> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied-expected.txt:4 >> +FAIL Test that when deferred img is loaded, it uses the base URL computed at parse time. assert_unreached: The image load should not fail, trying to load with invalid base URL. Reached unreachable code > > Is this new failure expected? Yes, I will put up a patch for this soon. >> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-referrer-policy-applied.sub-expected.txt:4 >> +FAIL Test that when deferred img is loaded, it uses the referrer-policy specified at parse time. assert_unreached: The image load should not fail, by sending the wrong referer header. Reached unreachable code > > ditto? Yes, unfortunately referrerPolicy on HTMLImageElement is not supported (yet) in WebKit, so this test can't pass right now (so not even lazy image loading related).

(In reply to Rob Buis from comment #133) > >> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-referrer-policy-applied.sub-expected.txt:4 > >> +FAIL Test that when deferred img is loaded, it uses the referrer-policy specified at parse time. assert_unreached: The image load should not fail, by sending the wrong referer header. Reached unreachable code > > > > ditto? > > Yes, unfortunately referrerPolicy on HTMLImageElement is not supported (yet) > in WebKit, so this test can't pass right now (so not even lazy image loading > related). Is there a bug on that?

(In reply to Simon Fraser (smfr) from comment #134) > > Yes, unfortunately referrerPolicy on HTMLImageElement is not supported (yet) > > in WebKit, so this test can't pass right now (so not even lazy image loading > > related). > > Is there a bug on that? I opened a new bug for that: https://bugs.webkit.org/show_bug.cgi?id=207901 Happy to work on that since I added referrerPolicy support for iframe and script last year.

Comment on attachment 391043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391043&action=review >>> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/original-base-url-applied-expected.txt:4 >>> +FAIL Test that when deferred img is loaded, it uses the base URL computed at parse time. assert_unreached: The image load should not fail, trying to load with invalid base URL. Reached unreachable code >> >> Is this new failure expected? > > Yes, I will put up a patch for this soon. Can you please add a comment in the changelog about the new failures and link to the bug?

Created attachment 391146 [details] Patch

Comment on attachment 391146 [details] Patch Clearing flags on attachment: 391146 Committed r256916: <https://trac.webkit.org/changeset/256916>

All reviewed patches have been landed. Closing bug.

The changes in https://trac.webkit.org/changeset/256916/webkit has caused imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/image-loading-lazy-in-cross-origin-ifame-001.sub.html to fail on iOS History: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fembedded-content%2Fthe-img-element%2Fimage-loading-lazy-in-cross-origin-ifame-001.sub.html Diff: --- /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/image-loading-lazy-in-cross-origin-ifame-001.sub-expected.txt +++ /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/image-loading-lazy-in-cross-origin-ifame-001.sub-actual.txt @@ -1,5 +1,7 @@ -PASS An image with loading='lazy' in cross origin iframe loads when it gets - visible by scrolling the iframe's scroll port +Harness Error (TIMEOUT), message = null +TIMEOUT An image with loading='lazy' in cross origin iframe loads when it gets + visible by scrolling the iframe's scroll port Test timed out +

(In reply to Truitt Savell from comment #140) > The changes in https://trac.webkit.org/changeset/256916/webkit > > has caused > imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img- > element/image-loading-lazy-in-cross-origin-ifame-001.sub.html Ah, this is an iframe lazy load test, that is not supported yet, it should be skipped , image-loading-lazy-in-cross-origin-ifame-002.sub.html as well. I'll make a patch for that tonight.

(In reply to Truitt Savell from comment #140) > The changes in https://trac.webkit.org/changeset/256916/webkit > > has caused > imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img- > element/image-loading-lazy-in-cross-origin-ifame-001.sub.html > > to fail on iOS Fix is going in here: https://bugs.webkit.org/show_bug.cgi?id=207946 Thanks for letting me know.

Rob, is there any remaining work to make lazy image loading shippable? Are you happy with the implementation?

(In reply to Simon Fraser (smfr) from comment #143) > Rob, is there any remaining work to make lazy image loading shippable? Are > you happy with the implementation? Pretty happy, it mostly needs some testing I feel, to check that it behaves as expected. It would be great if you could look at https://bugs.webkit.org/show_bug.cgi?id=203557. Bug https://bugs.webkit.org/show_bug.cgi?id=207902 is up for review. The two final things I think are crossOrigin and referrerPolicy (https://bugs.webkit.org/show_bug.cgi?id=207901) handling. I'll work on referrerPolicy for sure, but at the moment it is not completely sure how dynamic changes of crossOrigin and/or referrerPolicy should be handled, I am in touch with the lazy load spec authors about that and expect it will be cleared up in the next few weeks. Of course more things/test cases may come up in the spec discussions, anyway I think the subissues still need to be closed before this can be shipped.

Is lazy image loading enabled by default now? This bug is marked resolved, but its dependency to enable by default is still open, so I am confused. https://bugs.webkit.org/show_bug.cgi?id=208094

Still dependency bug 203557 left to fix.

(In reply to Maciej Stachowiak from comment #145) > Is lazy image loading enabled by default now? This bug is marked resolved, > but its dependency to enable by default is still open, so I am confused. > https://bugs.webkit.org/show_bug.cgi?id=208094 Yes, this was confusing, so I re-opened this bug. Main dependency still to fix is: https://bugs.webkit.org/show_bug.cgi?id=203557.

Since the issues are being tracked elsewhere, I'm setting this back to Resolved.