[webkit-reviews] review granted: [Bug 221917] [macOS] Introduce a new context menu item to preview images : [Attachment 420392] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 221917: [macOS] Introduce a new context menu item to preview images
https://bugs.webkit.org/show_bug.cgi?id=221917

Attachment 420392: Patch

https://bugs.webkit.org/attachment.cgi?id=420392&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 420392
  --> https://bugs.webkit.org/attachment.cgi?id=420392
Patch

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

> Source/WebCore/page/ContextMenuController.cpp:525
> +#endif // ENABLE(IMAGE_EXTRACTION)

These comments on #endif don’t seem too helpful when the code is <10 lines
long.

> Source/WebCore/page/ContextMenuController.cpp:869
> +		   ContextMenuItem RevealImageItem(ActionType,
ContextMenuItemTagRevealImage, contextMenuItemTagRevealImage());

Why is this not at the top of the function with the other ContextMenuItem
objects?

> Source/WebCore/platform/ContextMenuItem.h:152
> +#if ENABLE(IMAGE_EXTRACTION)

Very concerned about #if in the definition of this enum since I think we
recently learned that binary compatibility with some clients (at least Mail)
relies on this enumeration matching the one in WKContextMenuItemTypes.h. I
think we need to do something to make this less fragile/dangerous. Maybe we can
start by not making new enumeration values conditional?

> Source/WebKit/Configurations/WebKit.xcconfig:118
> +WK_UNIFORM_TYPE_IDENTIFIERS_LDFLAGS_MACOS_SINCE_1200 = -framework
UniformTypeIdentifiers;

Does this have a launch time or web process startup time cost?

> Source/WebKit/Configurations/WebKit.xcconfig:135
> +WK_QUARTZ_LDFLAGS_MACOS_SINCE_1200 = -framework Quartz;

Same question?