Attempt to restore highlights after paint to account for newly loaded content.
Created attachment 423193 [details] Patch
Comment on attachment 423193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423193&action=review > Source/WebCore/page/Page.cpp:1614 > + if (MonotonicTime() - document.appHighlightStorage().lastRangeSearchTime() > 1_s) { Extra space before > Isn't this going to create the appHighlightStorage if it doesn't exist? Maybe you want to use "IfExists"? > Source/WebCore/page/Page.cpp:1615 > + document.eventLoop().queueTask(TaskSource::InternalAsyncTask, [weakDocument = makeWeakPtr(document)] { Probably should not even bother enqueueing the task if you don't have any remaining outstanding highlights.
Comment on attachment 423193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423193&action=review > Source/WebCore/page/Page.cpp:1621 > + appHighlightStorage->restoreUnrestoredAppHighlights(); It seems like you're going to queue up a ton of these when you paint quickly, right?? Nothing ever sets m_timeAtLastRangeSearch...
Comment on attachment 423193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423193&action=review >> Source/WebCore/page/Page.cpp:1621 >> + appHighlightStorage->restoreUnrestoredAppHighlights(); > > It seems like you're going to queue up a ton of these when you paint quickly, right?? Nothing ever sets m_timeAtLastRangeSearch... ... until you actually run this, but in the meantime you could have queued up hundreds.
Comment on attachment 423193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423193&action=review > Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:268 > + m_timeAtLastRangeSearch = MonotonicTime(); Did you mean to use `MonotonicTime::now()`? From a glance, it seems like the time offset computed inside the if statement below (`MonotonicTime() - document.appHighlightStorage().lastRangeSearchTime()`) will always be 0_s. > Source/WebCore/Modules/highlight/AppHighlightStorage.h:59 > + MonotonicTime lastRangeSearchTime() { return m_timeAtLastRangeSearch; }; Nit - lastRangeSearchTime() should be marked `const`. You can remove the extra ";" at the end of the method definition.
Created attachment 423214 [details] Patch
Comment on attachment 423214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423214&action=review > Source/WebCore/page/Page.cpp:1615 > + if (MonotonicTime() - appHighlightStorage->lastRangeSearchTime() > 1_s) { This looks like it's still subtracting the last range search time from 1/1/1970.
Created attachment 423234 [details] Patch
Created attachment 423266 [details] Patch
rdar://74722200
Created attachment 423283 [details] Patch
Created attachment 423291 [details] Patch
(In reply to Tim Horton from comment #4) > Comment on attachment 423193 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423193&action=review > > >> Source/WebCore/page/Page.cpp:1621 > >> + appHighlightStorage->restoreUnrestoredAppHighlights(); > > > > It seems like you're going to queue up a ton of these when you paint quickly, right?? Nothing ever sets m_timeAtLastRangeSearch... > > ... until you actually run this, but in the meantime you could have queued > up hundreds. I don't think you've fixed this.
Created attachment 423302 [details] Patch
Comment on attachment 423302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423302&action=review > Source/WebCore/page/Page.cpp:1614 > + if (auto appHighlightStorage = document.appHighlightStorageIfExists()) { This could just be an early return, since you're in a lambda
Created attachment 423334 [details] Patch
Created attachment 423361 [details] Patch for landing
Committed r274501: <https://commits.webkit.org/r274501> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423361 [details].