Bug 224773 – Support scrolling to a selected AppHighlight

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

[Megan Gardner]

Support scrolling to a selected AppHighlight

[Megan Gardner]

Created attachment 426450 [details]
Patch

[Tim Horton]

Comment on attachment 426450 [details]
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::create(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::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), i == index ? ScrollToHighlight::Yes : ScrollToHighlight::No);

[Wenson Hsieh]

Comment on attachment 426450 [details]
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::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), ScrollToHighlight::Yes);
> +        else
> +            document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(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.

[Wenson Hsieh]

Comment on attachment 426450 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:266
> +        TemporarySelectionChange selectionChange(*strongDocument, { range.value() }, temporarySelectionOptions);

Does this option work in the case where there was already a selection in the DOM prior to highlight restoration?

[Megan Gardner]

Created attachment 426498 [details]
Patch

[Tim Horton]

Comment on attachment 426498 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:29
> +

No newline here.

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

Oof

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
> +static void restoreHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)

This does no restoration, just does type conversion and adds it to a vector.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7492
> +    unsigned i = 0;

I think this type and the inner type of index should match, though I don't know which is right.

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

Multiple review comments about this code above were not applied. Intentional?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:95
> +    [webViewCreate synchronouslyLoadHTMLString:@"<br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br>Test"];

<div style='height: 1000px'></div>? Or just use lots-of-text.html like all the other tests.

[Wenson Hsieh]

Comment on attachment 426498 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
>> +static void restoreHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)
> 
> This does no restoration, just does type conversion and adds it to a vector.

Also, minor nit - `&` and `*` are on the wrong sides. Perhaps a better helper method would be something along the lines of:

static Vector<Ref<WebKit::SharedMemory>> copyHighlightDataToSharedMemory(NSArray<NSData *> *highlights)
{
    …
}

[Megan Gardner]

Created attachment 426621 [details]
Patch

[Megan Gardner]

Created attachment 426625 [details]
Patch

[Wenson Hsieh]

Comment on attachment 426625 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);

Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".

[Megan Gardner]

Comment on attachment 426625 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
>> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);
> 
> Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".

I used to have a bunch of <br>s but Tim said to just have a tall div instead.

[Tim Horton]

(In reply to Megan Gardner from comment #11)
> Comment on attachment 426625 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426625&action=review
> 
> >> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
> >> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);
> > 
> > Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".
> 
> I used to have a bunch of <br>s but Tim said to just have a tall div instead.

I did, but:

1) you put the text inside the div; I think you wanted to put it after, and 
2) your "restore" webview only has this: [webViewRestore synchronouslyLoadHTMLString:@"Test"];

[Wenson Hsieh]

Comment on attachment 426625 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
>>> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);
>> 
>> Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".
> 
> I used to have a bunch of <br>s but Tim said to just have a tall div instead.

I do agree that having a tall div element is a bit cleaner than lots of br elements. But that was on the WKWebView where we created the highlight, right? When restoring the highlight, it looks like we're restoring the highlight to an unscrollable web page…(I suppose it's not really clear to me why we expect `pageYOffset != 0` after restoration).

Also, it looks like the test is calling `-_restoreAppHighlights:` instead of `-_restoreAndScrollToAppHighlight:`. Does the former also trigger scrolling, or is it just the latter?

[Megan Gardner]

Created attachment 426640 [details]
Patch

[Wenson Hsieh]

Comment on attachment 426640 [details]
Patch

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

Looking much better! A couple of minor comments.

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:63
>      bool hasUnrestoredHighlights() const { return m_unrestoredHighlights.size(); }

Do we need to update this implementation to `return m_unrestoredScrollHighlight || m_unrestoredHighlights.size();`? It looks like in the case where we have an unrestored highlight that we need to scroll to, the highlight won't end up being stored in `m_unrestoredHighlights`, so this will return `false`.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
> +static void convertAndAddHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)

Nit - the `&` and `*` are still on the wrong side (https://bugs.webkit.org/show_bug.cgi?id=224773#c7).

[Tim Horton]

Comment on attachment 426640 [details]
Patch

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

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

Newline below here, before the members.

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

Should make sure that there's a test that covers this code (maybe in another patch)

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
> +static void convertAndAddHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)

star's on the wrong side

[Megan Gardner]

Created attachment 426647 [details]
Patch

[Tim Horton]

Comment on attachment 426647 [details]
Patch

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

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

still missing this newline

[Megan Gardner]

Created attachment 426649 [details]
Patch for landing

[EWS]

Committed r276347 (236825@main): <https://commits.webkit.org/236825@main>

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

[Megan Gardner]

rdar://76166593