Bug 235664 – ImageAnalysisQueue should prioritize elements that intersect the visible viewport

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 450092 [details]
For EWS

[Radar WebKit Bug Importer]

<rdar://problem/88131827>

[Darin Adler]

Comment on attachment 450092 [details]
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.

[Wenson Hsieh]

Comment on attachment 450092 [details]
For EWS

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

Thanks for the review!

>> Source/WebCore/page/ImageAnalysisQueue.h:65
>> +    inline unsigned nextTaskNumber();
> 
> I don’t think either of these "inline" keywords are needed or helpful.

Good point — removed!

>> 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.

Changed from ImageAnalysisQueue::Task => Task.

Perhaps for now, I can rename the function (and its arguments) for clarity — I was thinking something like `isHigherPriorityThanOtherTask(const Task& task, const Task& other)`, which would make it more clear which direction the comparison goes.

>> Tools/TestWebKitAPI/cocoa/ImageAnalysisTestingUtilities.mm:158
>> +    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.

Got it. Yeah, ideally these would not use MRC, but I think it's tricky in this case because `ImageAnalysisTestingUtilities.mm` is a unified source, so I don't think we can easily mark the file as ARC (without opting out of unified sources, that is). This makes me wonder if we should adjust the way we unify sources, such that we can unify all ARC-enabled source files together (separately from non-ARC files).

If you prefer, I can `@no-unify` this file and add `-fobjc-arc` to this file; otherwise, I'll keep it as-is for now, if that's okay.

[Darin Adler]

Comment on attachment 450092 [details]
For EWS

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

>>> 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.
> 
> Changed from ImageAnalysisQueue::Task => Task.
> 
> Perhaps for now, I can rename the function (and its arguments) for clarity — I was thinking something like `isHigherPriorityThanOtherTask(const Task& task, const Task& other)`, which would make it more clear which direction the comparison goes.

I would use a name like firstIsHigherPriority.

[Darin Adler]

Comment on attachment 450092 [details]
For EWS

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

>>> Tools/TestWebKitAPI/cocoa/ImageAnalysisTestingUtilities.mm:158
>>> +    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.
> 
> Got it. Yeah, ideally these would not use MRC, but I think it's tricky in this case because `ImageAnalysisTestingUtilities.mm` is a unified source, so I don't think we can easily mark the file as ARC (without opting out of unified sources, that is). This makes me wonder if we should adjust the way we unify sources, such that we can unify all ARC-enabled source files together (separately from non-ARC files).
> 
> If you prefer, I can `@no-unify` this file and add `-fobjc-arc` to this file; otherwise, I'll keep it as-is for now, if that's okay.

We need to start turning the tide and not just accumulate more and more non-ARC code.

Not by doing @no-unify, but my making ARC normal and non-ARC the exception in this project. It’s the non-ARC that will need something like @no-unify. But I understand if you can’t be the one to do it with this one source file.

By the way, I think we can convert all our tests and it won’t even be a challenging project. Converting WebKit itself is far more difficult.

[Wenson Hsieh]

Comment on attachment 450092 [details]
For EWS

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

>>>> 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.
>> 
>> Changed from ImageAnalysisQueue::Task => Task.
>> 
>> Perhaps for now, I can rename the function (and its arguments) for clarity — I was thinking something like `isHigherPriorityThanOtherTask(const Task& task, const Task& other)`, which would make it more clear which direction the comparison goes.
> 
> I would use a name like firstIsHigherPriority.

Sounds good — I'll go with firstIsHigherPriority(first, second).

>>>> Tools/TestWebKitAPI/cocoa/ImageAnalysisTestingUtilities.mm:158
>>>> +    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.
>> 
>> Got it. Yeah, ideally these would not use MRC, but I think it's tricky in this case because `ImageAnalysisTestingUtilities.mm` is a unified source, so I don't think we can easily mark the file as ARC (without opting out of unified sources, that is). This makes me wonder if we should adjust the way we unify sources, such that we can unify all ARC-enabled source files together (separately from non-ARC files).
>> 
>> If you prefer, I can `@no-unify` this file and add `-fobjc-arc` to this file; otherwise, I'll keep it as-is for now, if that's okay.
> 
> We need to start turning the tide and not just accumulate more and more non-ARC code.
> 
> Not by doing @no-unify, but my making ARC normal and non-ARC the exception in this project. It’s the non-ARC that will need something like @no-unify. But I understand if you can’t be the one to do it with this one source file.
> 
> By the way, I think we can convert all our tests and it won’t even be a challenging project. Converting WebKit itself is far more difficult.

👍🏻 I'll keep this part of the patch as-is, then — I filed https://bugs.webkit.org/show_bug.cgi?id=235722 to track enabling ARC in TestWebKitAPI.

From a cursory glance, there aren't a whole lot of manual -release/-retains, so (as you mentioned) it should be relatively straightforward! I'll take a look later.

[Wenson Hsieh]

Created attachment 450156 [details]
Patch for landing

[Darin Adler]

Comment on attachment 450156 [details]
Patch for landing

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

> Source/WebCore/page/ImageAnalysisQueue.h:87
> +inline unsigned ImageAnalysisQueue::nextTaskNumber()
> +{
> +    return ++m_currentTaskNumber;
> +}

Just realized this function can be in the .cpp file. Sure, it’s inline, but all callers are also in the .cpp file.

[Darin Adler]

Comment on attachment 450156 [details]
Patch for landing

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

>> Source/WebCore/page/ImageAnalysisQueue.h:87
>> +}
> 
> Just realized this function can be in the .cpp file. Sure, it’s inline, but all callers are also in the .cpp file.

Or, it’s so short it could just be up above:

    unsigned nextTaskNumber() { return ++m_currentTaskNumber; }

[Wenson Hsieh]

(In reply to Darin Adler from comment #10)
> Comment on attachment 450156 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450156&action=review
> 
> >> Source/WebCore/page/ImageAnalysisQueue.h:87
> >> +}
> > 
> > Just realized this function can be in the .cpp file. Sure, it’s inline, but all callers are also in the .cpp file.
> 
> Or, it’s so short it could just be up above:
> 
>     unsigned nextTaskNumber() { return ++m_currentTaskNumber; }

Sounds good — will move it above!

[Wenson Hsieh]

Created attachment 450158 [details]
Patch for landing

[EWS]

Committed r288690 (246489@main): <https://commits.webkit.org/246489@main>

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