Bug 236628

Summary: [macOS] Add a "Markup Image" item to the sharing services picker context menu
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, kangil.han, katherine_cheney, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 236602    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Rebase on trunk
ews-feeder: commit-queue-
Fix Big Sur build
none
For landing none

Description Wenson Hsieh 2022-02-14 18:28:58 PST 
rdar://86446810
Comment 1 Wenson Hsieh 2022-02-14 22:53:01 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-02-15 14:58:34 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2022-02-15 15:13:05 PST 
Created attachment 452098 [details]
Fix Big Sur build
Comment 4 Darin Adler 2022-02-15 15:44:56 PST 
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.
Comment 5 Wenson Hsieh 2022-02-15 17:32:53 PST 
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).
Comment 6 Darin Adler 2022-02-15 17:47:45 PST 
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.
Comment 7 Wenson Hsieh 2022-02-15 18:25:56 PST 
Created attachment 452121 [details]
For landing
Comment 8 EWS 2022-02-16 01:21:09 PST 
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].