Bug 233266 – ImageAnalysisQueue should analyze image elements that are loaded after the call to enqueueAllImages()

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

.

[Radar WebKit Bug Importer]

<rdar://problem/85731875>

[Wenson Hsieh]

Created attachment 449292 [details]
For EWS

[Darin Adler]

Comment on attachment 449292 [details]
For EWS

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

> Source/WebCore/page/ImageAnalysisQueue.cpp:68
> +    auto& renderImage = downcast<RenderImage>(*element.renderer());

I would just call this local variable "renderer". Yes the type is RenderImage, but that doesn’t need to be the name as well.

> Source/WebCore/page/ImageAnalysisQueue.cpp:99
> +    auto imageIterator = document.images()->createIterator();
> +    for (RefPtr node = imageIterator.next(); node; node = imageIterator.next()) {
> +        if (auto* element = dynamicDowncast<HTMLElement>(*node))
> +            enqueueIfNeeded(*element);
> +    }

It’s a mistake to use Document::images here. An HTMLCollection is designed as an API for use from JavaScript and offers no significant advantages over descendantsOfType<HTMLImageElement> for use in C++ code.

    for (auto& image : descendantsOfType<HTMLImageElement>(document))
        enqueueIfNeeded(image);

Using Document::images will just waste memory and time allocating objects to fulfill the abstraction needed to work with the JavaScript code.

Note also that the m_page check, the isConnected check, and document check in ImageAnalysisQueue::enqueueIfNeeded are all unnecessary here. But harmless I suppose.

> Source/WebCore/page/ImageAnalysisQueue.h:51
> +    void enqueueIfNeeded(HTMLElement&);

This can use HTMLImageElement.

> Source/WebCore/page/ImageAnalysisQueue.h:61
> +    WeakHashSet<HTMLElement> m_queuedElements;
> +    Deque<WeakPtr<HTMLElement>> m_queue;

These can use HTMLImageElement.

[Wenson Hsieh]

Comment on attachment 449292 [details]
For EWS

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

Thanks for the review!

>> Source/WebCore/page/ImageAnalysisQueue.cpp:68
>> +    auto& renderImage = downcast<RenderImage>(*element.renderer());
> 
> I would just call this local variable "renderer". Yes the type is RenderImage, but that doesn’t need to be the name as well.

Renamed to `renderer`.

>> Source/WebCore/page/ImageAnalysisQueue.cpp:99
>> +    }
> 
> It’s a mistake to use Document::images here. An HTMLCollection is designed as an API for use from JavaScript and offers no significant advantages over descendantsOfType<HTMLImageElement> for use in C++ code.
> 
>     for (auto& image : descendantsOfType<HTMLImageElement>(document))
>         enqueueIfNeeded(image);
> 
> Using Document::images will just waste memory and time allocating objects to fulfill the abstraction needed to work with the JavaScript code.
> 
> Note also that the m_page check, the isConnected check, and document check in ImageAnalysisQueue::enqueueIfNeeded are all unnecessary here. But harmless I suppose.

Good point — changed this to just use `descendantsOfType<>`!

Also removed the unnecessary early returns.

>> Source/WebCore/page/ImageAnalysisQueue.h:51
>> +    void enqueueIfNeeded(HTMLElement&);
> 
> This can use HTMLImageElement.

Changed to HTMLImageElement.

>> Source/WebCore/page/ImageAnalysisQueue.h:61
>> +    Deque<WeakPtr<HTMLElement>> m_queue;
> 
> These can use HTMLImageElement.

Changed these to <HTMLImageElement> as well.

[Wenson Hsieh]

Created attachment 449305 [details]
For EWS

[Darin Adler]

Comment on attachment 449305 [details]
For EWS

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

> Source/WebCore/page/ImageAnalysisQueue.h:60
> +    WeakHashSet<HTMLImageElement> m_queuedElements;
> +    Deque<WeakPtr<HTMLImageElement>> m_queue;

One other thought. The reason that ListHashSet exists is for use cases like this one where we want an ordered collection without duplicates. While it doesn’t have "deque" in its name, it does have an efficient takeFirst operation, and and new items are added at the end, so it’s much like a deque combined with a set.

I don’t know, though, how well it works with WeakPtr. But it would be nicer to use that combined data structure rather than building it out of the two pieces manually as we do here.

[Wenson Hsieh]

(In reply to Darin Adler from comment #6)
> Comment on attachment 449305 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449305&action=review
> 
> > Source/WebCore/page/ImageAnalysisQueue.h:60
> > +    WeakHashSet<HTMLImageElement> m_queuedElements;
> > +    Deque<WeakPtr<HTMLImageElement>> m_queue;
> 
> One other thought. The reason that ListHashSet exists is for use cases like
> this one where we want an ordered collection without duplicates. While it
> doesn’t have "deque" in its name, it does have an efficient takeFirst
> operation, and and new items are added at the end, so it’s much like a deque
> combined with a set.
> 
> I don’t know, though, how well it works with WeakPtr. But it would be nicer
> to use that combined data structure rather than building it out of the two
> pieces manually as we do here.

In this case, elements are removed from `m_queue` when they're analyzed, but not removed from `m_queuedElements`, since want to avoid repeatedly re-analyzing elements.

[EWS]

Committed r288102 (246116@main): <https://commits.webkit.org/246116@main>

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