Bug 215442 – Implement lazy iframe loading

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rob Buis]

Implement lazy iframe loading

[Rob Buis]

Created attachment 406499 [details]
Patch

[Rob Buis]

Created attachment 406500 [details]
Patch

[Rob Buis]

Created attachment 406504 [details]
Patch

[Rob Buis]

Created attachment 406506 [details]
Patch

[Rob Buis]

Created attachment 406510 [details]
Patch

[EWS Watchlist]

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

[Rob Buis]

Created attachment 406582 [details]
Patch

[Rob Buis]

Created attachment 406587 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/67458008>

[Rob Buis]

Created attachment 407283 [details]
Patch

[Rob Buis]

Created attachment 407286 [details]
Patch

[Rob Buis]

Created attachment 407291 [details]
Patch

[Rob Buis]

Created attachment 407667 [details]
Patch

[Rob Buis]

Created attachment 407670 [details]
Patch

[Rob Buis]

Created attachment 407689 [details]
Patch

[Rob Buis]

Created attachment 408320 [details]
Patch

[Rob Buis]

Created attachment 408321 [details]
Patch

[Darin Adler]

Comment on attachment 408321 [details]
Patch

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

> Source/WebCore/html/HTMLFrameElementBase.cpp:62
> -    // FIXME: Why is it valuable to return true when m_URL is empty?
> +    // FIXME: Why is it valuable to return true when m_url is empty?

Aww, I was fond of m_URL even though it violates our naming rules. Maybe it would be better to rename to m_frameURL or some other "word plus URL" name. Also, including that in this patch distracts me from the real changes.

> Source/WebCore/html/HTMLFrameElementBase.h:59
> +    AtomString m_url;

We’d really like to avoid protected data members. Can we instead add a protected getter, setter, or both?

> Source/WebCore/html/HTMLFrameOwnerElement.h:65
> +    virtual bool lazyLoadFrame() { return false; }

We try to avoid names for boolean predicates that also sound like commands: "lazily load this frame now". That’s where names like "shouldLoadFrameLazily" come from, and that’s what I suggest naming this here. Unless the name comes straight from a specification or something.

> Source/WebCore/html/HTMLFrameOwnerElement.h:66
> +    virtual bool lazyLoadObserverActive() const { return false; }

I guess this name is OK, but I might stick an "is" in front of the name to make the predicate nature more obvious.

> Source/WebCore/html/HTMLFrameOwnerElement.h:81
> +protected:
> +    bool m_lazyLoadObserverActive { false };

Can we add a protected setter instead of a making the data member protected? Also same name considerations.

> Source/WebCore/html/HTMLIFrameElement.cpp:113
> +        if (m_lazyLoadFrameObserver && !equalLettersIgnoringASCIICase(value, "lazy")) {
> +            m_lazyLoadFrameObserver->unobserve();
> +            loadDeferredFrame();
> +        }

I think it’s worth a comment explaining (in just a few words, I hope) that you can make something non-lazily load by changing the attribute, but you can never make it lazily load (because it’s too late, I guess).

Otherwise, this has the look for an incorrect parseAttribute implementation, which implements one direction and not the other.

> Source/WebCore/html/HTMLIFrameElement.cpp:158
> +    auto& attributeValue = attributeWithoutSynchronization(HTMLNames::loadingAttr);

Don’t need a local variable for this.

> Source/WebCore/html/HTMLIFrameElement.cpp:167
> +static bool isFrameLazyLoadable(const URL& url, const AtomString& attributeValue)

Dodge the lowercase "url" by naming this frameURL or completeURL or fullURL or whatever?

I am a big advocate of short names, but attributeValue seems too ambiguous to me. Maybe loadingAttributeValue would be clearer.

> Source/WebCore/html/HTMLIFrameElement.cpp:172
> +    if (!url.protocolIsInHTTPFamily())
> +        return false;
> +
> +    return equalLettersIgnoringASCIICase(attributeValue, "lazy");

Why not use &&?

> Source/WebCore/html/HTMLIFrameElement.cpp:178
> +        auto policy = referrerPolicy();

No need for this local variable.

> Source/WebCore/html/HTMLIFrameElement.cpp:193
> +    SetForScope<AtomString> urlScope(m_url);
> +    m_url = m_lazyLoadFrameObserver->url();

Wow this is strange. I don’t get it. Why are we temporarily changing the URL and then changing it back? What is m_url, anyway?

Also, SetForScope takes an argument for the new value, so this can be a single line.

