Bug 227279 - Add ID and versioning support for AppHighlights
Summary: Add ID and versioning support for AppHighlights
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-22 22:36 PDT by Megan Gardner
Modified: 2021-06-30 16:54 PDT (History)
11 users (show)

See Also:


Attachments
Patch (7.20 KB, patch)
2021-06-23 19:03 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (17.18 KB, patch)
2021-06-25 15:21 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (25.13 KB, patch)
2021-06-28 15:16 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (24.30 KB, patch)
2021-06-28 16:08 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.74 KB, patch)
2021-06-29 14:40 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.34 KB, patch)
2021-06-29 16:00 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.14 KB, patch)
2021-06-30 13:19 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (28.14 KB, patch)
2021-06-30 14:52 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2021-06-22 22:36:46 PDT
Add ID and versioning support for AppHighlights
Comment 1 Megan Gardner 2021-06-23 19:03:46 PDT
Created attachment 432124 [details]
Patch
Comment 2 Wenson Hsieh 2021-06-23 19:10:03 PDT
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.
Comment 3 Chris Dumez 2021-06-23 19:30:41 PDT
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?
Comment 4 Tim Horton 2021-06-23 20:55:42 PDT
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
Comment 5 Sam Weinig 2021-06-24 08:19:12 PDT
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?
Comment 6 Megan Gardner 2021-06-24 12:58:10 PDT
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.
Comment 7 Megan Gardner 2021-06-25 15:21:34 PDT
Created attachment 432296 [details]
Patch
Comment 8 Tim Horton 2021-06-25 15:46:20 PDT
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
Comment 9 Tim Horton 2021-06-25 15:46:51 PDT
(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!
Comment 10 Megan Gardner 2021-06-28 15:16:16 PDT
Created attachment 432429 [details]
Patch
Comment 11 Tim Horton 2021-06-28 15:25:58 PDT
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.
Comment 12 Megan Gardner 2021-06-28 16:08:08 PDT
Created attachment 432438 [details]
Patch
Comment 13 Tim Horton 2021-06-28 16:26:03 PDT
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>"
Comment 14 Megan Gardner 2021-06-29 14:40:13 PDT
Created attachment 432534 [details]
Patch
Comment 15 Tim Horton 2021-06-29 14:48:42 PDT
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)
Comment 16 Megan Gardner 2021-06-29 16:00:35 PDT
Created attachment 432547 [details]
Patch
Comment 17 Tim Horton 2021-06-29 17:11:18 PDT
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).
Comment 18 Radar WebKit Bug Importer 2021-06-29 22:37:16 PDT
<rdar://problem/79950614>
Comment 19 Megan Gardner 2021-06-30 13:19:43 PDT
Created attachment 432620 [details]
Patch
Comment 20 Tim Horton 2021-06-30 14:22:15 PDT
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
Comment 21 Megan Gardner 2021-06-30 14:52:08 PDT
Created attachment 432628 [details]
Patch for landing
Comment 22 EWS 2021-06-30 16:54:48 PDT
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].