Move painting code from RenderBoxModelObject into BoxPainter.

Move painting code from RenderBoxModelObject into BoxPainter.

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

[chrishtr, 2014-09-10 21:56:31]

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

[chrishtr, 2014-09-10 21:56:33]

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:635: bool
getBackgroundPaintedExtent(LayoutRect&);
This is fallout from BoxPainter::m_renderBox not yet being const.

[pdr., 2014-09-11 05:59:13]

This looks great. LGTM with a nit and a comment.

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1108:
BoxPainter(*this).calculateBackgroundImageGeometry(0,
style()->backgroundLayers(), backgroundRect, geometry);
I only saw that this changed after BoxPainter landed, but this pattern will end
up being slower than a static calculateBackgroundImageGeometry that passes in
the RenderBox reference.

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:635: bool
getBackgroundPaintedExtent(LayoutRect&);
On 2014/09/10 21:56:32, chrishtr wrote:
> This is fallout from BoxPainter::m_renderBox not yet being const.

Could we add a fixme here so we don't forget to fix it up?

[chrishtr, 2014-09-11 17:45:53]

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1108:
BoxPainter(*this).calculateBackgroundImageGeometry(0,
style()->backgroundLayers(), backgroundRect, geometry);
On 2014/09/11 at 05:59:13, pdr wrote:
> I only saw that this changed after BoxPainter landed, but this pattern will
end up being slower than a static calculateBackgroundImageGeometry that passes
in the RenderBox reference.

Ack.

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/559733005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:635: bool
getBackgroundPaintedExtent(LayoutRect&);
On 2014/09/11 at 05:59:13, pdr wrote:
> On 2014/09/10 21:56:32, chrishtr wrote:
> > This is fallout from BoxPainter::m_renderBox not yet being const.
> 
> Could we add a fixme here so we don't forget to fix it up?

Done.

[pdr., 2014-09-11 17:47:04]

On 2014/09/11 17:45:53, chrishtr wrote:
> > Could we add a fixme here so we don't forget to fix it up?
> 
> Done.

LGTM2

[chrishtr, 2014-09-11 18:07:53]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:08:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559733005/40001

[commit-bot: I haz the power, 2014-09-11 18:54:58]

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

[commit-bot: I haz the power, 2014-09-11 18:55:00]

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

[chrishtr, 2014-09-11 20:38:30]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:39:30]

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

[commit-bot: I haz the power, 2014-09-11 21:19:07]

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

[commit-bot: I haz the power, 2014-09-11 21:19:08]

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

[chrishtr, 2014-09-11 23:25:44]

I ran into a bug, which was that InlineFlowBox's renderer inherits directly from
RenderBoxModelObject. I had hoped that the code did not require differentiating
from RenderBox, but it does. I made various things static to able to pass around
a RenderBoxModelObject, and also addded back in some toRenderBox conversions
that were in the old code.

[pdr., 2014-09-11 23:28:06]

https://codereview.chromium.org/559733005/diff/100001/Source/core/paint/BoxPa...
File Source/core/paint/BoxPainter.h (right):

https://codereview.chromium.org/559733005/diff/100001/Source/core/paint/BoxPa...
Source/core/paint/BoxPainter.h:28: static void
paintFillLayerExtended(RenderBoxModelObject&, const PaintInfo&, const Color&,
const FillLayer&, const LayoutRect&, BackgroundBleedAvoidance, InlineFlowBox* =
0, const LayoutSize& = LayoutSize(), CompositeOperator = CompositeSourceOver,
RenderObject* backgroundObject = 0, bool skipBaseColor = false);
Should all of these be const?

[pdr., 2014-09-11 23:38:11]

Explicit LGTM

[chrishtr, 2014-09-11 23:40:19]

https://codereview.chromium.org/559733005/diff/100001/Source/core/paint/BoxPa...
File Source/core/paint/BoxPainter.h (right):

https://codereview.chromium.org/559733005/diff/100001/Source/core/paint/BoxPa...
Source/core/paint/BoxPainter.h:28: static void
paintFillLayerExtended(RenderBoxModelObject&, const PaintInfo&, const Color&,
const FillLayer&, const LayoutRect&, BackgroundBleedAvoidance, InlineFlowBox* =
0, const LayoutSize& = LayoutSize(), CompositeOperator = CompositeSourceOver,
RenderObject* backgroundObject = 0, bool skipBaseColor = false);
On 2014/09/11 23:28:06, pdr wrote:
> Should all of these be const?

I was able to make one of them const.

[chrishtr, 2014-09-11 23:40:55]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-09-11 23:41:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559733005/120001

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

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

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

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

[chrishtr, 2014-09-12 04:20:36]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-09-12 04:21:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559733005/140001

[commit-bot: I haz the power, 2014-09-12 04:21:17]

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

[commit-bot: I haz the power, 2014-09-12 04:21:18]

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

Patch:       Source/core/rendering/RenderImage.cpp
Index: Source/core/rendering/RenderImage.cpp
diff --git a/Source/core/rendering/RenderImage.cpp
b/Source/core/rendering/RenderImage.cpp
index
2a9caff178da4b8c2b7737bfc0cc0ceacc8abefd..087f844fe419dfcfb66bf54bee10299dade610bd
100644
--- a/Source/core/rendering/RenderImage.cpp
+++ b/Source/core/rendering/RenderImage.cpp
@@ -40,6 +40,7 @@
 #include "core/html/HTMLMapElement.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/inspector/InspectorTraceEvents.h"
+#include "core/paint/BoxPainter.h"
 #include "core/rendering/HitTestResult.h"
 #include "core/rendering/PaintInfo.h"
 #include "core/rendering/RenderLayer.h"
@@ -450,7 +451,7 @@ void RenderImage::paintIntoRect(GraphicsContext* context,
const LayoutRect& rect
     HTMLImageElement* imageElt = isHTMLImageElement(node()) ?
toHTMLImageElement(node()) : 0;
     CompositeOperator compositeOperator = imageElt ?
imageElt->compositeOperator() : CompositeSourceOver;
     Image* image = img.get();
-    InterpolationQuality interpolationQuality =
chooseInterpolationQuality(context, image, image, alignedRect.size());
+    InterpolationQuality interpolationQuality =
BoxPainter::chooseInterpolationQuality(*this, context, image, image,
alignedRect.size());
 
     TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"), "PaintImage",
"data", InspectorPaintImageEvent::data(*this));
     // FIXME(361045): remove InspectorInstrumentation calls once DevTools
Timeline migrates to tracing.

[chrishtr, 2014-09-12 04:30:25]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-09-12 04:31:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559733005/160001

[commit-bot: I haz the power, 2014-09-12 05:40:23]

Committed patchset #9 (id:160001) as 181880