Issue 1145993002: Refactor root element background painting

Issue 1145993002: Refactor root element background painting (Closed)

Created:
4 years, 11 months ago by trchen

Modified:
4 years, 10 months ago

Reviewers:
chrishtr, Xianzhu

CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-rendering, blink-reviews-style_chromium.org, dglazkov+blink, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, sof, slimming-paint-reviews_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Refactor root element background painting Base background is the default color on the canvas when the document is transparent. Logically it should be the embedder to paint the base background, but instead we make Blink to paint it to perform culling optimization and pre-blending with the root background color to reduce pixel count. Before this CL, the <html> element is responsible for painting root background and base background. This is incorrect, because in CSS, the root background should be painted to the whole canvas, and technically the root background doesn't belong to the root stacking context. This CL does the following improvement: 1. Background for transformed root element now correctly expands to the whole canvas. This also fixed the issue with fixed-positioned root element because fixed-position blocks are now painted using transformation code path. 2. The root element no longer paints background. Hacks that involved root element special rules are no longer needed. Now the LayoutView paints both base and root background, and the invalidation is done by normal style update. 3. Correct the style invalidation with <body> background. This involves another CSS quirk that <html> with no background should steal <body>'s background. 4. Frames no longer incorrectly paint base background. The main frame which owns the frame set will paint base background. Backgrounds specified on the main frame will be correctly revealed by transparent frames. Design document: https://goo.gl/KGhYpU BUG=458601, 475115, 481769, 499000 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196973

Patch Set 1 #

Patch Set 2 : rebase #

Total comments: 2

Patch Set 3 : rebase & fix unit tests, try again #

Patch Set 4 : rebase... #

Patch Set 5 : update test expectation #

Patch Set 6 : rebase #

Patch Set 7 : try again #

Patch Set 8 : rebase & revise expectation #

Total comments: 12

Patch Set 9 : revise for xianzhu's comment #

Total comments: 24

Patch Set 10 : revise #

Patch Set 11 : make invalidation more robust. also improve incremental invalidation for some common cases #

Total comments: 2

Patch Set 12 : fix DocumentLifecycle error #

Total comments: 1

Patch Set 13 : revert to patchset 10 #

Total comments: 14

Patch Set 14 : fix under invalidation (override LayoutView::visualOverflowRect), revise LayoutBoxModelObject::styl… #

Total comments: 6

Patch Set 15 : revise as suggested. also add extra comments for LayoutView::visualOverflowRect #

Patch Set 16 : update expectation and fix incremental invalidation #

Patch Set 17 : rebase #

Total comments: 52

Patch Set 18 : add comments, revert irrelevant bug fix, adding back bgCompositeCopy.html, revert TestExpectation #

Patch Set 19 : sample rebaseline for linux #

Patch Set 20 : rebase, adding back TestExpectation #

Created: 4 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+513 lines, -381 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	4 chunks	+138 lines, -8 lines	0 comments	Download
M	LayoutTests/fast/repaint/view-background-from-body-1.html	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/transforms/transformed-document-element.html	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/platform/android/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/android/virtual/slimmingpaint/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/linux/css1/formatting_model/canvas-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/linux/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/linux/tables/mozilla/bugs/bug2479-2-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/linux/virtual/slimmingpaint/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/mac/css1/formatting_model/canvas-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/mac/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/mac/tables/mozilla/bugs/bug2479-2-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/mac/virtual/slimmingpaint/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/win-xp/css1/formatting_model/canvas-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/win-xp/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/win-xp/tables/mozilla/bugs/bug2479-2-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/win-xp/virtual/slimmingpaint/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/win/css1/formatting_model/canvas-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/win/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/win/tables/mozilla/bugs/bug2479-2-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/platform/win/virtual/slimmingpaint/svg/transforms/animated-path-inside-transformed-html-expected.txt	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	3 chunks	+31 lines, -4 lines	0 comments	Download
M	Source/core/layout/LayoutBox.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutBox.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	7 chunks	+27 lines, -44 lines	0 comments	Download
M	Source/core/layout/LayoutBoxModelObject.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+5 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutBoxModelObject.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	3 chunks	+42 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutObject.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+0 lines, -3 lines	0 comments	Download
M	Source/core/layout/LayoutObject.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+0 lines, -26 lines	0 comments	Download
M	Source/core/layout/LayoutView.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+4 lines, -2 lines	0 comments	Download
M	Source/core/layout/LayoutView.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	3 chunks	+26 lines, -22 lines	0 comments	Download
M	Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+1 line, -5 lines	0 comments	Download
M	Source/core/paint/BoxPainter.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+11 lines, -8 lines	0 comments	Download
M	Source/core/paint/BoxPainter.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	13 chunks	+99 lines, -180 lines	0 comments	Download
M	Source/core/paint/DisplayItemListPaintTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	5 chunks	+6 lines, -8 lines	0 comments	Download
M	Source/core/paint/FrameSetPainter.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/core/paint/ViewPainter.h	View		1 chunk	+0 lines, -2 lines	0 comments	Download
M	Source/core/paint/ViewPainter.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	2 chunks	+94 lines, -51 lines	0 comments	Download
M	Source/core/style/FillLayer.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 59 (14 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

trchen

NOT ready for review yet. Lots of tests need manual rebaseline. Invalidation correctness needs to ...

4 years, 11 months ago (2015-05-20 06:07:31 UTC) #2

Xianzhu

Can you summarize the CL in the description? https://codereview.chromium.org/1145993002/diff/20001/Source/core/layout/LayoutBox.cpp File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1145993002/diff/20001/Source/core/layout/LayoutBox.cpp#newcode1344 Source/core/layout/LayoutBox.cpp:1344: bool ...

4 years, 11 months ago (2015-05-20 16:14:45 UTC) #3

trchen

I think this is ready for review. Tried a few major websites so far everything ...

4 years, 11 months ago (2015-06-02 21:26:21 UTC) #4

trchen

https://codereview.chromium.org/1145993002/diff/130001/Source/core/layout/LayoutBox.cpp File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1145993002/diff/130001/Source/core/layout/LayoutBox.cpp#newcode1335 Source/core/layout/LayoutBox.cpp:1335: bool LayoutBox::paintInvalidationLayerRectsForImage(WrappedImagePtr image, const FillLayer& layers, bool drawingBackground) > ...

4 years, 11 months ago (2015-06-02 21:33:37 UTC) #5

Xianzhu

https://codereview.chromium.org/1145993002/diff/130001/LayoutTests/fast/backgrounds/bgCompositeCopy.html File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left): https://codereview.chromium.org/1145993002/diff/130001/LayoutTests/fast/backgrounds/bgCompositeCopy.html#oldcode14 LayoutTests/fast/backgrounds/bgCompositeCopy.html:14: </body> What happened to this test case? https://codereview.chromium.org/1145993002/diff/130001/Source/core/dom/Document.cpp File ...

4 years, 11 months ago (2015-06-02 23:34:36 UTC) #6

trchen

https://codereview.chromium.org/1145993002/diff/130001/LayoutTests/fast/backgrounds/bgCompositeCopy.html File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left): https://codereview.chromium.org/1145993002/diff/130001/LayoutTests/fast/backgrounds/bgCompositeCopy.html#oldcode14 LayoutTests/fast/backgrounds/bgCompositeCopy.html:14: </body> On 2015/06/02 23:34:35, Xianzhu wrote: > What happened ...

4 years, 11 months ago (2015-06-03 00:06:59 UTC) #7

https://codereview.chromium.org/1145993002/diff/130001/LayoutTests/fast/backg...
File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left):

