Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(284)

Issue 550363004: Factor painting code out of RenderBox into a new class called BoxPainter. (Closed)

Created:
6 years, 3 months ago by chrishtr
Modified:
6 years, 3 months ago
Reviewers:
esprehn, slimming-paint-reviews, eseidel, pdr., Stephen Chennney, ojan
CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1
Project:
blink
Visibility:
Public.

Description

Factor painting code out of RenderBox into a new class called BoxPainter. Along the way, create BoxDecorationData and BorderEdge classes, to represent classes that used to be internal to the RenderBox and subclass implementations. Also moves a couple of methods to RenderStyle that use only style data. This CL increases the size of RenderBox, RenderTable, Render TableCell and RenderFieldset objects by one pointer, to accomodate a back pointer from the BoxPainter to the Render object. BUG=412088 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181742

Patch Set 1 #

Patch Set 2 : Fix. #

Patch Set 3 : Fix #

Patch Set 4 : Fix #

Total comments: 16

Patch Set 5 : Addressed comments. #

Patch Set 6 : Fix debug build. #

Total comments: 14

Patch Set 7 : Return false if no border. #

Patch Set 8 : Addressed comments from pdr. #

Patch Set 9 : Fixed compile. #

Patch Set 10 : Fixed compile. #

Patch Set 11 : Fixed build #

Patch Set 12 : Fix #

Patch Set 13 : fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+629 lines, -440 lines) Patch
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -0 lines 0 comments Download
A Source/core/paint/BoxDecorationData.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +35 lines, -0 lines 0 comments Download
A Source/core/paint/BoxDecorationData.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +73 lines, -0 lines 0 comments Download
A Source/core/paint/BoxPainter.h View 1 1 chunk +40 lines, -0 lines 0 comments Download
A Source/core/paint/BoxPainter.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +241 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBox.h View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +3 lines, -25 lines 0 comments Download
M Source/core/rendering/RenderBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +5 lines, -231 lines 0 comments Download
M Source/core/rendering/RenderBoxModelObject.h View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +11 lines, -13 lines 0 comments Download
M Source/core/rendering/RenderBoxModelObject.cpp View 1 2 3 4 5 6 7 8 17 chunks +20 lines, -161 lines 0 comments Download
M Source/core/rendering/RenderFieldset.cpp View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -4 lines 0 comments Download
M Source/core/rendering/RenderImage.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/rendering/RenderObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/rendering/RenderTable.cpp View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +3 lines, -2 lines 0 comments Download
M Source/core/rendering/RenderTableCell.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -2 lines 0 comments Download
A Source/core/rendering/style/BorderEdge.h View 1 2 3 4 5 6 7 8 1 chunk +41 lines, -0 lines 0 comments Download
A Source/core/rendering/style/BorderEdge.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +84 lines, -0 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +51 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (3 generated)
chrishtr
Spamming a lot of reviewers to make sure this pattern is Kosher.
6 years, 3 months ago (2014-09-08 22:17:00 UTC) #2
chrishtr
+slimming-paint-reviews
6 years, 3 months ago (2014-09-08 22:18:47 UTC) #4
Stephen Chennney
I'm firing up the discussion on how we implement this. I didn't carefully review every ...
6 years, 3 months ago (2014-09-09 16:14:38 UTC) #5
chrishtr
https://codereview.chromium.org/550363004/diff/60001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/550363004/diff/60001/Source/core/core.gypi#newcode1499 Source/core/core.gypi:1499: 'rendering/BoxDecorationData.cpp', On 2014/09/09 at 16:14:37, Stephen Chenney wrote: > ...
6 years, 3 months ago (2014-09-09 16:55:45 UTC) #6
Stephen Chennney
Just one nit for the code. But I think we do need to check for ...
6 years, 3 months ago (2014-09-09 19:16:48 UTC) #7
chrishtr
https://codereview.chromium.org/550363004/diff/100001/Source/core/rendering/RenderBoxModelObject.h File Source/core/rendering/RenderBoxModelObject.h (right): https://codereview.chromium.org/550363004/diff/100001/Source/core/rendering/RenderBoxModelObject.h#newcode266 Source/core/rendering/RenderBoxModelObject.h:266: bool isDocumentElementWithOpaqueBackground() const; On 2014/09/09 19:16:48, Stephen Chenney wrote: ...
6 years, 3 months ago (2014-09-09 19:50:35 UTC) #8
pdr.
https://codereview.chromium.org/550363004/diff/100001/Source/core/paint/BoxDecorationData.h File Source/core/paint/BoxDecorationData.h (right): https://codereview.chromium.org/550363004/diff/100001/Source/core/paint/BoxDecorationData.h#newcode17 Source/core/paint/BoxDecorationData.h:17: class BoxDecorationData { I feel like this is more ...
6 years, 3 months ago (2014-09-09 20:45:42 UTC) #9
chrishtr
Perf results: I ran ./tools/perf/run_benchmark rasterize_and_record_micro.fast_path_gpu_rasterization.key_silk_cases with and without the patch. Record time was unchanged ...
6 years, 3 months ago (2014-09-09 22:38:18 UTC) #10
esprehn
Why can't we create the painters on the stack? I'm not a big fan of ...
6 years, 3 months ago (2014-09-09 22:52:18 UTC) #11
chrishtr
On 2014/09/09 22:52:18, esprehn wrote: > Why can't we create the painters on the stack? ...
6 years, 3 months ago (2014-09-09 23:31:42 UTC) #12
Stephen Chennney
Thanks to everyone for all the feedback and to Chris for the perf results. I'm ...
6 years, 3 months ago (2014-09-10 13:23:55 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/550363004/240001
6 years, 3 months ago (2014-09-10 16:25:48 UTC) #15
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 16:29:22 UTC) #16
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as 181742

Powered by Google App Engine
This is Rietveld 408576698