[webkit-reviews] review granted: [Bug 200764] Main implementation for lazy image loading : [Attachment 390791] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 200764: Main implementation for lazy image loading
https://bugs.webkit.org/show_bug.cgi?id=200764

Attachment 390791: Patch

https://bugs.webkit.org/attachment.cgi?id=390791&action=review




--- Comment #116 from Darin Adler <darin at apple.com> ---
Comment on attachment 390791
  --> https://bugs.webkit.org/attachment.cgi?id=390791
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!