Bug 236628 – [macOS] Add a "Markup Image" item to the sharing services picker context menu

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://86446810

[Wenson Hsieh]

Created attachment 451997 [details]
Patch

[Wenson Hsieh]

Created attachment 452094 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 452098 [details]
Fix Big Sur build

[Darin Adler]

Comment on attachment 452098 [details]
Fix Big Sur build

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:144
>      if (ImageControlsMac::isImageControlsButtonElement(node)) {
> -        auto shadowHost = node.shadowHost();
> +        RefPtr shadowHost = node.shadowHost();
>          if (!is<HTMLImageElement>(*shadowHost))
>              return false;
>          if (auto* image = imageFromImageElementNode(*shadowHost)) {

This mix of early return and nested if doesn’t seem logical. Either 3 nested if statements or 3 early returns would be better than 2 and 1.

[Wenson Hsieh]

Comment on attachment 452098 [details]
Fix Big Sur build

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

Thanks for the review!

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:144
>>          if (auto* image = imageFromImageElementNode(*shadowHost)) {
> 
> This mix of early return and nested if doesn’t seem logical. Either 3 nested if statements or 3 early returns would be better than 2 and 1.

Good point! Replaced the `if (auto* image = …)` with an early return.

(Also changed this to use the `dynamicDowncast<HTMLImageElement>(…)` idiom here instead of having separate `is<>/downcast<>` calls).

[Darin Adler]

Comment on attachment 452098 [details]
Fix Big Sur build

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

>>> Source/WebCore/dom/mac/ImageControlsMac.cpp:144
>>>          if (auto* image = imageFromImageElementNode(*shadowHost)) {
>> 
>> This mix of early return and nested if doesn’t seem logical. Either 3 nested if statements or 3 early returns would be better than 2 and 1.
> 
> Good point! Replaced the `if (auto* image = …)` with an early return.
> 
> (Also changed this to use the `dynamicDowncast<HTMLImageElement>(…)` idiom here instead of having separate `is<>/downcast<>` calls).

Even though I didn't say anything, I was hoping you would consider that. Making Chris Dumez’s day.

[Wenson Hsieh]

Created attachment 452121 [details]
For landing

[EWS]

Committed r289882 (247320@main): <https://commits.webkit.org/247320@main>

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