[webkit-reviews] review requested: [Bug 224773] Support scrolling to a selected AppHighlight : [Attachment 426450] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has asked  for review:
Bug 224773: Support scrolling to a selected AppHighlight
https://bugs.webkit.org/show_bug.cgi?id=224773

Attachment 426450: Patch

https://bugs.webkit.org/attachment.cgi?id=426450&action=review




--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 426450
  --> https://bugs.webkit.org/attachment.cgi?id=426450
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:67
> +    bool attemptHighlightRestoreAndScroll(AppHighlightRangeData&,
ScrollToHighlight);

Nit - maybe `attemptToRestoreHighlightAndScroll`?

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:71
> +    Optional<AppHighlightRangeData> m_unrestoreScrollHighlight;

Nit - `m_unrestoredScrollHighlight`

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2184
> +- (void)_restoreAndScrollToAppHighlight:(NSData *)highlight
> +{
> +#if ENABLE(APP_HIGHLIGHTS)
> +    Vector<Ref<WebKit::SharedMemory>> buffers;
> +    
> +    auto sharedMemory = WebKit::SharedMemory::allocate(highlight.length);
> +    if (sharedMemory) {
> +	   [highlight getBytes:sharedMemory->data() length:highlight.length];
> +	   buffers.append(*sharedMemory);
> +    }
> +    _page->restoreAppHighlightsAndScrollToIndex(buffers, 0);

Nit - maybe some of this code can be shared with `-_restoreAppHighlights:` via
a static helper function?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7500
> +	   if (i == index)
> +	      
document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::cr
eate(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()),
ScrollToHighlight::Yes);
> +	   else
> +	      
document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::cr
eate(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()),
ScrollToHighlight::No);

Nit - I think you can avoid duplication here as well by making the
`ScrollToHighlight` argument a local variable.