Use DrawingRecorder::canUseCachedDrawing

Use DrawingRecorder::canUseCachedDrawing

If canUseCachedDrawing is true, the renderer is not invalidated and
the cached display item should be used. The callers check the flag,
and if it is true, omit all drawing operations for the renderer.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186997

[Xianzhu, 2014-12-11 19:49:35]

Patchset #1 (id:1) has been deleted

[Xianzhu, 2014-12-11 19:49:44]

Patchset #1 (id:20001) has been deleted

[Xianzhu, 2014-12-11 19:50:58]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[Xianzhu, 2014-12-11 19:50:59]

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/Render...
File Source/core/paint/RenderDrawingRecorder.h (right):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/Render...
Source/core/paint/RenderDrawingRecorder.h:18: class RenderDrawingRecorder :
public DrawingRecorder {
This is to inherit the canUseCachedDrawing method. Otherwise we would need to
delegate it.

[chrishtr, 2014-12-11 20:01:49]

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/BlockP...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/BlockP...
Source/core/paint/BlockPainter.cpp:92:
ScrollableAreaPainter(*m_renderBlock.layer()->scrollableArea()).paintOverflowControls(paintInfo.context,
roundedIntPoint(paintOffset), paintInfo.rect, false /* paintingOverlayControls
*/);
Why are these safe? Unless we are very careful, we have to emit cached display
items in the correct order, or paint order might be messed up.

[Xianzhu, 2014-12-11 20:22:16]

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/BlockP...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/BlockP...
Source/core/paint/BlockPainter.cpp:92:
ScrollableAreaPainter(*m_renderBlock.layer()->scrollableArea()).paintOverflowControls(paintInfo.context,
roundedIntPoint(paintOffset), paintInfo.rect, false /* paintingOverlayControls
*/);
On 2014/12/11 20:01:48, chrishtr wrote:
> Why are these safe? Unless we are very careful, we have to emit cached display
> items in the correct order, or paint order might be messed up.

I think we'll emit DrawingDisplayItems/CachedDisplayItems just like before in
the same order, while avoiding unnecessary paint operations on the context.

We have ensured that DrawingRecorders' life-times don't overlap, so we should
never emit DrawingDisplayItems/CachedDisplayItems within the block controlled by
'if (!recorder.canUseCachedDrawing())'.

[chrishtr, 2014-12-12 00:18:08]

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/BlockP...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/BlockP...
Source/core/paint/BlockPainter.cpp:92:
ScrollableAreaPainter(*m_renderBlock.layer()->scrollableArea()).paintOverflowControls(paintInfo.context,
roundedIntPoint(paintOffset), paintInfo.rect, false /* paintingOverlayControls
*/);
On 2014/12/11 at 20:22:16, Xianzhu wrote:
> On 2014/12/11 20:01:48, chrishtr wrote:
> > Why are these safe? Unless we are very careful, we have to emit cached
display
> > items in the correct order, or paint order might be messed up.
> 
> I think we'll emit DrawingDisplayItems/CachedDisplayItems just like before in
the same order, while avoiding unnecessary paint operations on the context.
> 
> We have ensured that DrawingRecorders' life-times don't overlap, so we should
never emit DrawingDisplayItems/CachedDisplayItems within the block controlled by
'if (!recorder.canUseCachedDrawing())'.

Oh right, it's within a block that has a DrawingRecorder. Good point. Nice.

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/ImageP...
File Source/core/paint/ImagePainter.cpp (left):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/ImageP...
Source/core/paint/ImagePainter.cpp:94: } else if (cWidth > 0 && cHeight > 0) {
Why the change to get rid of the else clause?

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/Render...
File Source/core/paint/RenderDrawingRecorder.h (right):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/Render...
Source/core/paint/RenderDrawingRecorder.h:18: class RenderDrawingRecorder :
public DrawingRecorder {
On 2014/12/11 at 19:50:59, Xianzhu wrote:
> This is to inherit the canUseCachedDrawing method. Otherwise we would need to
delegate it.

Let's delegate instead. That pattern is already in place, for the reason that
it's cleaner to reason about composition rather than inheritance,
especially regarding member variables.

[Xianzhu, 2014-12-12 00:56:49]

ptal.

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/ImageP...
File Source/core/paint/ImagePainter.cpp (left):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/ImageP...
Source/core/paint/ImagePainter.cpp:94: } else if (cWidth > 0 && cHeight > 0) {
On 2014/12/12 00:18:08, chrishtr wrote:
> Why the change to get rid of the else clause?

Restored.

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/Render...
File Source/core/paint/RenderDrawingRecorder.h (right):

https://codereview.chromium.org/799563002/diff/40001/Source/core/paint/Render...
Source/core/paint/RenderDrawingRecorder.h:18: class RenderDrawingRecorder :
public DrawingRecorder {
On 2014/12/12 00:18:08, chrishtr wrote:
> On 2014/12/11 at 19:50:59, Xianzhu wrote:
> > This is to inherit the canUseCachedDrawing method. Otherwise we would need
to
> delegate it.
> 
> Let's delegate instead. That pattern is already in place, for the reason that
> it's cleaner to reason about composition rather than inheritance,
> especially regarding member variables.

Done.

[chrishtr, 2014-12-12 00:58:00]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-12-12 00:58:00]

lgtm

[commit-bot: I haz the power, 2014-12-12 00:58:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799563002/60001

[commit-bot: I haz the power, 2014-12-12 00:58:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 00:58:33]

Failed to apply patch for Source/core/paint/BlockPainter.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/paint/BlockPainter.cpp
  Hunk #1 FAILED at 87.
  Hunk #2 succeeded at 177 (offset -1 lines).
  Hunk #3 succeeded at 226 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/paint/BlockPainter.cpp.rej

Patch:       Source/core/paint/BlockPainter.cpp
Index: Source/core/paint/BlockPainter.cpp
diff --git a/Source/core/paint/BlockPainter.cpp
b/Source/core/paint/BlockPainter.cpp
index
809f8ba106cf29a26706314efc9060c4a52080ff..68ae45ec6128cb035d44eb22ee84a9305c7a9a24
100644
--- a/Source/core/paint/BlockPainter.cpp
+++ b/Source/core/paint/BlockPainter.cpp
@@ -87,8 +87,9 @@ void BlockPainter::paintOverflowControlsIfNeeded(const
PaintInfo& paintInfo, con
 {
     PaintPhase phase = paintInfo.phase;
     if (m_renderBlock.hasOverflowClip() && m_renderBlock.style()->visibility()
== VISIBLE && (phase == PaintPhaseBlockBackground || phase ==
PaintPhaseChildBlockBackground) &&
paintInfo.shouldPaintWithinRoot(&m_renderBlock) &&
!paintInfo.paintRootBackgroundOnly()) {
-        RenderDrawingRecorder recorder(paintInfo.context, &m_renderBlock,
paintInfo.phase, pixelSnappedIntRect(paintOffset,
m_renderBlock.visualOverflowRect().size()));
-       
ScrollableAreaPainter(*m_renderBlock.layer()->scrollableArea()).paintOverflowControls(paintInfo.context,
roundedIntPoint(paintOffset), paintInfo.rect, false /* paintingOverlayControls
*/);
+        RenderDrawingRecorder recorder(paintInfo.context, m_renderBlock,
paintInfo.phase, pixelSnappedIntRect(paintOffset,
m_renderBlock.visualOverflowRect().size()));
+        if (!recorder.canUseCachedDrawing())
+           
ScrollableAreaPainter(*m_renderBlock.layer()->scrollableArea()).paintOverflowControls(paintInfo.context,
roundedIntPoint(paintOffset), paintInfo.rect, false /* paintingOverlayControls
*/);
     }
 }
 
@@ -178,14 +179,16 @@ void BlockPainter::paintObject(const PaintInfo& paintInfo,
const LayoutPoint& pa
     }
 
     if (paintPhase == PaintPhaseMask && m_renderBlock.style()->visibility() ==
VISIBLE) {
-        RenderDrawingRecorder recorder(paintInfo.context, &m_renderBlock,
paintPhase, bounds);
-        m_renderBlock.paintMask(paintInfo, paintOffset);
+        RenderDrawingRecorder recorder(paintInfo.context, m_renderBlock,
paintPhase, bounds);
+        if (!recorder.canUseCachedDrawing())
+            m_renderBlock.paintMask(paintInfo, paintOffset);
         return;
     }
 
     if (paintPhase == PaintPhaseClippingMask &&
m_renderBlock.style()->visibility() == VISIBLE) {
-        RenderDrawingRecorder recorder(paintInfo.context, &m_renderBlock,
paintPhase, bounds);
-        m_renderBlock.paintClippingMask(paintInfo, paintOffset);
+        RenderDrawingRecorder recorder(paintInfo.context, m_renderBlock,
paintPhase, bounds);
+        if (!recorder.canUseCachedDrawing())
+            m_renderBlock.paintClippingMask(paintInfo, paintOffset);
         return;
     }
 
@@ -225,8 +228,9 @@ void BlockPainter::paintObject(const PaintInfo& paintInfo,
const LayoutPoint& pa
     // If the caret's node's render object's containing block is this block,
and the paint action is PaintPhaseForeground,
     // then paint the caret.
     if (paintPhase == PaintPhaseForeground) {
-        RenderDrawingRecorder recorder(paintInfo.context, &m_renderBlock,
paintPhase, bounds);
-        paintCarets(paintInfo, paintOffset);
+        RenderDrawingRecorder recorder(paintInfo.context, m_renderBlock,
paintPhase, bounds);
+        if (!recorder.canUseCachedDrawing())
+            paintCarets(paintInfo, paintOffset);
     }
 }

[Xianzhu, 2014-12-12 01:04:36]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:05:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799563002/80001

[commit-bot: I haz the power, 2014-12-12 01:33:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:33:46]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41057)

[Xianzhu, 2014-12-12 02:44:36]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:45:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799563002/80001

[commit-bot: I haz the power, 2014-12-12 04:08:39]

Committed patchset #3 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186997