Bug 200764 - Main implementation for lazy image loading
Summary: Main implementation for lazy image loading
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on: 203566
Blocks: 196698
  Show dependency treegraph
 
Reported: 2019-08-15 02:38 PDT by Rob Buis
Modified: 2019-12-11 06:14 PST (History)
21 users (show)

See Also:


Attachments
Patch (62.84 KB, patch)
2019-08-15 02:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (62.09 KB, patch)
2019-08-15 08:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (62.09 KB, patch)
2019-08-15 09:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (62.09 KB, patch)
2019-08-15 11:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (62.00 KB, patch)
2019-08-21 06:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (62.21 KB, patch)
2019-08-23 01:13 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (62.98 KB, patch)
2019-08-29 08:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (63.47 KB, patch)
2019-09-04 06:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (60.32 KB, patch)
2019-09-04 08:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (59.82 KB, patch)
2019-09-04 09:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (61.29 KB, patch)
2019-09-05 06:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (65.62 KB, patch)
2019-09-05 08:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (2.67 MB, application/zip)
2019-09-05 09:52 PDT, Build Bot
no flags Details
Patch (65.62 KB, patch)
2019-09-05 10:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (14.24 MB, application/zip)
2019-09-05 12:23 PDT, Build Bot
no flags Details
Patch (63.09 KB, patch)
2019-09-06 00:20 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (63.36 KB, patch)
2019-09-06 02:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (57.05 KB, patch)
2019-09-06 02:24 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (56.39 KB, patch)
2019-09-06 06:13 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (54.78 KB, patch)
2019-09-06 06:36 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.93 MB, application/zip)
2019-09-06 07:55 PDT, Build Bot
no flags Details
Patch (53.82 KB, patch)
2019-09-06 08:04 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.80 MB, application/zip)
2019-09-06 09:35 PDT, Build Bot
no flags Details
Patch (54.60 KB, patch)
2019-09-06 11:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (54.62 KB, patch)
2019-09-07 10:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (55.18 KB, patch)
2019-09-08 05:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (56.48 KB, patch)
2019-09-09 05:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (57.52 KB, patch)
2019-09-09 08:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.78 MB, application/zip)
2019-09-09 10:53 PDT, Build Bot
no flags Details
Patch (56.03 KB, patch)
2019-09-09 12:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (60.08 KB, patch)
2019-09-13 12:19 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (60.74 KB, patch)
2019-09-16 02:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (67.46 KB, patch)
2019-09-19 11:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (67.18 KB, patch)
2019-10-03 12:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (67.42 KB, patch)
2019-10-25 05:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (72.23 KB, patch)
2019-10-25 11:17 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (72.17 KB, patch)
2019-10-26 02:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (71.99 KB, patch)
2019-10-26 08:03 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2019-10-29 01:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (76.72 KB, patch)
2019-10-30 12:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (75.70 KB, patch)
2019-10-30 13:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (75.96 KB, patch)
2019-10-31 13:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (75.50 KB, patch)
2019-10-31 14:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (74.07 KB, patch)
2019-11-08 11:09 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (78.44 KB, patch)
2019-12-02 06:29 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (79.36 KB, patch)
2019-12-10 05:45 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (84.39 KB, patch)
2019-12-11 06:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2019-08-15 02:38:08 PDT
Provide main implementation for lazy image loading.
Comment 1 Rob Buis 2019-08-15 02:45:39 PDT
Created attachment 376371 [details]
Patch
Comment 2 Rob Buis 2019-08-15 08:21:06 PDT
Created attachment 376381 [details]
Patch
Comment 3 Rob Buis 2019-08-15 09:22:56 PDT
Created attachment 376383 [details]
Patch
Comment 4 Rob Buis 2019-08-15 11:45:11 PDT
Created attachment 376400 [details]
Patch
Comment 5 Rob Buis 2019-08-15 13:32:22 PDT
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.
Comment 6 Rob Buis 2019-08-21 06:40:26 PDT
Created attachment 376872 [details]
Patch
Comment 7 Darin Adler 2019-08-22 17:30:19 PDT
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?
Comment 8 Rob Buis 2019-08-23 01:13:45 PDT
Created attachment 377113 [details]
Patch
Comment 9 Rob Buis 2019-08-23 14:25:48 PDT
(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 10 Frédéric Wang (:fredw) 2019-08-29 04:18:13 PDT
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 )
Comment 11 Rob Buis 2019-08-29 08:40:20 PDT
Created attachment 377592 [details]
Patch
Comment 12 Rob Buis 2019-08-29 12:27:05 PDT
(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!
Comment 13 Rob Buis 2019-09-04 06:25:03 PDT
Created attachment 377976 [details]
Patch
Comment 14 Rob Buis 2019-09-04 08:25:21 PDT
Created attachment 377983 [details]
Patch
Comment 15 Rob Buis 2019-09-04 09:28:54 PDT
Created attachment 377984 [details]
Patch
Comment 16 Said Abou-Hallawa 2019-09-04 14:17:58 PDT
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.
Comment 17 Rob Buis 2019-09-05 06:57:39 PDT
Created attachment 378079 [details]
Patch
Comment 18 Rob Buis 2019-09-05 08:38:32 PDT
Created attachment 378085 [details]
Patch
Comment 19 Build Bot 2019-09-05 09:52:35 PDT
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.
Comment 20 Build Bot 2019-09-05 09:52:37 PDT
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
Comment 21 Rob Buis 2019-09-05 10:50:27 PDT
Created attachment 378095 [details]
Patch
Comment 22 Build Bot 2019-09-05 12:23:47 PDT
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
Comment 23 Build Bot 2019-09-05 12:23:53 PDT
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 24 Simon Fraser (smfr) 2019-09-05 12:50:44 PDT
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 25 Said Abou-Hallawa 2019-09-05 15:23:35 PDT
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 }; ?
Comment 26 Rob Buis 2019-09-06 00:20:30 PDT
Created attachment 378164 [details]
Patch
Comment 27 Rob Buis 2019-09-06 02:01:15 PDT
Created attachment 378172 [details]
Patch
Comment 28 Rob Buis 2019-09-06 02:24:13 PDT
Created attachment 378175 [details]
Patch
Comment 29 Rob Buis 2019-09-06 06:13:54 PDT
Created attachment 378186 [details]
Patch
Comment 30 Rob Buis 2019-09-06 06:36:18 PDT
Created attachment 378187 [details]
Patch
Comment 31 Build Bot 2019-09-06 07:55:34 PDT
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
Comment 32 Build Bot 2019-09-06 07:55:42 PDT
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
Comment 33 Rob Buis 2019-09-06 08:04:52 PDT
Created attachment 378189 [details]
Patch
Comment 34 Build Bot 2019-09-06 09:35:14 PDT
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
Comment 35 Build Bot 2019-09-06 09:35:24 PDT
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 36 Darin Adler 2019-09-06 09:55:55 PDT
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 37 Rob Buis 2019-09-06 10:05:03 PDT
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 38 Rob Buis 2019-09-06 10:24:14 PDT
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.
Comment 39 Rob Buis 2019-09-06 11:27:53 PDT
Created attachment 378212 [details]
Patch
Comment 40 Simon Fraser (smfr) 2019-09-06 11:48:12 PDT
(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.
Comment 41 Rob Buis 2019-09-06 12:35:51 PDT
(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 42 Rob Buis 2019-09-06 12:50:40 PDT
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 43 Darin Adler 2019-09-06 18:54:13 PDT
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 44 Rob Buis 2019-09-06 22:14:49 PDT
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 :)
Comment 45 Rob Buis 2019-09-07 10:41:02 PDT
Created attachment 378295 [details]
Patch
Comment 46 Rob Buis 2019-09-08 05:49:58 PDT
Created attachment 378323 [details]
Patch
Comment 47 Rob Buis 2019-09-08 09:51:14 PDT
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.
Comment 48 Rob Buis 2019-09-09 05:41:56 PDT
Created attachment 378365 [details]
Patch
Comment 49 Rob Buis 2019-09-09 08:48:19 PDT
Created attachment 378369 [details]
Patch
Comment 50 Darin Adler 2019-09-09 09:56:35 PDT
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 51 Build Bot 2019-09-09 10:53:12 PDT
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
Comment 52 Build Bot 2019-09-09 10:53:17 PDT
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 53 Said Abou-Hallawa 2019-09-09 11:08:45 PDT
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 54 Rob Buis 2019-09-09 12:39:14 PDT
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 55 Rob Buis 2019-09-09 12:42:41 PDT
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.
Comment 56 Rob Buis 2019-09-09 12:45:31 PDT
Created attachment 378397 [details]
Patch
Comment 57 Rob Buis 2019-09-09 12:52:31 PDT
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.
Comment 58 Rob Buis 2019-09-13 12:19:36 PDT
Created attachment 378744 [details]
Patch
Comment 59 Rob Buis 2019-09-13 13:44:59 PDT
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 60 Simon Fraser (smfr) 2019-09-13 14:20:28 PDT
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?
Comment 61 Rob Buis 2019-09-16 02:07:42 PDT
Created attachment 378849 [details]
Patch
Comment 62 Rob Buis 2019-09-16 03:06:52 PDT
(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.
Comment 63 Dima Voytenko 2019-09-18 08:42:30 PDT
(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.
Comment 64 Rob Buis 2019-09-19 11:39:33 PDT
Created attachment 379149 [details]
Patch
Comment 65 Rob Buis 2019-09-19 14:09:14 PDT
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 66 Simon Fraser (smfr) 2019-10-02 12:43:58 PDT
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.
Comment 67 Rob Buis 2019-10-03 12:09:49 PDT
Created attachment 380151 [details]
Patch
Comment 68 Rob Buis 2019-10-03 14:28:13 PDT
(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 69 Simon Fraser (smfr) 2019-10-21 15:25:43 PDT
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?
Comment 70 Rob Buis 2019-10-25 05:50:47 PDT
Created attachment 381911 [details]
Patch
Comment 71 Rob Buis 2019-10-25 11:17:26 PDT
Created attachment 381946 [details]
Patch
Comment 72 Rob Buis 2019-10-25 13:26:51 PDT
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.
Comment 73 Rob Buis 2019-10-25 13:30:50 PDT
(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?
Comment 74 Simon Fraser (smfr) 2019-10-25 14:44:45 PDT
(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 75 Simon Fraser (smfr) 2019-10-25 14:47:56 PDT
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?
Comment 76 Rob Buis 2019-10-26 02:25:16 PDT
Created attachment 382008 [details]
Patch
Comment 77 Rob Buis 2019-10-26 08:03:35 PDT
Created attachment 382014 [details]
Patch
Comment 78 WebKit Commit Bot 2019-10-26 10:18:02 PDT
Comment on attachment 382014 [details]
Patch

Clearing flags on attachment: 382014

Committed r251637: <https://trac.webkit.org/changeset/251637>
Comment 79 WebKit Commit Bot 2019-10-26 10:18:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 80 Radar WebKit Bug Importer 2019-10-26 10:19:27 PDT
<rdar://problem/56646614>
Comment 81 Rob Buis 2019-10-26 10:29:06 PDT
(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.
Comment 83 Rob Buis 2019-10-28 19:44:14 PDT
(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.
Comment 84 Rob Buis 2019-10-29 01:45:25 PDT
Reopening to attach new patch.
Comment 85 Rob Buis 2019-10-29 01:45:27 PDT
Created attachment 382167 [details]
Patch
Comment 86 Truitt Savell 2019-10-29 10:06:36 PDT
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.
Comment 87 Rob Buis 2019-10-30 12:50:43 PDT
Created attachment 382342 [details]
Patch
Comment 88 Rob Buis 2019-10-30 13:22:14 PDT
Created attachment 382345 [details]
Patch
Comment 89 Darin Adler 2019-10-30 15:15:16 PDT Comment hidden (obsolete)
Comment 90 Darin Adler 2019-10-30 15:17:30 PDT Comment hidden (obsolete)
Comment 91 Darin Adler 2019-10-30 15:24:43 PDT Comment hidden (obsolete)
Comment 92 Rob Buis 2019-10-30 16:03:25 PDT
(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 93 Simon Fraser (smfr) 2019-10-30 16:05:58 PDT
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?
Comment 94 Rob Buis 2019-10-31 13:38:36 PDT
Created attachment 382495 [details]
Patch
Comment 95 Rob Buis 2019-10-31 14:12:39 PDT
Created attachment 382500 [details]
Patch
Comment 96 Rob Buis 2019-11-08 11:09:46 PST
Created attachment 383144 [details]
Patch
Comment 97 Rob Buis 2019-11-08 13:10:43 PST
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 98 Darin Adler 2019-11-11 16:58:21 PST
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?
Comment 99 Rob Buis 2019-11-12 13:54:01 PST
(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.
Comment 100 Rob Buis 2019-12-02 06:29:53 PST
Created attachment 384614 [details]
Patch
Comment 101 Rob Buis 2019-12-10 05:45:42 PST
Created attachment 385253 [details]
Patch
Comment 102 Rob Buis 2019-12-11 06:14:13 PST
Created attachment 385383 [details]
Patch