[webkit-reviews] review granted: [Bug 227279] Add ID and versioning support for AppHighlights : [Attachment 432296] 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 227279: Add ID and versioning support for AppHighlights
https://bugs.webkit.org/show_bug.cgi?id=227279

Attachment 432296: Patch

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




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

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
> -	  
document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::cr
eate(static_cast<const uint8_t*>(sharedMemory->data()), sharedMemory->size()),
i == index ? ScrollToHighlight::Yes : ScrollToHighlight::No);
> +	  
document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::cr
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
_test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals"
configureJSCForTesting:YES];

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,
"document.execCommand('SelectAll')");

    NSMutableData *modifiedHighlightData = [NSMutableData
dataWithData:highlight.highlight];
    [modifiedHighlightData appendData:[@"TestGarbageData"
dataUsingEncoding:NSUTF8StringEncoding]];

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

    TestWebKitAPI::Util::waitForConditionWithLogging([&] () -> bool {
	return [webViewRestore
stringByEvaluatingJavaScript:@"internals.numberOfAppHighlights()"].intValue ==
1;
    }, 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
length:sizeof(maxversion)];

A lot of your objc stars are on the wrong side