Bug 235985 – Share more WebPage code related to marking layers volatile

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Simon Fraser (smfr)]

Share more WebPage code related to marking layers volatile

[Simon Fraser (smfr)]

Created attachment 450585 [details]
Patch

[Chris Dumez]

Comment on attachment 450585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450585&action=review

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2842
> +    WEBPAGE_RELEASE_LOG(Layers, "markLayersVolatile");

This seems to go against the usual style of our release logging?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2853
> +void WebPage::markLayersVolatileOrRetry(MarkLayersVolatileStopReason stopReason, Seconds timerInterval)

Why the second parameter? Can we just use initialLayerVolatilityTimerInterval inside the function like we used to?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2876
> +    WEBPAGE_RELEASE_LOG_ERROR(Layers, "markLayersVolatile: Failed to mark all layers as volatile, will retry in %g ms", timerInterval.milliseconds());

This was downgraded from error logging to regular logging a while back because this happens a lot and I am tired of getting radars about it.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1592
> +    enum class MarkLayersVolatileStopReason {

nit: Could be on one line.
Also: We should use uint8_t as underlying type.

Also, I find the name of this enum super confusing given that the function does something no matter what the stop reason is. I would prefer something like:
enum class ShouldRetryMarkingAsVolatile { NoDueToSuspensionUnderLock, NoDueToTimeout, Yes };

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 450585 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450585&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2842
> > +    WEBPAGE_RELEASE_LOG(Layers, "markLayersVolatile");
> 
> This seems to go against the usual style of our release logging?
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2853
> > +void WebPage::markLayersVolatileOrRetry(MarkLayersVolatileStopReason stopReason, Seconds timerInterval)
> 
> Why the second parameter? Can we just use
> initialLayerVolatilityTimerInterval inside the function like we used to?
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2876
> > +    WEBPAGE_RELEASE_LOG_ERROR(Layers, "markLayersVolatile: Failed to mark all layers as volatile, will retry in %g ms", timerInterval.milliseconds());
> 
> This was downgraded from error logging to regular logging a while back
> because this happens a lot and I am tired of getting radars about it.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:1592
> > +    enum class MarkLayersVolatileStopReason {
> 
> nit: Could be on one line.
> Also: We should use uint8_t as underlying type.
> 
> Also, I find the name of this enum super confusing given that the function
> does something no matter what the stop reason is. I would prefer something
> like:
> enum class ShouldRetryMarkingAsVolatile { NoDueToSuspensionUnderLock,
> NoDueToTimeout, Yes };

I meant:
enum class ShouldRetryMarkingAsVolatile : uint8_t { NoDueToSuspensionUnderLock, NoDueToTimeout, Yes };

[Simon Fraser (smfr)]

Comment on attachment 450585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450585&action=review

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2853
>> +void WebPage::markLayersVolatileOrRetry(MarkLayersVolatileStopReason stopReason, Seconds timerInterval)
> 
> Why the second parameter? Can we just use initialLayerVolatilityTimerInterval inside the function like we used to?

Because it's called from the timer callback with a new interval (2x the last one).

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2876
>> +    WEBPAGE_RELEASE_LOG_ERROR(Layers, "markLayersVolatile: Failed to mark all layers as volatile, will retry in %g ms", timerInterval.milliseconds());
> 
> This was downgraded from error logging to regular logging a while back because this happens a lot and I am tired of getting radars about it.

Will fix.

>> Source/WebKit/WebProcess/WebPage/WebPage.h:1592
>> +    enum class MarkLayersVolatileStopReason {
> 
> nit: Could be on one line.
> Also: We should use uint8_t as underlying type.
> 
> Also, I find the name of this enum super confusing given that the function does something no matter what the stop reason is. I would prefer something like:
> enum class ShouldRetryMarkingAsVolatile { NoDueToSuspensionUnderLock, NoDueToTimeout, Yes };

Can do.

[Chris Dumez]

Comment on attachment 450585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450585&action=review

>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2853
>>> +void WebPage::markLayersVolatileOrRetry(MarkLayersVolatileStopReason stopReason, Seconds timerInterval)
>> 
>> Why the second parameter? Can we just use initialLayerVolatilityTimerInterval inside the function like we used to?
> 
> Because it's called from the timer callback with a new interval (2x the last one).

Oh, I see it now (newInterval). Sorry.

[Simon Fraser (smfr)]

The timers should probably be startOneShot(), since they only fire once before reschedule.

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #6)
> The timers should probably be startOneShot(), since they only fire once
> before reschedule.

Yes, the startRepeating() is confusing here too.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/288940/webkit

[Radar WebKit Bug Importer]

<rdar://problem/88362884>