Bug 221802 – Change App Highlights API to operate in terms of a single serialized highlight at a time

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Update data path for App Highlights

[Megan Gardner]

Created attachment 420215 [details]
Patch

[Megan Gardner]

Created attachment 420322 [details]
Patch

[Megan Gardner]

Created attachment 420331 [details]
Patch

[Tim Horton]

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?

[Tim Horton]

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

[Alex Christensen]

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 *

[Megan Gardner]

Created attachment 420381 [details]
Patch

[Tim Horton]

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!

[Megan Gardner]

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

[Megan Gardner]

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.

[Megan Gardner]

Created attachment 420391 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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)

[Chris Dumez]

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.

[Megan Gardner]

Created attachment 420398 [details]
Patch for landing

[Chris Dumez]

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.

[Megan Gardner]

Sorry Chris, I didn't see your comments before! Should be addressed now.

[Megan Gardner]

Created attachment 420415 [details]
Patch

[Chris Dumez]

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.

[Megan Gardner]

Created attachment 420424 [details]
Patch

[Chris Dumez]

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());

[Megan Gardner]

Created attachment 420479 [details]
Patch

[Tim Horton]

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

[Megan Gardner]

Created attachment 420522 [details]
Patch

[Megan Gardner]

Created attachment 420524 [details]
Patch for landing

[EWS]

Committed r272925: <https://commits.webkit.org/r272925>

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

[Radar WebKit Bug Importer]

<rdar://problem/74405038>

[Devin Rousso]

unreviewed build fix r272978: <https://commits.webkit.org/r272978>