https://codereview.chromium.org/1145993002/diff/130001/LayoutTests/fast/backg...
LayoutTests/fast/backgrounds/bgCompositeCopy.html:14: </body>
On 2015/06/02 23:34:35, Xianzhu wrote:
> What happened to this test case?

This test case is wrong since beginning. Backgrounds always create isolation
group, passing background-composite:copy on the bottom layer is no different
from source-over (the default). This CL corrected it as a drive-by change (point
4).

https://codereview.chromium.org/1145993002/diff/130001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1145993002/diff/130001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:1676: || documentStyle->columnGap() != columnGap
On 2015/06/02 23:34:35, Xianzhu wrote:
> Better to keep the same order when calculating, checking and setting them.

Acknowledged.

https://codereview.chromium.org/1145993002/diff/130001/Source/core/layout/Lay...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1145993002/diff/130001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.cpp:263: bodyLayout->
setShouldDoFullPaintInvalidation();
On 2015/06/02 23:34:35, Xianzhu wrote:
> s/-> /->/

Acknowledged.

https://codereview.chromium.org/1145993002/diff/130001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/130001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:199: void BoxPainter::paintFillLayers(const
PaintInfo& paintInfo, const Color& c, const FillLayer& fillLayer, const
LayoutRect& rect, BackgroundBleedAvoidance bleedAvoidance, SkXfermode::Mode op,
LayoutObject* backgroundObject)
On 2015/06/02 23:34:35, Xianzhu wrote:
> Could this (as well as other FillLayer changes) be in a separate CL?

Probably not. The major changes here are:

1. Separating culling loop from paintFillLayers() to a helper function, because
ViewPainter needs it too. Too much code duplication otherwise.

2. Remove the base background hack since it will be handle by ViewPainter now.

3. calculateBackgroundImageGeometry needs to handle the case that paint rect is
expanded to cover the whole canvas.

Those are all pretty core to this CL.

I already reduced much of the irrelevant change. Actually I even want to make
another CL to further refactor paintFillLayerExtended() to split background
positioning and clipping to helper functions. These functions are way too long
and messy. :(

https://codereview.chromium.org/1145993002/diff/130001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.h (right):

https://codereview.chromium.org/1145993002/diff/130001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.h:28: bool
calculateFillLayerOcclusionCulling(Vector<const FillLayer*, 8>
&reversedPaintList, const FillLayer&);
On 2015/06/02 23:34:35, Xianzhu wrote:
> s/ &/& /
> 
> Is there specific reason that '8' is chosen? If not, use default.

It was there in the old paintFillLayers() code. Using the default (no on-stack
buffer) may be too inefficient since we already know at least one layer will be
added, and for most layout objects exactly one will be added.

I don't know why was 8 picked in the beginning, but allocating 64 bytes on the
stack doesn't sound like a big deal to me.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1145993002/150001

4 years, 11 months ago (2015-06-03 03:31:11 UTC) #9

Xianzhu

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxPainter.h File Source/core/paint/BoxPainter.h (right): https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxPainter.h#newcode28 Source/core/paint/BoxPainter.h:28: bool calculateFillLayerOcclusionCulling(Vector<const FillLayer*, 8> &reversedPaintList, const FillLayer&); Would it ...

4 years, 11 months ago (2015-06-03 04:40:34 UTC) #10

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64929)

4 years, 11 months ago (2015-06-03 08:38:47 UTC) #12

chrishtr

Thanks for attacking the root of the problem. This patch is massive, is there a ...

4 years, 11 months ago (2015-06-03 18:15:16 UTC) #13

trchen

