Bug 221802

Summary: Change App Highlights API to operate in terms of a single serialized highlight at a time
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, beidson, cdumez, ews-watchlist, gyuyoung.kim, hi, japhet, ryuan.choi, sergio, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Megan Gardner 2021-02-11 20:18:45 PST
Update data path for App Highlights
Comment 1 Megan Gardner 2021-02-12 23:22:39 PST
Created attachment 420215 [details]
Patch
Comment 2 Megan Gardner 2021-02-15 09:21:03 PST
Created attachment 420322 [details]
Patch
Comment 3 Megan Gardner 2021-02-15 10:34:55 PST
Created attachment 420331 [details]
Patch
Comment 4 Tim Horton 2021-02-15 11:38:18 PST
Comment on attachment 420331 [details]
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?
Comment 5 Tim Horton 2021-02-15 11:39:17 PST
Comment on attachment 420331 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Update data path for App Highlights

The title could use some help. Maybe "Change App Highlights API to operate in terms of a single serialized highlight at a time" or something
Comment 6 Alex Christensen 2021-02-15 13:32:51 PST
Comment on attachment 420331 [details]
Patch

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

> Source/WebCore/Headers.cmake:55
> +

Nit extra space.

> Source/WebCore/Modules/highlight/AppHighlightInformation.h:44
> +

Nit extra space

> Source/WebCore/Modules/highlight/AppHighlightInformation.h:81
> +    Optional<String> text;

I think you're encoding an extra bool here.  The Optional encoder already does this for you.
You can just encode text always, and then decode it like this:
Optional<Optional<String>> text;
decoder >> text;
if (!text)
return WTF::nullopt;

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:49
> +    auto decoder = buffer.decoder();
> +    Optional<AppHighlightRangeData> data;
> +    decoder >> data;
> +    return data;

This is a little strange.  It seems like it does double decoding and encoding.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7277
> +        document->appHighlightStorage().restoreAppHighlight(SharedBuffer::create(static_cast<const char *>(sharedMemory->data()), sharedMemory->size()));

Nit: extra space before *
Comment 7 Megan Gardner 2021-02-15 14:58:18 PST
Created attachment 420381 [details]
Patch
Comment 8 Tim Horton 2021-02-15 16:15:07 PST
Comment on attachment 420381 [details]
Patch

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

Please fix the title, still.

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

