Support scrolling to a selected AppHighlight
Created attachment 426450 [details] Patch
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);
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.
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?
Created attachment 426498 [details] Patch
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.
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) { … }
Created attachment 426621 [details] Patch
Created attachment 426625 [details] Patch
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".
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.
(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"];
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?
Created attachment 426640 [details] Patch
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).
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
Created attachment 426647 [details] Patch
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
Created attachment 426649 [details] Patch for landing
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].
rdar://76166593