https://codereview.chromium.org/1145993002/diff/150001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1145993002/diff/150001/Source/core/dom/Document.cpp#newcode1616 Source/core/dom/Document.cpp:1616: if (isHTMLHtmlElement(documentElement()) && isHTMLBodyElement(body) && !backgroundStyle->hasBackground()) On 2015/06/03 18:15:15, ...

4 years, 11 months ago (2015-06-03 23:15:24 UTC) #14

https://codereview.chromium.org/1145993002/diff/150001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:1616: if (isHTMLHtmlElement(documentElement()) &&
isHTMLBodyElement(body) && !backgroundStyle->hasBackground())
On 2015/06/03 18:15:15, chrishtr wrote:
> Comment why you are doing this conditional override.

Acknowledged.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:1629:
currentLayer->setAttachment(LocalBackgroundAttachment);
On 2015/06/03 18:15:15, chrishtr wrote:
> This looks a little hacky. Is there a way to never set the
> ScrollBackgroundAttachment in the first place?

They do behave like background-attachment:local on the LayoutView. This makes
many things work automatically, for example, --root-layer-scrolls
--enable-prefer-compositing-to-lcd-text.

IMO solving this at the invalidation & painting layer would require lots of
special rules -- being more hacky.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutBox.cpp:96: // or if the <body>'s parent is not the
document element's layoutObject (e.g. inside SVG foreignObject).
On 2015/06/03 18:15:15, chrishtr wrote:
> Are we still handling the SVG foreign object case correctly?

Yes. See backgroundStolenForBeingBody(). CSS requires the quirk only applied to
first child of a root <html> element.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutBox.cpp:1312: if (!parent() && !isLayoutView())
On 2015/06/03 18:15:15, chrishtr wrote:
> split into a protected methodimageChangedInternal for code after this line,
and
> override imageChanged in LayoutView to early out if !parent() before calling
> imageChangedInternal.

It is the opposite, LayoutView should not early out even if !parent(). Actually
I doubt early exiting is even needed here. Can we just remove it?

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.cpp:731: return false;
On 2015/06/03 18:15:15, chrishtr wrote:
> Is this an independent bug?

Yes this is a drive-by fix.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.h:207: bool
backgroundStolenForBeingBody() const;
On 2015/06/03 18:15:15, chrishtr wrote:
> Add comments explaining what this is about.

Acknowledged.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (left):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:211: &&
curLayer->image()->canRender(m_layoutBox, m_layoutBox.style()->effectiveZoom())
On 2015/06/03 18:15:15, chrishtr wrote:
> The conditionals here are changed, why is the new one more correct? Why is it
> needed in this patch?

(Note that the CL made a mistake, the negation for !imageSize().isEmpty() was
left out. Will make correction in the next CL.).

The conditions are almost the same, except there was no !imageSize().isEmpty().
Without it, a zero-sized generated image can incorrectly cull layers below. This
is causing problem for some layout tests.

!m_layoutBox.boxShadowShouldBeAppliedToBackground(bleedAvoidance) is moved out
of the loop because it doesn't apply to LayoutView. The sole purpose is to not
to cull the bottom layer because the bottom layer is responsible for painting
shadow.

Other than that the conditions are identical.

There are multiple occurrence of the same conditions: hasOpaqueImage() &&
canRender() && !imageSize().isEmpty() && hasRepeatXY(), it has been refactored
into a helper function isFillLayerOpaque() above.

blendMode() == WebBlendModeNormal is removed because it is already covered by
hasOpaqueImage().

Arguably some of these minor improvements are optional. I tend to fix them when
I came across one because it is very distracting when many of them add up.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:204: && lastLayer.clip() == BorderFillBox
On 2015/06/03 18:15:15, chrishtr wrote:
> What is the BorderFillBox code here and above for? Can this be split off as a
> separate patch?
> 
> Same for the refactoring/fix here, I'm not sure what it is fixing.

