Bug 237127 - Refactor logic for showing "Markup Image" and Quick Note items in the callout bar
Summary: Refactor logic for showing "Markup Image" and Quick Note items in the callout...
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-23 20:48 PST by Wenson Hsieh
Modified: 2022-02-24 12:15 PST (History)
6 users (show)

See Also:


Attachments
For EWS (34.22 KB, patch)
2022-02-23 21:12 PST, Wenson Hsieh
megan_gardner: review+
Details | Formatted Diff | Diff
For landing (34.45 KB, patch)
2022-02-24 09:33 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-23 20:48:17 PST
.
Comment 1 Radar WebKit Bug Importer 2022-02-23 20:48:33 PST
<rdar://problem/89396617>
Comment 2 Wenson Hsieh 2022-02-23 21:12:26 PST
Created attachment 453068 [details]
For EWS
Comment 3 Megan Gardner 2022-02-24 08:11:58 PST
Comment on attachment 453068 [details]
For EWS

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKContentViewEditingActions.mm:96
> +    EXPECT_FALSE([menuBuilder containsActionWithTitle:WebCore::contextMenuItemTagAddHighlightToNewQuickNote()]);

You should still test for both App Highlight menu items. If things change around the logic for which item to show when, it's unlikely that this test would be updated as a part of that. So, best to just do it now.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKContentViewEditingActions.mm:104
> +    EXPECT_TRUE([menuBuilder containsActionWithTitle:WebCore::contextMenuItemTagAddHighlightToNewQuickNote()]);

ditto.
Comment 4 Wenson Hsieh 2022-02-24 09:09:14 PST
Comment on attachment 453068 [details]
For EWS

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

Thanks for the review!

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKContentViewEditingActions.mm:96
>> +    EXPECT_FALSE([menuBuilder containsActionWithTitle:WebCore::contextMenuItemTagAddHighlightToNewQuickNote()]);
> 
> You should still test for both App Highlight menu items. If things change around the logic for which item to show when, it's unlikely that this test would be updated as a part of that. So, best to just do it now.

Sounds good — I'll add checks to verify that the "Add to existing Quick Note" items are not also present in either case.
Comment 5 Wenson Hsieh 2022-02-24 09:33:48 PST
Created attachment 453109 [details]
For landing
Comment 6 EWS 2022-02-24 12:15:00 PST
Committed r290450 (247752@main): <https://commits.webkit.org/247752@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453109 [details].