[webkit-reviews] review granted: [Bug 227279] Add ID and versioning support for AppHighlights : [Attachment 432296] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 25 15:46:20 PDT 2021

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 227279: Add ID and versioning support for AppHighlights

Attachment 432296: Patch


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

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:116
> +    std::optional<unsigned> version;

I think you want to use an explicitly-sized datatype here (uint64_t also?)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7616
> -	  
eate(static_cast<const uint8_t*>(sharedMemory->data()), sharedMemory->size()),
i == index ? ScrollToHighlight::Yes : ScrollToHighlight::No);
> +	  
eate(static_cast<const uint8_t*>(sharedMemory->data()), ipcHandle.dataSize), i
== index ? ScrollToHighlight::Yes : ScrollToHighlight::No);

If something takes us a whole day to figure out, you should probably call it
out in the changelog :)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:147
> +TEST(AppHighlights, AppHighlightCreateAndRestoreWithExtraBytes)

Each of these could use a reason why they are important properties to uphold
(for this one: "It is important that extra data at the end of the highlight
blob is ignored, so that future versions of the blob format can add additional
data but still be decoded successfully by versions of WebKit that only know
about the v1 format" or something).

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:149
> +    WKWebViewConfiguration *configuration = [WKWebViewConfiguration

This whole file would hugely benefit from some helper functions. You could
imagine this test just looking like:

TEST(AppHighlights, AppHighlightCreateAndRestoreWithExtraBytes)
    const char *html = "Test";
    _WKAppHighlight *highlight = createAppHighlightWithHTML(html,

    NSMutableData *modifiedHighlightData = [NSMutableData
    [modifiedHighlightData appendData:[@"TestGarbageData"

    WKWebView *webViewRestore = createWebViewForAppHighlightsWithHTML(html);
    [webViewRestore _restoreAppHighlights:@[ modifiedHighlightData ]];

    TestWebKitAPI::Util::waitForConditionWithLogging([&] () -> bool {
	return [webViewRestore
stringByEvaluatingJavaScript:@"internals.numberOfAppHighlights()"].intValue ==
    }, 2, @"Expected Highlights to be populated.");

and then most of the other tests could also ues createAppHighlightWithHTML and
createWebViewForAppHighlightsWithHTML, and adding tests would be cheaper and
the interesting parts would be more central and easier to read.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:195
> +	   unsigned maxversion = 100; // increase this if we ever go past
version 100 in AppHighlightRangeData for the most accurate tests

You could infer this instead of hardcoding it by making a blob and looking at
the version field :) But this is fine.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:196
> +	   NSData* versionData = [NSData dataWithBytes:&maxversion

A lot of your objc stars are on the wrong side

More information about the webkit-reviews mailing list