Bug 221020 – Add plumbing to allow AppHighlights to be stored.

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

[Megan Gardner]

Add plumbing to allow AppHighlights to be stored.

[Megan Gardner]

Created attachment 418494 [details]
Patch

[Tim Horton]

Comment on attachment 418494 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:212
> +void AppHighlightStorage::createAndSendAppHighlightsStorage()

"send" is weird. Also, should this all really be called "update"? Does it get called whenever things change?

> Source/WebCore/page/ChromeClient.h:306
> +    virtual void didCreateHighlightsStorageData(Ref<WebCore::SharedBuffer>&&) const = 0;

Ditto all the way through wondering if "didCreate" is the right verb

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1406
> +#if ENABLE(APP_HIGHLIGHT)

It's gotta be APP_HIGHLIGHT or APP_HIGHLIGHTS, can't be both. Did you test this?

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlightStorageDelegate.h:28
> +@protocol _WKAppHighlightStorageDelegate <NSObject>

If you're going to add a new delegate, I would be /bold/ and drop the "storage"

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlightStorageDelegate.h:32
> +- (void)_webView:(WKWebView *)webView didCreateHighlightsStorageData:(NSData *)data;

Ditto with the name

> Source/WebKit/UIProcess/WebPageProxy.messages.in:539
> +    DidCreateHighlightsStorageData(IPC::SharedBufferCopy data) // <MMG> SharedBufferDataReference instead??

<MMG>

[Megan Gardner]

Created attachment 418504 [details]
Patch

[Megan Gardner]

Created attachment 418558 [details]
Patch

[Megan Gardner]

Created attachment 418566 [details]
Patch

[Wenson Hsieh]

Comment on attachment 418566 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:99
> +    RetainPtr<NSData> nsData = imageBuffer->createNSData();

s/imageBuffer/data/ here? Also, do we need a .get() below?

[Devin Rousso]

Comment on attachment 418566 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:228
> +    m_document->page()->chrome().updateAppHighlightsStorage(listData.toSharedBuffer());

NIT: should we null-check `m_document->page()`?  I think currently it's assumed to exist since this is only called from `WebPage::createAppHighlightInSelectedRange` which also assumes `m_page`.  Is it possible for anything in the loop to disconnect the document?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1954
> +- (id <_WKAppHighlightDelegate>)_appHighlightsDelegate

Do we need an `#import` for this?  Also, should this be a private/internal `@property`?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1956
> +    return _appHighlightsDelegate.getAutoreleased();

Do we need an `_appHighlightsDelegate` ivar?

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlightStorageDelegate.h:28
> +@protocol _WKAppHighlightDelegate <NSObject>

This should match the filename (or vice versa).

>> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:99
>> +    RetainPtr<NSData> nsData = imageBuffer->createNSData();
> 
> s/imageBuffer/data/ here? Also, do we need a .get() below?

NIT: also `auto`?

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1244
> +    m_page.send(Messages::WebPageProxy::UpdateAppHighlightsStorage(data));

Should this be `WTFMove(data)`?

[Megan Gardner]

Created attachment 418621 [details]
Patch

[Megan Gardner]

Created attachment 418625 [details]
Patch

[Megan Gardner]

Created attachment 418654 [details]
Patch

[Devin Rousso]

Comment on attachment 418654 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebPageProxy.cpp:9924
> +    pageClient().updateAppHighlightsStorage(*data.buffer());

NIT: should we check `data.buffer()` before dereferencing it (especially since it's a message from the WebProcess)?

[Chris Dumez]

Comment on attachment 418654 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:9924
>> +    pageClient().updateAppHighlightsStorage(*data.buffer());
> 
> NIT: should we check `data.buffer()` before dereferencing it (especially since it's a message from the WebProcess)?

Indeed, you likely want a MESSAGE_CHECK(data.buffer()); at the beginning of this function.

[Megan Gardner]

Created attachment 418686 [details]
Patch for landing

[EWS]

Committed r272042: <https://trac.webkit.org/changeset/272042>

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

[Radar WebKit Bug Importer]

<rdar://problem/73734447>