Yes this part can be split as a separate patch. This is for an optimization that
if the element has an opaque background color, isolation group can be skipped.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.h (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.h:28: bool
calculateFillLayerOcclusionCulling(Vector<const FillLayer*, 8>
&reversedPaintList, const FillLayer&);
On 2015/06/03 04:40:34, Xianzhu wrote:
> Would it be better to define a type for Vector<const FillLayer*, 8>?

Acknowledged.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/Fram...
File Source/core/paint/FrameSetPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/Fram...
Source/core/paint/FrameSetPainter.cpp:44:
context->fillRect(IntRect(borderRect.location(), IntSize(1,
borderRect.height())), borderStartEdgeColor());
On 2015/06/03 18:15:15, chrishtr wrote:
> Why the changes in this file?

There is an over-paint with column border. Without the fix, column borders will
stick into frames.

My CL revealed this bug because sub-frames no longer paint base background.
(which is incorrect when the main frame has background, and is redundant too,
painting unnecessary pixels.)

Comment added to the commit message.

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/View...
File Source/core/paint/ViewPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/View...
Source/core/paint/ViewPainter.cpp:69:
transform.multiply(rootLayer.renderableTransform(paintInfo.paintBehavior));
On 2015/06/03 04:40:34, Xianzhu wrote:
> Do we need to invalidate / have we invalidated the layoutView when rootLayer's
> offset or transform change?

Yes we do. Tried with the inspector we are already doing it correctly.

Xianzhu

https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/ViewPainter.cpp File Source/core/paint/ViewPainter.cpp (right): https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/ViewPainter.cpp#newcode69 Source/core/paint/ViewPainter.cpp:69: transform.multiply(rootLayer.renderableTransform(paintInfo.paintBehavior)); On 2015/06/03 23:15:24, trchen wrote: > On 2015/06/03 ...

4 years, 11 months ago (2015-06-03 23:24:19 UTC) #15

trchen

On 2015/06/03 23:24:19, Xianzhu wrote: > https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/ViewPainter.cpp > File Source/core/paint/ViewPainter.cpp (right): > > https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/ViewPainter.cpp#newcode69 > ...

4 years, 11 months ago (2015-06-04 22:16:02 UTC) #16

Xianzhu

On 2015/06/04 22:16:02, trchen wrote: > On 2015/06/03 23:24:19, Xianzhu wrote: > > > https://codereview.chromium.org/1145993002/diff/150001/Source/core/paint/ViewPainter.cpp ...

4 years, 11 months ago (2015-06-05 00:17:01 UTC) #17

trchen

On 2015/06/05 00:17:01, Xianzhu wrote: > On 2015/06/04 22:16:02, trchen wrote: > > On 2015/06/03 ...

4 years, 11 months ago (2015-06-05 00:36:48 UTC) #18

Xianzhu

I didn't see how transform/offset change (without layoutView overflow change) of documentElement invalidates the layoutView. ...

4 years, 11 months ago (2015-06-05 00:52:46 UTC) #19

trchen

On 2015/06/05 00:52:46, Xianzhu wrote: > I didn't see how transform/offset change (without layoutView overflow ...

4 years, 11 months ago (2015-06-05 01:30:48 UTC) #20

trchen

Here is the diff from patch set 10 to 11 for your convenience: https://codereview.chromium.org/1164933003

4 years, 11 months ago (2015-06-05 01:37:23 UTC) #21

Xianzhu

https://codereview.chromium.org/1145993002/diff/190001/Source/core/layout/LayoutView.cpp File Source/core/layout/LayoutView.cpp (right): https://codereview.chromium.org/1145993002/diff/190001/Source/core/layout/LayoutView.cpp#newcode301 Source/core/layout/LayoutView.cpp:301: } We don't allow setShouldDoFullPaintInvalidation() during paint invalidation to ...

4 years, 11 months ago (2015-06-05 03:44:51 UTC) #22

trchen

On 2015/06/05 03:44:51, Xianzhu wrote: > https://codereview.chromium.org/1145993002/diff/190001/Source/core/layout/LayoutView.cpp > File Source/core/layout/LayoutView.cpp (right): > > https://codereview.chromium.org/1145993002/diff/190001/Source/core/layout/LayoutView.cpp#newcode301 > ...

4 years, 11 months ago (2015-06-05 04:29:00 UTC) #23

Xianzhu

On 2015/06/05 01:30:48, trchen wrote: > On 2015/06/05 00:52:46, Xianzhu wrote: > > I didn't ...

4 years, 11 months ago (2015-06-05 05:08:04 UTC) #24

Xianzhu

https://codereview.chromium.org/1145993002/diff/210001/Source/core/layout/LayoutView.cpp File Source/core/layout/LayoutView.cpp (right): https://codereview.chromium.org/1145993002/diff/210001/Source/core/layout/LayoutView.cpp#newcode313 Source/core/layout/LayoutView.cpp:313: getBackgroundPositioningFromDocumentElement(m_previousRootBackgroundPositioning); This seems incorrect. m_previousRootBackgroundPositioning and newPosition will be ...

4 years, 11 months ago (2015-06-05 05:08:28 UTC) #25

Xianzhu

On 2015/06/05 05:08:28, Xianzhu wrote: > https://codereview.chromium.org/1145993002/diff/210001/Source/core/layout/LayoutView.cpp > File Source/core/layout/LayoutView.cpp (right): > > https://codereview.chromium.org/1145993002/diff/210001/Source/core/layout/LayoutView.cpp#newcode313 > ...

4 years, 11 months ago (2015-06-05 05:16:12 UTC) #26

commit-bot: I haz the power

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

4 years, 11 months ago (2015-06-05 23:12:03 UTC) #29

chrishtr

Let's meet in person on Monday to iterate. https://codereview.chromium.org/1145993002/diff/230001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1145993002/diff/230001/Source/core/dom/Document.cpp#newcode1679 Source/core/dom/Document.cpp:1679: || ...

4 years, 11 months ago (2015-06-05 23:54:19 UTC) #32

trchen

https://codereview.chromium.org/1145993002/diff/230001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1145993002/diff/230001/Source/core/dom/Document.cpp#newcode1679 Source/core/dom/Document.cpp:1679: || documentStyle->visitedDependentColor(CSSPropertyBackgroundColor) != backgroundColor On 2015/06/05 23:54:18, chrishtr wrote: ...

4 years, 11 months ago (2015-06-06 05:30:02 UTC) #33

https://codereview.chromium.org/1145993002/diff/230001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1145993002/diff/230001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:1679: ||
documentStyle->visitedDependentColor(CSSPropertyBackgroundColor) !=
backgroundColor
On 2015/06/05 23:54:18, chrishtr wrote:
> How do you know this list is complete? Is it exhaustive in some way?

I think so. Everything in css3-background is covered, but there is no way to be
sure if some other CSS modules are involved. image-rendering is one of them
caught by layout tests.

IMO if no layout tests are broken, we should assume it is correct, and we should
fix it and add tests if problem emerges later.

https://codereview.chromium.org/1145993002/diff/230001/Source/core/layout/Lay...
File Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/1145993002/diff/230001/Source/core/layout/Lay...
Source/core/layout/LayoutBox.cpp:1357: if (view()->frameView())
On 2015/06/05 23:54:18, chrishtr wrote:
> Why are all these hacks not needed any more? In particular the frameView()
bit.

Thanks for catching this! Yes this will result in under-invalidation. Anyway I
think invalidating every compositing layer in the frame is an overkill. The
LayoutView should correctly report its visual overflow. For normal mode without
--root-layer-scrolls, its visual overflow should be the same as layout overflow
instead of the border box.

Another experiment showed that background-attachment:local with
--prefer-compositing-to-lcd-text under-invalidates too, which means root
background will be broken for --root-layer-scrolls
--prefer-compositing-to-lcd-text too. I will file another crbug.

https://codereview.chromium.org/1145993002/diff/230001/Source/core/layout/Lay...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1145993002/diff/230001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.cpp:254: // Refers to
backgroundStolenForBeingBody().
On 2015/06/05 23:54:18, chrishtr wrote:
> Is this code to handle when backgroundStolenForBeingBody() might change? Can
you
> instead compare the value of backgroundStolenForBeingBody() before and after?

Acknowledged.

https://codereview.chromium.org/1145993002/diff/230001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.cpp:730: if (style()->maskImage())
On 2015/06/05 23:54:18, chrishtr wrote:
> This seems a little random. Is it fixing a particular layout test?

Nope. Just feel dangerous when I see this. I can split this to another CL.

https://codereview.chromium.org/1145993002/diff/230001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/230001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:175: && isFillLayerOpaque(*currentLayer,
m_layoutBox)) {
On 2015/06/05 23:54:18, chrishtr wrote:
> Why no early outs for blend mode and shadow bleed avoidance?

Blend mode is already covered by layer.hasOpaqueImage.

The other condition is not for bleed avoidance. It is an optimization to allow
box shadow to be painted in a single command with the solid background, so Skia
can cull unnecessary shadow pixels, assuming that large portion of the shadow
will be covered by the background.

This doesn't need to be checked on every layer. We just need to make sure the
bottom layer is always painted. The check is still performed in
BoxPainter::paintFillLayers(), and is not supposed to be in
calculateFillLayerOcclusionCulling() because it really has nothing to do with
fill layer culling.

https://codereview.chromium.org/1145993002/diff/230001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:184: void BoxPainter::paintFillLayers(const
PaintInfo& paintInfo, const Color& c, const FillLayer& fillLayer, const
LayoutRect& rect, BackgroundBleedAvoidance bleedAvoidance, SkXfermode::Mode op,
LayoutObject* backgroundObject)
On 2015/06/05 23:54:18, chrishtr wrote:
> It would be really nice if you just made the minimal changes to this function,
> itdetermine if the new one is the same. What's the minimum update you can make
> here that is correct?

