Bug 221123 - Add plumbing to allow AppHighlights to be restored.
Summary: Add plumbing to allow AppHighlights to be restored.
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-01-28 23:56 PST by Megan Gardner
Modified: 2021-01-29 18:41 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.26 KB, patch)
2021-01-29 00:00 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (9.21 KB, patch)
2021-01-29 09:36 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (9.20 KB, patch)
2021-01-29 18:03 PST, 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-01-28 23:56:26 PST
Add plumbing to allow AppHighlights to be restored.
Comment 1 Megan Gardner 2021-01-29 00:00:06 PST
Created attachment 418710 [details]
Patch
Comment 2 Tim Horton 2021-01-29 00:07:37 PST
Comment on attachment 418710 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:255
> +    return;

You can just delete this now

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:366
> +- (void)_restoreAppHighlights:(NSData *)data WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I would put a newline between this and the family of methods above.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:547
> +    IPC::SharedBufferDataReference dataReference {data};

Spaces inside the { }

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:550
> +    if (!hasRunningProcess())
> +        return;

Seems like this should come first.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7191
> +void WebPage::restoreAppHighlights(const IPC::DataReference& data)

Newline before this.
Comment 3 Tim Horton 2021-01-29 00:09:28 PST
Comment on attachment 418710 [details]
Patch

I'm not sure all the SharedBuffer machinations are right, it would be good if someone who'd touched it recently could peek (things look slightly different than I remember).
Comment 4 Wenson Hsieh 2021-01-29 08:27:10 PST
Comment on attachment 418710 [details]
Patch

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

LGTM with Tim's comments.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2048
> +    _page->restoreAppHighlights(WTFMove(buffer));

Nit - I would just write this as `_page->restoreAppHighlights(WebCore::SharedBuffer::create(data));`.
Comment 5 Megan Gardner 2021-01-29 09:36:01 PST
Created attachment 418738 [details]
Patch
Comment 6 Megan Gardner 2021-01-29 18:03:22 PST
Created attachment 418796 [details]
Patch for landing
Comment 7 EWS 2021-01-29 18:39:29 PST
Committed r272096: <https://trac.webkit.org/changeset/272096>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418796 [details].
Comment 8 Radar WebKit Bug Importer 2021-01-29 18:41:29 PST
<rdar://problem/73781161>