Bug 215442 - Implement lazy iframe loading
Summary: Implement lazy iframe loading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-13 00:04 PDT by Rob Buis
Modified: 2020-09-12 12:38 PDT (History)
14 users (show)

See Also:


Attachments
Patch (37.33 KB, patch)
2020-08-13 00:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (37.25 KB, patch)
2020-08-13 00:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (37.41 KB, patch)
2020-08-13 02:14 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (37.36 KB, patch)
2020-08-13 04:02 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (40.46 KB, patch)
2020-08-13 06:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (38.91 KB, patch)
2020-08-14 03:23 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (40.97 KB, patch)
2020-08-14 05:23 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (31.45 KB, patch)
2020-08-26 01:46 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (31.45 KB, patch)
2020-08-26 02:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (32.27 KB, patch)
2020-08-26 05:05 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (34.27 KB, patch)
2020-09-01 03:15 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (34.35 KB, patch)
2020-09-01 03:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (34.81 KB, patch)
2020-09-01 09:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (35.85 KB, patch)
2020-09-09 02:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (36.41 KB, patch)
2020-09-09 02:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (34.97 KB, patch)
2020-09-11 08:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (35.90 KB, patch)
2020-09-11 11:42 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (35.96 KB, patch)
2020-09-11 23:43 PDT, 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 2020-08-13 00:04:48 PDT
Implement lazy iframe loading
Comment 1 Rob Buis 2020-08-13 00:08:07 PDT
Created attachment 406499 [details]
Patch
Comment 2 Rob Buis 2020-08-13 00:16:07 PDT
Created attachment 406500 [details]
Patch
Comment 3 Rob Buis 2020-08-13 02:14:23 PDT
Created attachment 406504 [details]
Patch
Comment 4 Rob Buis 2020-08-13 04:02:26 PDT
Created attachment 406506 [details]
Patch
Comment 5 Rob Buis 2020-08-13 06:31:41 PDT
Created attachment 406510 [details]
Patch
Comment 6 EWS Watchlist 2020-08-13 11:49:11 PDT
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
Comment 7 Rob Buis 2020-08-14 03:23:51 PDT
Created attachment 406582 [details]
Patch
Comment 8 Rob Buis 2020-08-14 05:23:55 PDT
Created attachment 406587 [details]
Patch
Comment 9 Radar WebKit Bug Importer 2020-08-20 00:05:15 PDT
<rdar://problem/67458008>
Comment 10 Rob Buis 2020-08-26 01:46:28 PDT
Created attachment 407283 [details]
Patch
Comment 11 Rob Buis 2020-08-26 02:49:10 PDT
Created attachment 407286 [details]
Patch
Comment 12 Rob Buis 2020-08-26 05:05:31 PDT
Created attachment 407291 [details]
Patch
Comment 13 Rob Buis 2020-09-01 03:15:07 PDT
Created attachment 407667 [details]
Patch
Comment 14 Rob Buis 2020-09-01 03:59:27 PDT
Created attachment 407670 [details]
Patch
Comment 15 Rob Buis 2020-09-01 09:41:03 PDT
Created attachment 407689 [details]
Patch
Comment 16 Rob Buis 2020-09-09 02:31:15 PDT
Created attachment 408320 [details]
Patch
Comment 17 Rob Buis 2020-09-09 02:57:18 PDT
Created attachment 408321 [details]
Patch
Comment 18 Darin Adler 2020-09-10 17:20:10 PDT
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.
Comment 19 Rob Buis 2020-09-11 01:56:58 PDT
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.
Comment 20 Rob Buis 2020-09-11 08:55:54 PDT
Created attachment 408536 [details]
Patch
Comment 21 Rob Buis 2020-09-11 11:42:13 PDT
Created attachment 408550 [details]
Patch
Comment 22 Rob Buis 2020-09-11 23:43:05 PDT
Created attachment 408586 [details]
Patch
Comment 23 Rob Buis 2020-09-12 01:11:37 PDT
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.
Comment 24 EWS 2020-09-12 01:35:31 PDT
Committed r266976: <https://trac.webkit.org/changeset/266976>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408586 [details].
Comment 25 Darin Adler 2020-09-12 12:38:05 PDT
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.