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

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has denied Megan Gardner
<megan_gardner at apple.com>'s request 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 #2 from Tim Horton <thorton 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:71
> +    Optional<AppHighlightRangeData> m_unrestoreScrollHighlight;

"unrestore"? unrestored, maybe, like the one above?

> Source/WebCore/page/Page.cpp:1594
> +#if ENABLE(APP_HIGHLIGHTS)

Newline above

> Source/WebCore/page/Page.cpp:1602
> +	   if (appHighlightStorage->hasUnrestoredHighlights() &&
MonotonicTime::now() - appHighlightStorage->lastRangeSearchTime() > 1_s) {

Why is all this code duplicated with that down below? Something is wrong with
the factoring here. (also maybe the logic? what's up with this one second
comparison here, after the one second timer?)

> Source/WebKit/UIProcess/WebPageProxy.h:1886
> +    void restoreAppHighlightsAndScrollToIndex(const
Vector<Ref<WebKit::SharedMemory>>& highlights, const int64_t index);

Why a signed index?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7498
> +	      
document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::cr
eate(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()),
ScrollToHighlight::Yes);

This will be a lot less duplicative if you shove the comparison into the last
argument:

document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::cr
eate(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), i
== index ? ScrollToHighlight::Yes : ScrollToHighlight::No);