WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.64 KB, patch)
2021-03-15 12:13 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(4.67 KB, patch)
2021-03-15 14:00 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(4.82 KB, patch)
2021-03-15 17:13 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(5.03 KB, patch)
2021-03-15 19:26 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(4.82 KB, patch)
2021-03-15 20:32 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(4.98 KB, patch)
2021-03-15 22:53 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(4.96 KB, patch)
2021-03-16 08:40 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch for landing
(4.95 KB, patch)
2021-03-16 11:12 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2021-03-15 10:17:41 PDT
Created
attachment 423193
[details]
Patch
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
Created
attachment 423214
[details]
Patch
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
Created
attachment 423234
[details]
Patch
Megan Gardner
Comment 9
2021-03-15 17:13:34 PDT
Created
attachment 423266
[details]
Patch
Megan Gardner
Comment 10
2021-03-15 17:14:00 PDT
rdar://74722200
Megan Gardner
Comment 11
2021-03-15 19:26:25 PDT
Created
attachment 423283
[details]
Patch
Megan Gardner
Comment 12
2021-03-15 20:32:13 PDT
Created
attachment 423291
[details]
Patch
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
Created
attachment 423302
[details]
Patch
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
Created
attachment 423334
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug