[webkit-reviews] review granted: [Bug 233266] ImageAnalysisQueue should analyze image elements that are loaded after the call to enqueueAllImages() : [Attachment 449292] For EWS

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 233266: ImageAnalysisQueue should analyze image elements that are loaded
after the call to enqueueAllImages()
https://bugs.webkit.org/show_bug.cgi?id=233266

Attachment 449292: For EWS

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




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