This should become "_webView:didCreateAppHighlight:" eventually.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2044
> +    for (NSData * highlight in highlights) {

Star is floating

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2046
> +        if (sharedMemory) {

Wonder if "just move on and ignore it" is the right thing here, or if we should bail entirely, or what?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:33
> +

Don't think this newline belongs here!
Comment 9 Megan Gardner 2021-02-15 16:21:29 PST
Comment on attachment 420331 [details]
Patch

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

>> Source/WebCore/Modules/highlight/AppHighlightInformation.h:81
>> +    Optional<String> text;
> 
> I think you're encoding an extra bool here.  The Optional encoder already does this for you.
> You can just encode text always, and then decode it like this:
> Optional<Optional<String>> text;
> decoder >> text;
> if (!text)
> return WTF::nullopt;

Oh, I definitely saw other places encoding optionals like this, so I don't know if this changed, or if other also didn't know this was doable.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1434
>> +        [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?

Yes, this patch doesn't address the delegate
Comment 10 Megan Gardner 2021-02-15 16:28:10 PST
Comment on attachment 420381 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1434
>> +        [delegate _webView:self updateAppHighlightsStorage:highlightInformation.highlight->createNSData().get()];
> 
> This should become "_webView:didCreateAppHighlight:" eventually.

Yes, this will be the next patch.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2046
>> +        if (sharedMemory) {
> 
> Wonder if "just move on and ignore it" is the right thing here, or if we should bail entirely, or what?

We can smooth out failure cases in the future.
Comment 11 Megan Gardner 2021-02-15 16:29:37 PST
Created attachment 420391 [details]
Patch
Comment 12 Chris Dumez 2021-02-15 16:42:57 PST
Comment on attachment 420391 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightInformation.h:55
> +    if (text)

I believe you want to drop this null check now that you're using the approach Alex suggested.
Comment 13 Chris Dumez 2021-02-15 16:43:47 PST
Comment on attachment 420391 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightInformation.h:90
> +    return {{ SharedBuffer::create(WTFMove(highlight)), WTFMove(text), isNewGroup }};

and I think this should be: WTFMove(*text)
Comment 14 Chris Dumez 2021-02-15 17:00:50 PST
Comment on attachment 420391 [details]
Patch

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

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

Why 2014?

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

If completely new: 2021, if modified: 2020-2021

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:64
> +    encoder << static_cast<uint64_t>(pathIndex);

I seem to remember it is completely fine to encode an unsigned type nowadays. I think used we do force uint64_t because we could have a 32bit vs 64bit mismatch between the WebProcess and the UIProcess.

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:96
> +    encoder << static_cast<uint64_t>(m_startOffset);

ditto.

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:98
> +    encoder << static_cast<uint64_t>(m_endOffset);

ditto.

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

same comment about date.

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:44
> +#define TEXT_PREVIEW_LENGTH 100

I believe we prefer to use static constexpr nowadays instead of #define.

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

Same comment about date.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:545
> +void WebPageProxy::restoreAppHighlights(Vector<Ref<SharedMemory>>& highlights)

can the parameter be const? I would either use `const Vector<Ref<SharedMemory>>&` or `Vector<Ref<SharedMemory>>&&`.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:554
> +        highlight->createHandle(handle, SharedMemory::Protection::ReadWrite);

I don't think the WebContent process really need Write access. Can probably be ReadOnly.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7271
> +void WebPage::restoreAppHighlights(const Vector<SharedMemory::IPCHandle> memoryHandles)

Vector<SharedMemory::IPCHandle>&&

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7276
> +        auto sharedMemory = SharedMemory::map(ipcHandle.handle, SharedMemory::Protection::ReadOnly);

Seems risky to not deal with sharedMemory being null.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1400
> +    void restoreAppHighlights(const Vector<SharedMemory::IPCHandle>);

Vector<SharedMemory::IPCHandle>&& since coming from IPC.
Comment 15 Megan Gardner 2021-02-15 17:11:07 PST
Created attachment 420398 [details]
Patch for landing
Comment 16 Chris Dumez 2021-02-15 17:27:55 PST
Comment on attachment 420398 [details]
Patch for landing

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

Please see review comments.

> Source/WebCore/Modules/highlight/AppHighlightInformation.h:55
> +    if (text)

This is still wrong.

> Source/WebCore/Modules/highlight/AppHighlightInformation.h:90
> +    return {{ SharedBuffer::create(WTFMove(highlight)), WTFMove(text), isNewGroup }};

Here too.
Comment 17 Megan Gardner 2021-02-15 18:30:48 PST
Sorry Chris, I didn't see your comments before! Should be addressed now.
Comment 18 Megan Gardner 2021-02-15 19:04:11 PST
Created attachment 420415 [details]
Patch
Comment 19 Chris Dumez 2021-02-15 20:37:24 PST
Comment on attachment 420415 [details]
Patch

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

I only commented on one decoder but please double check the other ones.

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:84
> +    Optional<uint64_t> pathIndex;

Watch out, the types need to match in the encoders and the decoders. You encoded an unsigned and decoding a uint64 here. This should use unsigned here to match.

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:89
> +    return {{ WTFMove(*identifier), WTFMove(*nodeName), WTFMove(*textData), static_cast<unsigned>(*pathIndex) }};

Then the cast here would no longer be needed.
Comment 20 Megan Gardner 2021-02-15 22:54:59 PST
Created attachment 420424 [details]
Patch
Comment 21 Chris Dumez 2021-02-16 08:34:06 PST
Comment on attachment 420424 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:89
> +    return {{ WTFMove(*identifier), WTFMove(*nodeName), WTFMove(*textData), (*pathIndex) }};

nit: (*pathIndex) -> *pathIndex

> Source/WebCore/Modules/highlight/AppHighlightRangeData.cpp:129
> +    return {{ WTFMove(*text), WTFMove(*startContainer), (*startOffset), WTFMove(*endContainer), (*endOffset) }};

ditto for startOffset, endOffset

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:45
> +

nit: unnecessary blank line.

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:234
> +    auto strongDocument = m_document.get();

What does strongDocument mean here? strongDocument is of type Document*. I would expect something called strongDocument to have a type such as RefPtr<Document> (where it actually ref's the type). Here, nothing keeps the document alive. Maybe you meant:
auto strongDocument = makeRefPtr(m_document.get());
Comment 22 Megan Gardner 2021-02-16 09:43:15 PST
Created attachment 420479 [details]
Patch
Comment 23 Tim Horton 2021-02-16 11:53:00 PST
Comment on attachment 420479 [details]
Patch

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

> Source/WebCore/Headers.cmake:50
> +    Modules/highlight/AppHighlightInformation.h

You could totally drop the "Information" without losing any "information" 🤷‍♂️

> Source/WebCore/page/ChromeClient.h:25
> +#include "AppHighlightInformation.h"

Please forward declare, this header is included eeeeeverywhere
Comment 24 Megan Gardner 2021-02-16 12:30:48 PST
Created attachment 420522 [details]
Patch
Comment 25 Megan Gardner 2021-02-16 12:36:23 PST
Created attachment 420524 [details]
Patch for landing
Comment 26 EWS 2021-02-16 13:44:36 PST
Committed r272925: <https://commits.webkit.org/r272925>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420524 [details].
Comment 27 Radar WebKit Bug Importer 2021-02-16 13:45:37 PST
<rdar://problem/74405038>
Comment 28 Devin Rousso 2021-02-17 09:22:23 PST
unreviewed build fix r272978: <https://commits.webkit.org/r272978>