SSIA
<rdar://problem/74363578>
Created attachment 420392 [details] Patch
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?
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.
Created attachment 420422 [details] Patch
Created attachment 420423 [details] Rebaseline test
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.
> > > > 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).
(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.
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
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.
Created attachment 420599 [details] For landing
Created attachment 420606 [details] Rebase on trunk
Committed r272997: <https://commits.webkit.org/r272997> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420606 [details].