Bug 236980

Summary: [iOS] Adjust some behaviors around the "Markup Image" action in the callout bar
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, hi, katherine_cheney, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
For landing none

Wenson Hsieh
Reported 2022-02-21 09:10:25 PST
.
Attachments
Patch (22.72 KB, patch)
2022-02-21 09:52 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
For landing (22.53 KB, patch)
2022-02-21 16:51 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2022-02-21 09:52:13 PST
Created attachment 452742 [details] Patch
Aditya Keerthi
Comment 2 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)
Wenson Hsieh
Comment 3 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);`
Wenson Hsieh
Comment 4 2022-02-21 16:51:24 PST
Created attachment 452800 [details] For landing
EWS
Comment 5 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].
Radar WebKit Bug Importer
Comment 6 2022-02-21 20:06:19 PST
<rdar://problem/89272083>
Note You need to log in before you can comment on or make changes to this bug.