Bug 221917 – [macOS] Introduce a new context menu item to preview images

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

SSIA

[Radar WebKit Bug Importer]

<rdar://problem/74363578>

[Wenson Hsieh]

Created attachment 420392 [details]
Patch

[Darin Adler]

Comment on attachment 420392 [details]
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?

[Wenson Hsieh]

Comment on attachment 420392 [details]
Patch

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

Thanks for the 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.

Fair point — removed all the #endifs on short code snippets like this, in this patch.

>> 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?

No reason! Moved to the top, to match the other items.

>> 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?

Hm…if I understand correctly, I think it's probably okay for an enum type like this that's internal to the WebKit stack, but dangerous to shift enum values that are exposed via SPI (which may lead to bincompat bugs like https://bugs.webkit.org/show_bug.cgi?id=220745). I can see some value in making these enum values match up with the SPI-exposed ones though, so I'll remove the `ENABLE(IMAGE_EXTRACTION)` guard here.

>> 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?

Using a simple test app, it looks like linking WebKit (indirectly) pulls in the UniformTypeIdentifiers framework on all the WebKit child processes and the UI process in shipping WebKit anyways, so I think this should not have any impact on launch time/startup cost.

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

Using the (same) simple test app, it looks like the web process and UI process both indirectly pull in Quartz.framework on shipping WebKit — however, the networking process currently doesn't pull in Quartz, so this linking could potentially regress the startup type in the networking process.

I'll change this so that we softlink Quartz instead.

[Wenson Hsieh]

Created attachment 420422 [details]
Patch

[Wenson Hsieh]

Created attachment 420423 [details]
Rebaseline test

[Darin Adler]

Comment on attachment 420392 [details]
Patch

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

>>> 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?
> 
> Hm…if I understand correctly, I think it's probably okay for an enum type like this that's internal to the WebKit stack, but dangerous to shift enum values that are exposed via SPI (which may lead to bincompat bugs like https://bugs.webkit.org/show_bug.cgi?id=220745). I can see some value in making these enum values match up with the SPI-exposed ones though, so I'll remove the `ENABLE(IMAGE_EXTRACTION)` guard here.

I believe the issue was that these values, intended to be internal only, are indirectly exposed by using them as tags on NSMenuItem objects, so people relied on them matching the actual API or SPI values.

>>> Source/WebKit/Configurations/WebKit.xcconfig:135
>>> +WK_QUARTZ_LDFLAGS_MACOS_SINCE_1200 = -framework Quartz;
>> 
>> Same question?
> 
> Using the (same) simple test app, it looks like the web process and UI process both indirectly pull in Quartz.framework on shipping WebKit — however, the networking process currently doesn't pull in Quartz, so this linking could potentially regress the startup type in the networking process.
> 
> I'll change this so that we softlink Quartz instead.

I believe that in the past people told me this was not an issue for frameworks "in the shared cache" but not 100% sure about that. Hope you didn’t spend too much effort on soft linking based on a shared misunderstanding you and I might have had.

[Wenson Hsieh]

> > 
> > Using the (same) simple test app, it looks like the web process and UI process both indirectly pull in Quartz.framework on shipping WebKit — however, the networking process currently doesn't pull in Quartz, so this linking could potentially regress the startup type in the networking process.
> > 
> > I'll change this so that we softlink Quartz instead.
> 
> I believe that in the past people told me this was not an issue for
> frameworks "in the shared cache" but not 100% sure about that. Hope you
> didn’t spend too much effort on soft linking based on a shared
> misunderstanding you and I might have had.

Ah, so it does look like Quartz.framework is in the DYLD shared cache on macOS — at least, according to `dyld_shared_cache_util -list | grep "Quartz.framework"`.

That said, soft-linking QuickLookUI (in Quartz) might still not be a bad idea, given that it makes little sense for, say, the WebKit networking process to pull in Quartz (or any of its subframeworks).

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #8)
> > > 
> > > Using the (same) simple test app, it looks like the web process and UI process both indirectly pull in Quartz.framework on shipping WebKit — however, the networking process currently doesn't pull in Quartz, so this linking could potentially regress the startup type in the networking process.
> > > 
> > > I'll change this so that we softlink Quartz instead.
> > 
> > I believe that in the past people told me this was not an issue for
> > frameworks "in the shared cache" but not 100% sure about that. Hope you
> > didn’t spend too much effort on soft linking based on a shared
> > misunderstanding you and I might have had.
> 
> Ah, so it does look like Quartz.framework is in the DYLD shared cache on
> macOS — at least, according to `dyld_shared_cache_util -list | grep
> "Quartz.framework"`.
> 
> That said, soft-linking QuickLookUI (in Quartz) might still not be a bad
> idea, given that it makes little sense for, say, the WebKit networking
> process to pull in Quartz (or any of its subframeworks).

...Tim Horton just reminded me that QuickLookUI doesn't exist in the base system on macOS, so we'll need to softlink in any case.

[Tim Horton]

Comment on attachment 420423 [details]
Rebaseline test

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

> Source/WebKit/ChangeLog:15
> +        Link against Quartz and UniformTypeIdentifiers.

LIES

> Source/WebKit/ChangeLog:29
> +        Add a non-const version of `webHitTestResultData()`, so that we can generate a `CGImage` using
> +        `WebHitTestResultData`'s bitmap data.

So confused why this needs non-const hit test result data

[Wenson Hsieh]

Comment on attachment 420423 [details]
Rebaseline test

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

Thanks for the review!

>> Source/WebKit/ChangeLog:15
>> +        Link against Quartz and UniformTypeIdentifiers.
> 
> LIES

Whoops, fixed!

>> Source/WebKit/ChangeLog:29
>> +        `WebHitTestResultData`'s bitmap data.
> 
> So confused why this needs non-const hit test result data

This is because ShareableBitmap::makeCGImageCopy() is non-const, due to the context object passed into `CGBitmapContextCreateWithData` being a non-const pointer :/

We use the handler to release (and possible destroy) the bitmap, so perhaps the non-const is warranted? But it's also weird that a method that just copies an image is non-const — I think I'll look into this in a followup.

[Wenson Hsieh]

Created attachment 420599 [details]
For landing

[Wenson Hsieh]

Created attachment 420606 [details]
Rebase on trunk

[EWS]

Committed r272997: <https://commits.webkit.org/r272997>

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