Bug 227279 – Add ID and versioning support for AppHighlights

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Add ID and versioning support for AppHighlights

[Megan Gardner]

Created attachment 432124 [details]
Patch

[Wenson Hsieh]

Comment on attachment 432124 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:110
> +    constexpr unsigned maxSupportedAppHighlighVersion = 1;

Nit - maxSupportedAppHighlightVersion

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:127
> +    

Nit - extra newline.

[Chris Dumez]

Comment on attachment 432124 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:44
> +static constexpr uint64_t highlightFileSignature = 0x3132303248504141;

static is not needed.

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:112
> +    std::optional<uint64_t> decodedHighlightFileSignature;

I am a little unclear why we need both a version and a signature. Both are constants that we can bump at any point. Why isn't the version sufficient?

[Tim Horton]

Comment on attachment 432124 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:119
> +    if (!version || *version > maxSupportedAppHighlighVersion)

I think forward compatibility is likely a P1 for this code, so we might need to think about this some more

[Sam Weinig]

Comment on attachment 432124 [details]
Patch

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

>> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:44
>> +static constexpr uint64_t highlightFileSignature = 0x3132303248504141;
> 
> static is not needed.

Probably worth explaining how this value was derived, even if that is just say it is a random value or something.

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:98
> +    encoder << highlightFileSignature;

Is this encoder/decoder used for both persisten and IPC encoding? If so, perhaps we only need to encode/decode this when doing persistent encoding/decoding?

[Megan Gardner]

Comment on attachment 432124 [details]
Patch

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

>> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:98
>> +    encoder << highlightFileSignature;
> 
> Is this encoder/decoder used for both persisten and IPC encoding? If so, perhaps we only need to encode/decode this when doing persistent encoding/decoding?

This is only used for persistent encoding. Technically the blob is passed via IPC with other data, but it is not decoded by any process other than WebKit, only stored.

[Megan Gardner]

Created attachment 432296 [details]
Patch

[Tim Horton]

Comment on attachment 432296 [details]
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::create(static_cast<const uint8_t*>(sharedMemory->data()), sharedMemory->size()), i == index ? ScrollToHighlight::Yes : ScrollToHighlight::No);
> +        document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(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

[Tim Horton]

(In reply to Megan Gardner from comment #6)
> Comment on attachment 432124 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432124&action=review
> 
> >> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:98
> >> +    encoder << highlightFileSignature;
> > 
> > Is this encoder/decoder used for both persisten and IPC encoding? If so, perhaps we only need to encode/decode this when doing persistent encoding/decoding?
> 
> This is only used for persistent encoding. Technically the blob is passed
> via IPC with other data, but it is not decoded by any process other than
> WebKit, only stored.

Some other encoders assert that they're only used for IPC or only used for Persistence; you could do the same!

[Megan Gardner]

Created attachment 432429 [details]
Patch

[Tim Horton]

Comment on attachment 432429 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:166
> +    finished = YES;
> +
> +    TestWebKitAPI::Util::run(&finished);

This makes no sense, you can get rid of the finished bit entirely, it is doing nothing

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:192
> +    finished = YES;
> +
> +    TestWebKitAPI::Util::run(&finished);

Ditto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:214
> +    finished = YES;
> +
> +    TestWebKitAPI::Util::run(&finished);

Etc.

[Megan Gardner]

Created attachment 432438 [details]
Patch

[Tim Horton]

Comment on attachment 432438 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:128
> +// Ensure that future versions of the blog format can add additional data and still be decoded successfully by version of WebKit that only know about the current format.

"blog"?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:145
> +// Ensure that we don't block against later versions with an explicite version check.

"explicite"?

I would say something more like "Older versions of WebKit need to be able to decode blobs encoded on newer versions of WebKit, so ensure that <blah blah>"

[Megan Gardner]

Created attachment 432534 [details]
Patch

[Tim Horton]

Comment on attachment 432534 [details]
Patch

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

> Source/WTF/wtf/persistence/PersistentDecoder.cpp:38
> +    , m_bufferBegin(buffer)

How does this differ from m_buffer?? (I don't think it is, I think you don't need to add anything)

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:113
> +    bool v0 = false;

I would go with `isVersion0` or something slightly less terse. Or maybe just move `std::optional<uint64_t> version` up here and just set that directly??

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:121
> +        if (!decoder.rewind(sizeof(highlightFileSignature)))
> +            return std::nullopt;

Nice

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:122
> +        v0 = 1;

`true`?

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:129
> +        identifier = "NULL";

Maybe just an empty (or null?) string? Then other folks looking at this can just check isEmpty instead of having to spread == "NULL" checks around. This is fine, too, though.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:128
> +// Ensure that future versions of the blog format can add additional data and still be decoded successfully by version of WebKit that only know about the current format.

Please go back and re-read my old comments about this comment and the next one (and any other previous review comments you might have missed)

[Megan Gardner]

Created attachment 432547 [details]
Patch

[Tim Horton]

Comment on attachment 432547 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:96
> +    static_assert(!Encoder::isIPCEncoder, "AppHighlightRangeData should not be used by WTF::IPC::Encoder");

Drop the WTF:: (IPC::Encoder is not in WTF namespace).

[Radar WebKit Bug Importer]

<rdar://problem/79950614>

[Megan Gardner]

Created attachment 432620 [details]
Patch

[Tim Horton]

Comment on attachment 432620 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:151
> +    uint64_t maxversion = std::numeric_limits<uint64_t>::max();

maximumVersion

[Megan Gardner]

Created attachment 432628 [details]
Patch for landing

[EWS]

Committed r279444 (239300@main): <https://commits.webkit.org/239300@main>

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