[webkit-reviews] review granted: [Bug 235664] ImageAnalysisQueue should prioritize elements that intersect the visible viewport : [Attachment 450092] 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 235664: ImageAnalysisQueue should prioritize elements that intersect the
visible viewport
https://bugs.webkit.org/show_bug.cgi?id=235664

Attachment 450092: For EWS

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 450092
  --> https://bugs.webkit.org/attachment.cgi?id=450092
For EWS

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

> Source/WebCore/page/ImageAnalysisQueue.h:65
> +    inline static bool isHigherPriority(const Task&, const Task&);
> +    inline unsigned nextTaskNumber();

I don’t think either of these "inline" keywords are needed or helpful.

> Source/WebCore/page/ImageAnalysisQueue.h:76
> +inline bool ImageAnalysisQueue::isHigherPriority(const
ImageAnalysisQueue::Task& left, const ImageAnalysisQueue::Task& right)

Can just write Task for the argument types here without the class prefix, since
these are inside the function definition.

Once we can use C++20 comparison, this would be better written as an ordering
function rather than a "isHigher" function, maybe with the name
orderingByPriority, although that doesn't make it clear what the secondary sort
would be. I find the name isHigherPriority ambiguous for a function that takes
two arguments.

> Tools/TestWebKitAPI/cocoa/ImageAnalysisTestingUtilities.mm:158
> +    RetainPtr<NSURL> _imageURL;
> +    RetainPtr<NSURL> _pageURL;

In tests it seems we can/should use ARC rather than relying on RetainPtr for
Objectve-C object lifetimes. Future is ARC. WebKit non-ARC code is just a
legacy we’ll eventually have to overcome/revise. Maybe we can start doing this
a source file at a time in the rest of WebKit too.