Add plumbing to allow AppHighlights to be stored.
Created attachment 418494 [details] Patch
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>
Created attachment 418504 [details] Patch
Created attachment 418558 [details] Patch
Created attachment 418566 [details] Patch
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?
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)`?
Created attachment 418621 [details] Patch
Created attachment 418625 [details] Patch
Created attachment 418654 [details] Patch
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)?
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.
Created attachment 418686 [details] Patch for landing
Committed r272042: <https://trac.webkit.org/changeset/272042> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418686 [details].
<rdar://problem/73734447>