Bug 236628 - [macOS] Add a "Markup Image" item to the sharing services picker context menu
Summary: [macOS] Add a "Markup Image" item to the sharing services picker context menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 236602
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-14 18:28 PST by Wenson Hsieh
Modified: 2022-02-16 01:21 PST (History)
13 users (show)

See Also:


Attachments
Patch (29.21 KB, patch)
2022-02-14 22:53 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (29.21 KB, patch)
2022-02-15 14:58 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Big Sur build (29.30 KB, patch)
2022-02-15 15:13 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For landing (29.52 KB, patch)
2022-02-15 18:25 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].