Bug 223191 – Attempt to restore highlights after paint to account for newly loaded content.

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Attempt to restore highlights after paint to account for newly loaded content.

[Megan Gardner]

Created attachment 423193 [details]
Patch

[Tim Horton]

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 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 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 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]

Created attachment 423214 [details]
Patch

[Wenson Hsieh]

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]

Created attachment 423234 [details]
Patch

[Megan Gardner]

Created attachment 423266 [details]
Patch

[Megan Gardner]

rdar://74722200

[Megan Gardner]

Created attachment 423283 [details]
Patch

[Megan Gardner]

Created attachment 423291 [details]
Patch

[Tim Horton]

(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]

Created attachment 423302 [details]
Patch

[Tim Horton]

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]

Created attachment 423334 [details]
Patch

[Megan Gardner]

Created attachment 423361 [details]
Patch for landing

[EWS]

Committed r274501: <https://commits.webkit.org/r274501>

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