Bug 236980 - [iOS] Adjust some behaviors around the "Markup Image" action in the callout bar
Summary: [iOS] Adjust some behaviors around the "Markup Image" action in the callout bar
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:
Blocks:
 
Reported: 2022-02-21 09:10 PST by Wenson Hsieh
Modified: 2022-02-21 20:06 PST (History)
6 users (show)

See Also:


Attachments
Patch (22.72 KB, patch)
2022-02-21 09:52 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For landing (22.53 KB, patch)
2022-02-21 16:51 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-21 09:10:25 PST
.
Comment 1 Wenson Hsieh 2022-02-21 09:52:13 PST
Created attachment 452742 [details]
Patch
Comment 2 Aditya Keerthi 2022-02-21 15:51:22 PST
Comment on attachment 452742 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:100
> +using namespace WebCore;

I think we prefer to avoid adding `using namespace` in new code, and it looks like you would just need to add prefixes to `contextMenuItemTitleMarkupImage()` below.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:317
> +    [webView objectByEvaluatingJavaScript:@"image = document.images[0]; getSelection().setBaseAndExtent(document.body, 0, image, 1);"];

Nit: I find this easier to read without the `image` variable.

(if you prefer to keep it, could use `let` for consistency with line 297)
Comment 3 Wenson Hsieh 2022-02-21 16:12:37 PST
Comment on attachment 452742 [details]
Patch

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

Thanks for the review!

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:100
>> +using namespace WebCore;
> 
> I think we prefer to avoid adding `using namespace` in new code, and it looks like you would just need to add prefixes to `contextMenuItemTitleMarkupImage()` below.

👍🏻 Changed to just use `WebCore::` namespace prefixes.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:317
>> +    [webView objectByEvaluatingJavaScript:@"image = document.images[0]; getSelection().setBaseAndExtent(document.body, 0, image, 1);"];
> 
> Nit: I find this easier to read without the `image` variable.
> 
> (if you prefer to keep it, could use `let` for consistency with line 297)

Good point — changed this to just `getSelection().setBaseAndExtent(document.body, 0, document.images[0], 1);`
Comment 4 Wenson Hsieh 2022-02-21 16:51:24 PST
Created attachment 452800 [details]
For landing
Comment 5 EWS 2022-02-21 20:05:15 PST
Committed r290286 (247610@main): <https://commits.webkit.org/247610@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452800 [details].
Comment 6 Radar WebKit Bug Importer 2022-02-21 20:06:19 PST
<rdar://problem/89272083>