I need to split out calculateFillLayerOcclusionCulling() because the ViewPainter
wants it too. Otherwise it is 25 lines of duplicated code.

Also I prefer to remove the base color hack. Of course we can leave that to a
cleanup CL, but that is the whole point of refactoring.

The only behavioral difference is: When the box shadow optimization is
performed, we used to not doing any culling at all. Now we still perform
culling, but bottom layer will be added back if it is culled.

Other than that the function behaves identically.

https://codereview.chromium.org/1145993002/diff/230001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:307: bool isBorderFill = bgLayer.clip() ==
BorderFillBox;
On 2015/06/05 23:54:18, chrishtr wrote:
> Same here - it's hard to tell what exactly you are doing to this function and
> why it's definitely correct.

1. Removing the base color hack. ViewPainter always handles base background
color itself.

2. We need to calculate BackgroundImageGeometry early, before evaluating
shouldPaintBackgroundImage. This is because we only set the container size on
the image in calculateBackgroundImageGeometry(), which affects whether SVG and
generated images can render. We have similar problem at
calculateFillLayerOcclusionCulling() above, which I left a TODO.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1145993002/250001

4 years, 11 months ago (2015-06-06 06:32:30 UTC) #36

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57924)

4 years, 11 months ago (2015-06-06 06:36:37 UTC) #38

chrishtr

https://codereview.chromium.org/1145993002/diff/250001/Source/core/layout/LayoutBoxModelObject.cpp File Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/1145993002/diff/250001/Source/core/layout/LayoutBoxModelObject.cpp#newcode254 Source/core/layout/LayoutBoxModelObject.cpp:254: if (isDocumentElement()) { Please add a comment. https://codereview.chromium.org/1145993002/diff/250001/Source/core/layout/LayoutView.cpp File ...

4 years, 10 months ago (2015-06-08 20:30:59 UTC) #39

trchen

4 years, 10 months ago (2015-06-08 22:46:55 UTC) #40

chrishtr

Getting close! I took a deeper look this time and understand what's going on better. ...

4 years, 10 months ago (2015-06-09 21:07:54 UTC) #42

trchen

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/TestExpectations#newcode1799 LayoutTests/TestExpectations:1799: # Now the LayoutView paints root element background, so ...

4 years, 10 months ago (2015-06-09 23:36:08 UTC) #43

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:1799: # Now the LayoutView paints root element
background, so the root layer will have background color.
On 2015/06/09 21:07:52, chrishtr wrote:
>  Please post a tryjob run on a patch set without these, so I can double-check
> the impact of your patch.

Yep, I'm going to do that too.

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backg...
File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backg...
LayoutTests/fast/backgrounds/bgCompositeCopy.html:4:
background-image:url(resources/rgba16.png);
On 2015/06/09 21:07:52, chrishtr wrote:
> Why delete this test?

This test was designed incorrectly, and the implementation was wrong too.

CSS requires that background layers always create an isolation group (i.e.
context.saveLayer() then merge the layer down using src-over), thus applying
-webkit-background-composite:copy on the bottom layer is no different from the
default (src-over) because the isolation group starts with transparent pixels.

