[webkit-reviews] review granted: [Bug 221802] Update data path for App Highlights : [Attachment 420331] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 221802: Update data path for App Highlights
https://bugs.webkit.org/show_bug.cgi?id=221802

Attachment 420331: Patch

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




--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 420331
  --> https://bugs.webkit.org/attachment.cgi?id=420331
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:42
> +#if ENABLE(APP_HIGHLIGHTS)

This has an oddly tight scope, I'd expect it to be after
AppHighlightRangeData.h.

> Source/WebCore/Modules/highlight/CreateNewGroupForHighlight.h:2
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

It's 2021!

> Source/WebCore/Modules/highlight/CreateNewGroupForHighlight.h:30
> +enum class CreateNewGroupForHighlight : bool { No, Yes };

Any reason not to put this in AppHighlightInformation.h? Seems wildly overkill
in this case. Though it's also fine.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1434
> -	   [delegate _webView:self updateAppHighlightsStorage:data];
> +	   [delegate _webView:self
updateAppHighlightsStorage:highlightInformation.highlight->createNSData().get()
];

This will change (to have more parameters and a better name), but in a
subsequent version of the patch?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2042
> +    Vector<Ref<WebCore::SharedBuffer>> buffers;

You can go straight to SharedMemory here and skip a step later.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:371
> -- (void)_restoreAppHighlights:(NSData *)data
WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> +- (void)_restoreAppHighlights:(NSArray<NSData *> *)data
WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Better make sure there are no clients of the old version.

> Source/WebKit/UIProcess/WebPageProxy.cpp:9846
> +    // MESSAGE_CHECK(m_process, info);

Don't check this in. Maybe you mean to check the buffer inside the info?