| Summary: | [iOS] Adjust some behaviors around the "Markup Image" action in the callout bar | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
| Component: | Platform | Assignee: | 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
Wenson Hsieh
2022-02-21 09:10:25 PST
Created attachment 452742 [details]
Patch
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 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);` Created attachment 452800 [details]
For landing
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]. |