Actually I want to propose to remove -webkit-background-composite. It is not
documented in any w3c draft, nor any webkit blog posts. The document at
developer.apple.com is bogus. I don't think anyone cares.

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/trans...
File LayoutTests/fast/transforms/transformed-document-element.html (right):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/trans...
LayoutTests/fast/transforms/transformed-document-element.html:9: <body
style="background-image:url('../../compositing/repaint/resources/grid.png')">
On 2015/06/09 21:07:52, chrishtr wrote:
> Are you trying to test a new scenario? Why delete the old test, is it a
> duplicate of another test?

The old test is incorrect. The background color of the root element should
expand to cover the viewport even with rotation, not painting a diamond shape.

Using a gray solid color doesn't verify the background is positioned correctly,
thus changed to use an image.

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
File
LayoutTests/platform/android/svg/transforms/animated-path-inside-transformed-html-expected.txt
(right):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
LayoutTests/platform/android/svg/transforms/animated-path-inside-transformed-html-expected.txt:8:
"backgroundColor": "#FFFFFF",
On 2015/06/09 21:07:52, chrishtr wrote:
> This one was broken before? Does Firefox agree with us now?

No, it is just the way we dump layer tree.

We dump the background color of the layer owner renderer as the background color
of the layer (which doesn't quite make sense anyway). The root layer is owned by
LayoutView, not the root element, thus root background color was never reported.

About 50% of the rebaseline belong to this type.

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
File LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt
(right):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt:4:
LayoutBlockFlow {HTML} at (0,0) size 800x551
On 2015/06/09 21:07:52, chrishtr wrote:
> ditto

This is due to the way we dump renderer tree.

The LayoutView is supposed to report background color, but somehow we don't
report background color if the renderer has no parent. (bug or feature?)

Whether the root element renderer reports background color isn't all that
important because it no longer paints background. It is not reported because it
has the same background color as its parent (the LayoutView), which doesn't make
sense at all (background color is not inherited, and having the same background
color as the parent doesn't imply painting can be skipped).

This type counts maybe 40% of the rebaseline.

I think the behavior is very buggy and we should fix LayoutTreeAsText, but it is
going to need hundreds or rebaseline, so definitely not in this CL.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/layout/Lay...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/layout/Lay...
Source/core/layout/LayoutBox.cpp:3924: if (layoutView->usesCompositing() &&
(!document().settings() || !document().settings()->rootLayerScrolls()))
On 2015/06/09 21:07:53, chrishtr wrote:
> Is this just an optional optimization?

I'm not sure if this optional. I think having this will improve the chance the
CL sticks.

Without this, growing the document size will fall into either these two cases:
1. LayoutView with no background other than base color: Line 3936 catches this
case, and return invalidationReason (incremental, which is good).
2. LayoutView with background: Line 3951 catches this case, and will result in
full paint invalidation. This is bad because before this CL repaint is
incremental. I'm almost certain this will cause a page load time regression.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (left):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:732: if (obj.style()->writingMode() ==
RightToLeftWritingMode)
On 2015/06/09 21:07:53, chrishtr wrote:
> Not needed to account for RTL?

This is a hack to try to recover border box from paint rect.
In LTR mode the paint rect include left margin but not right margin.
In RTL mode the paint rect include right margin but not left margin.
I use a different approach.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:169: if (currentLayer->composite() !=
CompositeSourceOver || currentLayer->blendMode() != WebBlendModeNormal)
On 2015/06/09 21:07:53, chrishtr wrote:
> Why condition on currentLayer->composite() != CompositeSourceOver here?
> 
> Sorry if I already asked this question earlier. It's hard to keep the whole
> patch in my head.

Because anything other than src-over is non-associative. CSS requires background
layers rendered in an isolation group (i.e. context.saveLayer()), so the
rendering order usually looks like this:

A + (B1 + B2)

Where A is existing contents on the canvas, and B1 B2 are background layers of
the current renderer.

For src-over and no blending, the operation is associative, thus

A + (B1 + B2) = A + B1 + B2

So we can skip saveLayer(). This optimization has been done in the past, but
didn't catch rare case like this one. fast/backgrounds/bgCompositeCopy.html was
incorrect but passed because of this bug.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:332: if (bgImage || shouldPaintBackgroundColor)
{
On 2015/06/09 21:07:53, chrishtr wrote:
> Was it a bug that we were checking bgImage->canRender(obj,
> obj.style()->effectiveZoom()) here before?

Because we can't check that at this point. See reply below.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:339: if (!clippedWithLocalScrolling && !bgImage
&& isBorderFill && isBottomLayer) {
On 2015/06/09 21:07:53, chrishtr wrote:
> ditto

See below.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:452: calculateBackgroundImageGeometry(obj,
paintInfo.paintContainer(), bgLayer, scrolledPaintRect, geometry,
backgroundObject);
On 2015/06/09 21:07:53, chrishtr wrote:
> What's the point of putting this call here rather than where it was before?

calculateBackgroundImageGeometry() primes the image by setting the container
size. This affects the opaqueness of SVG and generated image. A generated image
with zero size can't be rendered.

Bitmap images are fine because their size aren't based on the container.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:603: ASSERT(rootBox);
On 2015/06/09 21:07:53, chrishtr wrote:
> You're asserting that something is non-null right after possibly setting it to
> null in the false condition of the ? operator. ?

It just means
ASSERT(obj.document().documentElement());
ASSERT(obj.document().documentElement()->layoutObject());

The logic follows that if no document element presents or the element has no
box, there will be no background positioning area, and we should not reach here
at all.

I just realize I also should check isBox() before casting. Will fix.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:647: left -= paintRect.x();
On 2015/06/09 21:07:53, chrishtr wrote:
> I don't get this part, can you explain? Also, why can we dump the margins? Is
> that included in the paint rect?

Yes, this is the tricky part. Line 620~652 computes the relative offset between
the background positioning area and the background painting area.

For example, if the positioning area is (12, 34, 56, 78) and the painting area
is (-34, -56, 78, 90), Then left/top should be 46, 90.

For normal elements, paintRect is always the border box, and background clipping
is done externally.

For LayoutView/ViewPainter, this is tricky because the positioning area is the
root element, but the painting area is always the whole canvas. The two boxes
can even have mismatch CTM!

The solution is to apply the root element transform on the canvas, then expand
the paintRect to cover the whole LayoutView in the space of the *root element*.
The paintRect can be arbitrary big rect say LayoutRect::infiniteRect() (the
actual implementation use the enclosingIntRect of the projected quad.).

Anyway, in this code path paintRect is no longer the border box, but fortunately
we know the caller already applied CTM, so the border box is known to be (0, 0,
width, height).

The whole point is to avoid recovering border box from the paintRect because
paintRect can be arbitrary. We only need to know how positioning area is
relative to the border box (line 626~637), and how border box is relative to the
paint rect (applied to CTM by caller).

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/Fram...
File Source/core/paint/FrameSetPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/Fram...
Source/core/paint/FrameSetPainter.cpp:44:
context->fillRect(IntRect(borderRect.location(), IntSize(1,
borderRect.height())), borderStartEdgeColor());
On 2015/06/09 21:07:53, chrishtr wrote:
> Why this change?

There was an over paint, the column border stick out to the frames. It was not
observed because we also over paint sub-frame backgrounds. Not standard
compliant and lots of pixel count wasted (sigh).

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/View...
File Source/core/paint/ViewPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/View...
Source/core/paint/ViewPainter.cpp:32: void
ViewPainter::paintBoxDecorationBackground(const PaintInfo& paintInfo)
On 2015/06/09 21:07:53, chrishtr wrote:
> What prevents this from being called when the background shouldn't be stolen?

LayoutView always steals backgrounds from the root element. Also it needs to
paint base background color.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/View...
Source/core/paint/ViewPainter.cpp:54: if (!rootObject || !rootObject->isBox()) {
On 2015/06/09 21:07:53, chrishtr wrote:
> Add comments explaining this code. Also comments explaining the overall
strategy
> of this method.

Acknowledged.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/View...
Source/core/paint/ViewPainter.cpp:103: GraphicsContextStateSaver
stateSaver(context);
On 2015/06/09 21:07:53, chrishtr wrote:
> Don't use a state saver here, just use save and restore directly. We're trying
> to deprecate that class.

Acknowledged.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/style/Fill...
File Source/core/style/FillLayer.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/style/Fill...
Source/core/style/FillLayer.cpp:377: if (m_composite == CompositeCopy)
On 2015/06/09 21:07:54, chrishtr wrote:
> Why this change?

Not necessary in this CL. Just fix it on the way when I saw it.

Blending is applied before compositing. Non-default blending with composite:copy
means first absorbing color from below, then discard the layer below, thus not
really being opaque.

chrishtr

4 years, 10 months ago (2015-06-10 01:03:39 UTC) #45

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backg...
File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backg...
LayoutTests/fast/backgrounds/bgCompositeCopy.html:4:
background-image:url(resources/rgba16.png);
On 2015/06/09 at 23:36:07, trchen wrote:
> On 2015/06/09 21:07:52, chrishtr wrote:
> > Why delete this test?
> 
> This test was designed incorrectly, and the implementation was wrong too.
> 
> CSS requires that background layers always create an isolation group (i.e.
context.saveLayer() then merge the layer down using src-over), thus applying
-webkit-background-composite:copy on the bottom layer is no different from the
default (src-over) because the isolation group starts with transparent pixels.
> 
> Actually I want to propose to remove -webkit-background-composite. It is not
documented in any w3c draft, nor any webkit blog posts. The document at
developer.apple.com is bogus. I don't think anyone cares.

Ok please file a but to track removal then. Sounds good to remove.

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
File LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt
(right):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt:4:
LayoutBlockFlow {HTML} at (0,0) size 800x551
On 2015/06/09 at 23:36:07, trchen wrote:
> On 2015/06/09 21:07:52, chrishtr wrote:
> > ditto
> 
> This is due to the way we dump renderer tree.
> 
> The LayoutView is supposed to report background color, but somehow we don't
report background color if the renderer has no parent. (bug or feature?)
> 
> Whether the root element renderer reports background color isn't all that
important because it no longer paints background. It is not reported because it
has the same background color as its parent (the LayoutView), which doesn't make
sense at all (background color is not inherited, and having the same background
color as the parent doesn't imply painting can be skipped).
> 
> This type counts maybe 40% of the rebaseline.
> 
> I think the behavior is very buggy and we should fix LayoutTreeAsText, but it
is going to need hundreds or rebaseline, so definitely not in this CL.

Good analysis, cool. Please file a bug to track this also.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:452: calculateBackgroundImageGeometry(obj,
paintInfo.paintContainer(), bgLayer, scrolledPaintRect, geometry,
backgroundObject);
On 2015/06/09 at 23:36:08, trchen wrote:
> On 2015/06/09 21:07:53, chrishtr wrote:
> > What's the point of putting this call here rather than where it was before?
> 
> calculateBackgroundImageGeometry() primes the image by setting the container
size. This affects the opaqueness of SVG and generated image. A generated image
with zero size can't be rendered.
> 
> Bitmap images are fine because their size aren't based on the container.

I see. Why can't we move the call to calculateBackgroundImageGeometry even
earlier in the method then?

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:603: ASSERT(rootBox);
On 2015/06/09 at 23:36:08, trchen wrote:
> On 2015/06/09 21:07:53, chrishtr wrote:
> > You're asserting that something is non-null right after possibly setting it
to
> > null in the false condition of the ? operator. ?
> 
> It just means
> ASSERT(obj.document().documentElement());
> ASSERT(obj.document().documentElement()->layoutObject());
> 
> The logic follows that if no document element presents or the element has no
box, there will be no background positioning area, and we should not reach here
at all.
> 
> I just realize I also should check isBox() before casting. Will fix.

Ok please add a brief comment explaining that next to the assert.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/style/Fill...
File Source/core/style/FillLayer.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/style/Fill...
Source/core/style/FillLayer.cpp:377: if (m_composite == CompositeCopy)
On 2015/06/09 at 23:36:08, trchen wrote:
> On 2015/06/09 21:07:54, chrishtr wrote:
> > Why this change?
> 
> Not necessary in this CL. Just fix it on the way when I saw it.
> 
> Blending is applied before compositing. Non-default blending with
composite:copy means first absorbing color from below, then discard the layer
below, thus not really being opaque.

Ok please move into another patch then.

trchen

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backgrounds/bgCompositeCopy.html File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left): https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backgrounds/bgCompositeCopy.html#oldcode4 LayoutTests/fast/backgrounds/bgCompositeCopy.html:4: background-image:url(resources/rgba16.png); crbug.com/498588 https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt File LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt (right): https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt#newcode4 LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt:4: LayoutBlockFlow ...

4 years, 10 months ago (2015-06-10 01:41:05 UTC) #46

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backg...
File LayoutTests/fast/backgrounds/bgCompositeCopy.html (left):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/fast/backg...
LayoutTests/fast/backgrounds/bgCompositeCopy.html:4:
background-image:url(resources/rgba16.png);
crbug.com/498588

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
File LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt
(right):

https://codereview.chromium.org/1145993002/diff/330001/LayoutTests/platform/a...
LayoutTests/platform/android/tables/mozilla/bugs/bug2479-2-expected.txt:4:
LayoutBlockFlow {HTML} at (0,0) size 800x551
crbug.com/498592

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:452: calculateBackgroundImageGeometry(obj,
paintInfo.paintContainer(), bgLayer, scrolledPaintRect, geometry,
backgroundObject);
On 2015/06/10 01:03:39, chrishtr wrote:
> On 2015/06/09 at 23:36:08, trchen wrote:
> > On 2015/06/09 21:07:53, chrishtr wrote:
> > > What's the point of putting this call here rather than where it was
before?
> > 
> > calculateBackgroundImageGeometry() primes the image by setting the container
> size. This affects the opaqueness of SVG and generated image. A generated
image
> with zero size can't be rendered.
> > 
> > Bitmap images are fine because their size aren't based on the container.
> 
> I see. Why can't we move the call to calculateBackgroundImageGeometry even
> earlier in the method then?

Because calculateBackgroundImageGeometry() needs scrolledPaintRect computed by
the giant switch above. (IMO it doesn't make sense.)

I think these functions should be further refactored. Clipping, positioning, and
painting should split out into smaller helper functions.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:603: ASSERT(rootBox);
On 2015/06/10 01:03:39, chrishtr wrote:
> On 2015/06/09 at 23:36:08, trchen wrote:
> > On 2015/06/09 21:07:53, chrishtr wrote:
> > > You're asserting that something is non-null right after possibly setting
it
> to
> > > null in the false condition of the ? operator. ?
> > 
> > It just means
> > ASSERT(obj.document().documentElement());
> > ASSERT(obj.document().documentElement()->layoutObject());
> > 
> > The logic follows that if no document element presents or the element has no
> box, there will be no background positioning area, and we should not reach
here
> at all.
> > 
> > I just realize I also should check isBox() before casting. Will fix.
> 
> Ok please add a brief comment explaining that next to the assert.

Done.

https://codereview.chromium.org/1145993002/diff/330001/Source/core/style/Fill...
File Source/core/style/FillLayer.cpp (right):

https://codereview.chromium.org/1145993002/diff/330001/Source/core/style/Fill...
Source/core/style/FillLayer.cpp:377: if (m_composite == CompositeCopy)
On 2015/06/10 01:03:39, chrishtr wrote:
> On 2015/06/09 at 23:36:08, trchen wrote:
> > On 2015/06/09 21:07:54, chrishtr wrote:
> > > Why this change?
> > 
> > Not necessary in this CL. Just fix it on the way when I saw it.
> > 
> > Blending is applied before compositing. Non-default blending with
> composite:copy means first absorbing color from below, then discard the layer
> below, thus not really being opaque.
> 
> Ok please move into another patch then.

Done.

chrishtr

Alright, seems the only remaining blockers to commiting this patch is filing bugs for the ...

4 years, 10 months ago (2015-06-10 18:28:56 UTC) #47

Xianzhu

Please also file bug for documentElement root transform/offset change if not addressed yet and not ...

4 years, 10 months ago (2015-06-10 18:40:06 UTC) #48

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1145993002/390001

4 years, 10 months ago (2015-06-10 22:35:27 UTC) #52

chrishtr

On 2015/06/10 at 21:31:54, trchen wrote: > Patch set 19 is sample rebaseline for Linux. ...

4 years, 10 months ago (2015-06-10 22:41:02 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66124)

4 years, 10 months ago (2015-06-11 00:51:55 UTC) #56

commit-bot: I haz the power

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

4 years, 10 months ago (2015-06-11 21:14:03 UTC) #58

commit-bot: I haz the power

4 years, 10 months ago (2015-06-11 21:18:48 UTC) #59

Message was sent while issue was closed.

Committed patchset #20 (id:390001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196973

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages