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

Issue 1236183003: Add 'printing' flag to PaintInfo (Closed)

Created:
5 years, 5 months ago by fs
Modified:
5 years, 5 months ago
CC:
blink-reviews, blink-reviews-paint_chromium.org, dshwang, slimming-paint-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Add 'printing' flag to PaintInfo This adds a new paint flag - PaintInfo::printing() - to be preferred before Document::printing() (and GC::printing()) during paint. These flags should always have the same value. The users of GC::printing() were the "PDF URL rect" code, SVG text painting (to disable shadows) and TextPainter. In the last case, the check can be dropped entirely because TextPainter::*PaintingStyle explicitly clears |Style::shadow| if |isPrinting| is true. Rearrange code in PrintContextTest::printSinglePage to better reflect the flow of actual printing. BUG=424655 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199034

Patch Set 1 #

Patch Set 2 : Fix PrintContextTest #

Patch Set 3 : Move flag to PaintInfo. #

Total comments: 9

Patch Set 4 : PrintAdaption -> Printing; printing -> isPrinting; Comment fixups. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -40 lines) Patch
M Source/core/page/PrintContextTest.cpp View 1 2 1 chunk +3 lines, -7 lines 0 comments Download
M Source/core/paint/BlockPainter.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/paint/DeprecatedPaintLayerPainter.cpp View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/paint/EllipsisBoxPainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/FramePainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/ImagePainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/InlinePainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/InlineTextBoxPainter.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/InlineTextBoxPainter.cpp View 1 2 3 4 chunks +7 lines, -6 lines 0 comments Download
M Source/core/paint/LineBoxListPainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/ObjectPainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/PaintInfo.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/paint/PaintPhase.h View 1 2 3 4 chunks +11 lines, -2 lines 0 comments Download
M Source/core/paint/PartPainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/ReplacedPainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/SVGContainerPainter.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/SVGInlineTextBoxPainter.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/paint/SVGInlineTextBoxPainter.cpp View 1 2 3 5 chunks +7 lines, -8 lines 0 comments Download
M Source/core/paint/SVGRootInlineBoxPainter.cpp View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M Source/core/paint/TextPainter.cpp View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 23 (6 generated)
fs
5 years, 5 months ago (2015-07-14 17:28:34 UTC) #4
Xianzhu
I have no strong preference, but GC::printing() looks a little better than Document::printing() to me ...
5 years, 5 months ago (2015-07-14 17:48:37 UTC) #5
fs
On 2015/07/14 17:48:37, Xianzhu wrote: > I have no strong preference, but GC::printing() looks a ...
5 years, 5 months ago (2015-07-14 18:14:39 UTC) #6
Xianzhu
On 2015/07/14 18:14:39, fs wrote: > On 2015/07/14 17:48:37, Xianzhu wrote: > > I have ...
5 years, 5 months ago (2015-07-15 01:25:28 UTC) #7
Xianzhu
+chrishtr for more opinions.
5 years, 5 months ago (2015-07-15 01:25:56 UTC) #9
fs
On 2015/07/15 01:25:56, Xianzhu wrote: > +chrishtr for more opinions. +jchaffraix too then since hes ...
5 years, 5 months ago (2015-07-15 07:59:38 UTC) #11
f(malita)
On 2015/07/14 18:14:39, fs wrote: > On 2015/07/14 17:48:37, Xianzhu wrote: > > I have ...
5 years, 5 months ago (2015-07-15 13:31:56 UTC) #12
chrishtr
On 2015/07/15 at 13:31:56, fmalita wrote: > On 2015/07/14 18:14:39, fs wrote: > > On ...
5 years, 5 months ago (2015-07-15 13:33:37 UTC) #13
Julien - ping for review
On 2015/07/15 at 13:33:37, chrishtr wrote: > On 2015/07/15 at 13:31:56, fmalita wrote: > > ...
5 years, 5 months ago (2015-07-15 14:28:28 UTC) #14
fs
On 2015/07/15 14:28:28, Julien Chaffraix - PST wrote: > On 2015/07/15 at 13:33:37, chrishtr wrote: ...
5 years, 5 months ago (2015-07-15 15:56:39 UTC) #15
Julien - ping for review
The flag propagation is fine, some comments. https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintContextTest.cpp File Source/core/page/PrintContextTest.cpp (left): https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintContextTest.cpp#oldcode98 Source/core/page/PrintContextTest.cpp:98: document().setPrinting(true); Should ...
5 years, 5 months ago (2015-07-15 17:14:25 UTC) #16
fs
https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintContextTest.cpp File Source/core/page/PrintContextTest.cpp (left): https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintContextTest.cpp#oldcode98 Source/core/page/PrintContextTest.cpp:98: document().setPrinting(true); On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote: ...
5 years, 5 months ago (2015-07-15 18:25:58 UTC) #17
fs
On 2015/07/15 18:25:58, fs wrote: ... > https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/PaintInfo.h > File Source/core/paint/PaintInfo.h (right): > > https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/PaintInfo.h#newcode81 ...
5 years, 5 months ago (2015-07-16 10:47:41 UTC) #18
Julien - ping for review
lgtm https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintContextTest.cpp File Source/core/page/PrintContextTest.cpp (left): https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintContextTest.cpp#oldcode98 Source/core/page/PrintContextTest.cpp:98: document().setPrinting(true); On 2015/07/15 at 18:25:58, fs wrote: > ...
5 years, 5 months ago (2015-07-16 14:32:01 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236183003/100001
5 years, 5 months ago (2015-07-16 14:32:31 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199034
5 years, 5 months ago (2015-07-16 14:36:17 UTC) #22
fs
5 years, 5 months ago (2015-07-16 14:45:00 UTC) #23
Message was sent while issue was closed.
On 2015/07/16 14:32:01, Julien Chaffraix - PST wrote:
> lgtm
> 
>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintC...
> File Source/core/page/PrintContextTest.cpp (left):
> 
>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/page/PrintC...
> Source/core/page/PrintContextTest.cpp:98: document().setPrinting(true);
> On 2015/07/15 at 18:25:58, fs wrote:
> > On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> > > Should we remove Document::setPrinting if we're not testing this
> configuration
> > > anymore? Or do you reserve this to a follow-up change (in which case a
TODO
> is
> > > in order)?
> > 
> > Document::setPrinting(...) is still called, only via
> printContext().begin(...), so this call was made redundant by rearranging the
> code (which now more like the "real" printing code.) The flag is still used
> during layout AFAIK. Maybe we can eventually drop the GC::setPrinting call
below
> though.
> 
> I am just concerned about having several sites holding the same information,
> potentially with discrepancies between them. You're bringing down the number
of
> places where we store the painting information so that's a great change.

Yeah, it was a similar concern that triggered the initial change (to make
GC::printing scarce with option to remove). As for Document::setPrinting it
looks like we're down to a fairly manageable amount: except for a few usages in
unit-tests (would great to not have those, but...) it's only the actual "real"
user left.

And BTW, great job with the flags - now maybe my head will spin slightly slower
when looking at that code =)

Powered by Google App Engine
This is Rietveld 408576698