RESOLVED FIXED 223191
Attempt to restore highlights after paint to account for newly loaded content.
https://bugs.webkit.org/show_bug.cgi?id=223191
Summary Attempt to restore highlights after paint to account for newly loaded content.
Megan Gardner
Reported 2021-03-15 10:14:16 PDT
Attempt to restore highlights after paint to account for newly loaded content.
Attachments
Patch (4.13 KB, patch)
2021-03-15 10:17 PDT, Megan Gardner
no flags
Patch (4.64 KB, patch)
2021-03-15 12:13 PDT, Megan Gardner
no flags
Patch (4.67 KB, patch)
2021-03-15 14:00 PDT, Megan Gardner
no flags
Patch (4.82 KB, patch)
2021-03-15 17:13 PDT, Megan Gardner
no flags
Patch (5.03 KB, patch)
2021-03-15 19:26 PDT, Megan Gardner
no flags
Patch (4.82 KB, patch)
2021-03-15 20:32 PDT, Megan Gardner
no flags
Patch (4.98 KB, patch)
2021-03-15 22:53 PDT, Megan Gardner
no flags
Patch (4.96 KB, patch)
2021-03-16 08:40 PDT, Megan Gardner
no flags
Patch for landing (4.95 KB, patch)
2021-03-16 11:12 PDT, Megan Gardner
no flags
Megan Gardner
Comment 1 2021-03-15 10:17:41 PDT
Tim Horton
Comment 2 2021-03-15 10:28:35 PDT
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.
Tim Horton
Comment 3 2021-03-15 10:30:03 PDT
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...
Tim Horton
Comment 4 2021-03-15 10:30:41 PDT
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.
Wenson Hsieh
Comment 5 2021-03-15 10:57:37 PDT
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.
Megan Gardner
Comment 6 2021-03-15 12:13:20 PDT
Wenson Hsieh
Comment 7 2021-03-15 12:22:07 PDT
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.
Megan Gardner
Comment 8 2021-03-15 14:00:35 PDT
Megan Gardner
Comment 9 2021-03-15 17:13:34 PDT
Megan Gardner
Comment 10 2021-03-15 17:14:00 PDT
Megan Gardner
Comment 11 2021-03-15 19:26:25 PDT
Megan Gardner
Comment 12 2021-03-15 20:32:13 PDT
Tim Horton
Comment 13 2021-03-15 20:42:16 PDT
(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.
Megan Gardner
Comment 14 2021-03-15 22:53:07 PDT
Tim Horton
Comment 15 2021-03-15 23:05:53 PDT
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
Megan Gardner
Comment 16 2021-03-16 08:40:15 PDT
Megan Gardner
Comment 17 2021-03-16 11:12:07 PDT
Created attachment 423361 [details] Patch for landing
EWS
Comment 18 2021-03-16 11:44:45 PDT
Committed r274501: <https://commits.webkit.org/r274501> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423361 [details].
Note You need to log in before you can comment on or make changes to this bug.