> Source/WebCore/html/LazyLoadFrameObserver.cpp:38
> +class LazyFrameLoadIntersectionObserverCallback final : public IntersectionObserverCallback {

Seems annoying that these callbacks are a class and not functions (lambdas).

> Source/WebCore/html/LazyLoadFrameObserver.cpp:95
> +        IntersectionObserver::Init options { WTF::nullopt, emptyString(), { } };

Are these not the defaults? Why do we need to specify nullopt and empty string?

> Source/WebCore/html/LazyLoadFrameObserver.cpp:99
> +        m_lazyLoadIntersectionObserver = observer.returnValue().ptr();

I think we can save reference count churn if we use observer.releaseReturnValue() here instead of observer.returnValue().ptr(). If we can’t it’s likely a Ref/RefPtr implementation bug.

> Source/WebCore/html/LazyLoadFrameObserver.h:39
> +    void observe(const AtomString& url, const ReferrerPolicy&);

Dodge the unpleasant url name by giving it a more specific name?

> Source/WebCore/html/LazyLoadFrameObserver.h:42
> +    AtomString url() const { return m_url; }

Same question.

> Source/WebCore/html/LazyLoadFrameObserver.h:46
> +    friend class HTMLIFrameElement;

Do we really have to do this? Do any other files even include this header? Why not make things public instead?

> Source/WebCore/html/LazyLoadFrameObserver.h:51
> +    static std::unique_ptr<LazyLoadFrameObserver> create(HTMLIFrameElement& element)
> +    {
> +        return std::unique_ptr<LazyLoadFrameObserver>(new LazyLoadFrameObserver(element));
> +    }

We don’t need this. It’s better to make the constructor public and have the caller use makeUnique.

We only need create functions with private constructors for these cases:

1) Classes that are RefCounted, where we don’t want callers to be able to make the mistake of not putting the object into a Ref<> or RefPt<>.
2) Classes where we need to do something alongside constructing, like some work afterward, or some argument validation before, and might need to fail.

Otherwise, it’s better to not bake in policy about whether they are heap allocated or not, and let the caller decide.

> Source/WebCore/html/LazyLoadFrameObserver.h:57
> +    HTMLIFrameElement& m_element;

Would be nice to clump the data members together.

> Source/WebCore/html/LazyLoadFrameObserver.h:59
> +    AtomString m_url;

Same URL question.

> Source/WebCore/html/LazyLoadFrameObserver.h:63
> +    // The intersection observer responsible for loading the image once it's near the viewport.

Seems like we should strive for a better name so this isn’t needed.

> Source/WebCore/loader/FrameLoader.cpp:831
> -        if (!child->loader().m_isComplete)
> +        if (!child->loader().m_isComplete && (!child->ownerElement() || !child->ownerElement()->lazyLoadObserverActive()))
>              return false;

This seems messy. Is there a way to make this part of a named concept in its own FrameLoader function. Like preventsParentFromBeingComplete? Just feels like we are hacking it in here.

[Rob Buis]

Comment on attachment 408321 [details]
Patch

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

>> Source/WebCore/html/HTMLFrameElementBase.cpp:62
>> +    // FIXME: Why is it valuable to return true when m_url is empty?
> 
> Aww, I was fond of m_URL even though it violates our naming rules. Maybe it would be better to rename to m_frameURL or some other "word plus URL" name. Also, including that in this patch distracts me from the real changes.

Sounds good, I made https://bugs.webkit.org/show_bug.cgi?id=216401 for this.

[Rob Buis]

Created attachment 408536 [details]
Patch

[Rob Buis]

Created attachment 408550 [details]
Patch

[Rob Buis]

Created attachment 408586 [details]
Patch

[Rob Buis]

Comment on attachment 408321 [details]
Patch

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

>>> Source/WebCore/html/HTMLFrameElementBase.cpp:62
>>> +    // FIXME: Why is it valuable to return true when m_url is empty?
>> 
>> Aww, I was fond of m_URL even though it violates our naming rules. Maybe it would be better to rename to m_frameURL or some other "word plus URL" name. Also, including that in this patch distracts me from the real changes.
> 
> Sounds good, I made https://bugs.webkit.org/show_bug.cgi?id=216401 for this.

The name change now landed, sorry for making this patch harder to review! I'll try to put name changes in separate patches from now on.

>> Source/WebCore/html/HTMLFrameElementBase.h:59
>> +    AtomString m_url;
> 
> We’d really like to avoid protected data members. Can we instead add a protected getter, setter, or both?

Done.

>> Source/WebCore/html/HTMLFrameOwnerElement.h:65
>> +    virtual bool lazyLoadFrame() { return false; }
> 
> We try to avoid names for boolean predicates that also sound like commands: "lazily load this frame now". That’s where names like "shouldLoadFrameLazily" come from, and that’s what I suggest naming this here. Unless the name comes straight from a specification or something.

Done.

>> Source/WebCore/html/HTMLFrameOwnerElement.h:66
>> +    virtual bool lazyLoadObserverActive() const { return false; }
> 
> I guess this name is OK, but I might stick an "is" in front of the name to make the predicate nature more obvious.

Done.

>> Source/WebCore/html/HTMLFrameOwnerElement.h:81
>> +    bool m_lazyLoadObserverActive { false };
> 
> Can we add a protected setter instead of a making the data member protected? Also same name considerations.

It turned out this could be removed.

>> Source/WebCore/html/HTMLIFrameElement.cpp:113
>> +        }
> 
> I think it’s worth a comment explaining (in just a few words, I hope) that you can make something non-lazily load by changing the attribute, but you can never make it lazily load (because it’s too late, I guess).
> 
> Otherwise, this has the look for an incorrect parseAttribute implementation, which implements one direction and not the other.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:158
>> +    auto& attributeValue = attributeWithoutSynchronization(HTMLNames::loadingAttr);
> 
> Don’t need a local variable for this.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:167
>> +static bool isFrameLazyLoadable(const URL& url, const AtomString& attributeValue)
> 
> Dodge the lowercase "url" by naming this frameURL or completeURL or fullURL or whatever?
> 
> I am a big advocate of short names, but attributeValue seems too ambiguous to me. Maybe loadingAttributeValue would be clearer.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:172
>> +    return equalLettersIgnoringASCIICase(attributeValue, "lazy");
> 
> Why not use &&?

I think in this case we can have a local variable for document().completeURL(m_url) since we use it twice.

>> Source/WebCore/html/HTMLIFrameElement.cpp:178
>> +        auto policy = referrerPolicy();
> 
> No need for this local variable.

Unfortunately it is, referrerPolicy() depends on m_lazyLoadFrameObserver, so putting it in observe call does not seem to have guaranteed evaluation order (I seem to recall this is better defined in C++17).

>> Source/WebCore/html/HTMLIFrameElement.cpp:193
>> +    m_url = m_lazyLoadFrameObserver->url();
> 
> Wow this is strange. I don’t get it. Why are we temporarily changing the URL and then changing it back? What is m_url, anyway?
> 
> Also, SetForScope takes an argument for the new value, so this can be a single line.

This was rewritten to use the new setter/getter. When loading a deferred frame, the url (and referrer policy) used at the time of lazy load begin should be reused, any later changes should be ignored, hence this code.

>> Source/WebCore/html/LazyLoadFrameObserver.cpp:95
>> +        IntersectionObserver::Init options { WTF::nullopt, emptyString(), { } };
> 
> Are these not the defaults? Why do we need to specify nullopt and empty string?

It is a simple struct, so we have to specify all members.

>> Source/WebCore/html/LazyLoadFrameObserver.cpp:99
>> +        m_lazyLoadIntersectionObserver = observer.returnValue().ptr();
> 
> I think we can save reference count churn if we use observer.releaseReturnValue() here instead of observer.returnValue().ptr(). If we can’t it’s likely a Ref/RefPtr implementation bug.

Done.

>> Source/WebCore/html/LazyLoadFrameObserver.h:39
>> +    void observe(const AtomString& url, const ReferrerPolicy&);
> 
> Dodge the unpleasant url name by giving it a more specific name?

Done.

>> Source/WebCore/html/LazyLoadFrameObserver.h:42
>> +    AtomString url() const { return m_url; }
> 
> Same question.

Done.

>> Source/WebCore/html/LazyLoadFrameObserver.h:46
>> +    friend class HTMLIFrameElement;
> 
> Do we really have to do this? Do any other files even include this header? Why not make things public instead?

Not needed indeed since the constructor became public.

>> Source/WebCore/html/LazyLoadFrameObserver.h:51
>> +    }
> 
> We don’t need this. It’s better to make the constructor public and have the caller use makeUnique.
> 
> We only need create functions with private constructors for these cases:
> 
> 1) Classes that are RefCounted, where we don’t want callers to be able to make the mistake of not putting the object into a Ref<> or RefPt<>.
> 2) Classes where we need to do something alongside constructing, like some work afterward, or some argument validation before, and might need to fail.
> 
> Otherwise, it’s better to not bake in policy about whether they are heap allocated or not, and let the caller decide.

Nice! That cleans things up a bit.

>> Source/WebCore/html/LazyLoadFrameObserver.h:57
>> +    HTMLIFrameElement& m_element;
> 
> Would be nice to clump the data members together.

Done.

>> Source/WebCore/html/LazyLoadFrameObserver.h:59
>> +    AtomString m_url;
> 
> Same URL question.

Done.

>> Source/WebCore/html/LazyLoadFrameObserver.h:63
>> +    // The intersection observer responsible for loading the image once it's near the viewport.
> 
> Seems like we should strive for a better name so this isn’t needed.

The comment incorrectly referred to image, sorry. I removed it and improved the name.

>> Source/WebCore/loader/FrameLoader.cpp:831
>>              return false;
> 
> This seems messy. Is there a way to make this part of a named concept in its own FrameLoader function. Like preventsParentFromBeingComplete? Just feels like we are hacking it in here.

Done.

[EWS]

Committed r266976: <https://trac.webkit.org/changeset/266976>

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

[Darin Adler]

Comment on attachment 408321 [details]
Patch

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

>>> Source/WebCore/html/LazyLoadFrameObserver.cpp:95
>>> +        IntersectionObserver::Init options { WTF::nullopt, emptyString(), { } };
>> 
>> Are these not the defaults? Why do we need to specify nullopt and empty string?
> 
> It is a simple struct, so we have to specify all members.

Doesn’t sound right. Should be able to omit members even when initializing a simple struct.

[Simon Fraser (smfr)]

Have any other browsers enabled lazy iframe loading by default? I seem to recall Chrome had some issues.