Share more WebPage code related to marking layers volatile
Created attachment 450585 [details] Patch
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 };
(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 };
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.
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.
The timers should probably be startOneShot(), since they only fire once before reschedule.
(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.
https://trac.webkit.org/changeset/288940/webkit
<rdar://problem/88362884>