[webkit-reviews] review denied: [Bug 198664] REGRESSION (r244182) [WK1]: Page updates should always scheduleCompositingLayerFlush() immediately : [Attachment 371594] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 198664: REGRESSION (r244182) [WK1]: Page updates should always
scheduleCompositingLayerFlush() immediately
https://bugs.webkit.org/show_bug.cgi?id=198664

Attachment 371594: Patch

https://bugs.webkit.org/attachment.cgi?id=371594&action=review




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 371594
  --> https://bugs.webkit.org/attachment.cgi?id=371594
Patch

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

> Source/WebCore/ChangeLog:22
> +	   --  RenderingUpdateScheduler::scheduleRenderingUpdate() is renamed
to 
> +	       scheduleCompositingLayerFlush() because this is what it does
when
> +	       the DispayRefresMonitor fires. Page::updateRendering() is called
> +	       from the flushLayers() methods.

I think this is going in the wrong direction. The fact that rendering updates
use composting is an implementation detail, so scheduleRenderingUpdate() is the
correct name to use.

> Source/WebCore/ChangeLog:24
> +	   --  RenderLayerCompositor::scheduleLayerFlush() will be calling 

will be calling -> now calls

> Source/WebCore/page/RenderingUpdateScheduler.h:49
> +    WEBCORE_EXPORT void scheduleCompositingLayerFlush();

Keep the old name.

> Source/WebCore/page/RenderingUpdateScheduler.h:50
> +    void scheduleCompositingLayerFlushImmediately();

Maybe this should be called scheduleImmediateRenderingUpdate? Or maybe
scheduleRenderingUpdate () should take an enum RenderingUpdateType { Immediate,
PerScreenUpdate }

> Source/WebCore/rendering/RenderLayerCompositor.cpp:469
> +	   page().chrome().client().scheduleCompositingLayerFlush();

Why does this not go through renderingUpdateScheduler?

> Tools/Tracing/SystemTracePoints.plist:-275
> -		    <string>Trigger rendering update</string>
> -		    <key>Type</key>
> -		    <string>Impulse</string>
> -		    <key>Component</key>
> -		    <string>47</string>
> -		    <key>Code</key>
> -		    <string>5029</string>
> -		</dict>
> -		<dict>
> -		    <key>Name</key>
> -		    <string>Rendering update</string>
> -		    <key>Type</key>
> -		    <string>Interval</string>
> -		    <key>Component</key>
> -		    <string>47</string>
> -		    <key>CodeBegin</key>
> -		    <string>5030</string>
> -		    <key>CodeEnd</key>
> -		    <string>5031</string>
> -		</dict>
> -		<dict>
> -		    <key>Name</key>
> -		    <string>Schedule rendering update</string>
> -		    <key>Type</key>
> -		    <string>Impulse</string>
> -		    <key>Component</key>
> -		    <string>47</string>
> -		    <key>Code</key>
> -		    <string>5028</string>
> -		</dict>
> -		<dict>
> -		    <key>Name</key>
> -		    <string>Trigger rendering update</string>

Don't remove keys. This breaks backwards compatibility.