Add 'printing' flag to PaintInfo

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

[fs, 2015-07-14 16:11:47]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, wangxianzhu@chromium.org

[fs, 2015-07-14 16:20:49]

Patchset #2 (id:20001) has been deleted

[fs, 2015-07-14 16:20:57]

Patchset #2 (id:40001) has been deleted

[Xianzhu, 2015-07-14 17:48:37]

I have no strong preference, but GC::printing() looks a little better than
Document::printing() to me for painting code because it may be faster.

[fs, 2015-07-14 18:14:39]

On 2015/07/14 17:48:37, Xianzhu wrote:
> I have no strong preference, but GC::printing() looks a little better than
> Document::printing() to me for painting code because it may be faster.

Yeah, on the micro level it's 3 vs. 1 dereference or something like that.
Relatively speaking it's probably a small cost to avoid having two flags for the
same thing (where the Document one is the superset.) If caring greatly about
this access, we could stick it in PaintInfo I suppose.

[Xianzhu, 2015-07-15 01:25:28]

On 2015/07/14 18:14:39, fs wrote:
> On 2015/07/14 17:48:37, Xianzhu wrote:
> > I have no strong preference, but GC::printing() looks a little better than
> > Document::printing() to me for painting code because it may be faster.
> 
> Yeah, on the micro level it's 3 vs. 1 dereference or something like that.
> Relatively speaking it's probably a small cost to avoid having two flags for
the
> same thing (where the Document one is the superset.) If caring greatly about
> this access, we could stick it in PaintInfo I suppose.

PaintInfo::printing() sgtm.

[Xianzhu, 2015-07-15 01:25:55]

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

[Xianzhu, 2015-07-15 01:25:56]

+chrishtr for more opinions.

[fs, 2015-07-15 07:59:37]

fs@opera.com changed reviewers:
+ jchaffraix@chromium.org

[fs, 2015-07-15 07:59:38]

On 2015/07/15 01:25:56, Xianzhu wrote:
> +chrishtr for more opinions.

+jchaffraix too then since hes cleaning up the PaintInfo (and related) flag
parade.

[f(malita), 2015-07-15 13:31:56]

On 2015/07/14 18:14:39, fs wrote:
> On 2015/07/14 17:48:37, Xianzhu wrote:
> > I have no strong preference, but GC::printing() looks a little better than
> > Document::printing() to me for painting code because it may be faster.
> 
> Yeah, on the micro level it's 3 vs. 1 dereference or something like that.
> Relatively speaking it's probably a small cost to avoid having two flags for
the
> same thing (where the Document one is the superset.) If caring greatly about
> this access, we could stick it in PaintInfo I suppose.

SGTM too - PaintInfo flags seems like a good place host this info.

[chrishtr, 2015-07-15 13:33:37]

On 2015/07/15 at 13:31:56, fmalita wrote:
> On 2015/07/14 18:14:39, fs wrote:
> > On 2015/07/14 17:48:37, Xianzhu wrote:
> > > I have no strong preference, but GC::printing() looks a little better than
> > > Document::printing() to me for painting code because it may be faster.
> > 
> > Yeah, on the micro level it's 3 vs. 1 dereference or something like that.
> > Relatively speaking it's probably a small cost to avoid having two flags for
the
> > same thing (where the Document one is the superset.) If caring greatly about
> > this access, we could stick it in PaintInfo I suppose.
> 
> SGTM too - PaintInfo flags seems like a good place host this info.

+1

[Julien - ping for review, 2015-07-15 14:28:28]

On 2015/07/15 at 13:33:37, chrishtr wrote:
> On 2015/07/15 at 13:31:56, fmalita wrote:
> > On 2015/07/14 18:14:39, fs wrote:
> > > On 2015/07/14 17:48:37, Xianzhu wrote:
> > > > I have no strong preference, but GC::printing() looks a little better
than
> > > > Document::printing() to me for painting code because it may be faster.
> > > 
> > > Yeah, on the micro level it's 3 vs. 1 dereference or something like that.
> > > Relatively speaking it's probably a small cost to avoid having two flags
for the
> > > same thing (where the Document one is the superset.) If caring greatly
about
> > > this access, we could stick it in PaintInfo I suppose.
> > 
> > SGTM too - PaintInfo flags seems like a good place host this info.
> 
> +1

I think it definitely is a GlobalPaintFlags as we don't expect it to change
during a paint phase. That said, the plumbing of GlobalPaintFlags is not done
yet so you can just put it into PaintInfo (probably PaintBehavior) with a
TODO(jchaffraix) to move it to GlobalPaintFlags.

[fs, 2015-07-15 15:56:39]

On 2015/07/15 14:28:28, Julien Chaffraix - PST wrote:
> On 2015/07/15 at 13:33:37, chrishtr wrote:
> > On 2015/07/15 at 13:31:56, fmalita wrote:
> > > On 2015/07/14 18:14:39, fs wrote:
> > > > On 2015/07/14 17:48:37, Xianzhu wrote:
> > > > > I have no strong preference, but GC::printing() looks a little better
> than
> > > > > Document::printing() to me for painting code because it may be faster.
> > > > 
> > > > Yeah, on the micro level it's 3 vs. 1 dereference or something like
that.
> > > > Relatively speaking it's probably a small cost to avoid having two flags
> for the
> > > > same thing (where the Document one is the superset.) If caring greatly
> about
> > > > this access, we could stick it in PaintInfo I suppose.
> > > 
> > > SGTM too - PaintInfo flags seems like a good place host this info.
> > 
> > +1
> 
> I think it definitely is a GlobalPaintFlags as we don't expect it to change
> during a paint phase. That said, the plumbing of GlobalPaintFlags is not done
> yet so you can just put it into PaintInfo (probably PaintBehavior) with a
> TODO(jchaffraix) to move it to GlobalPaintFlags.

Did something along these lines (I hope...), PTAL and then scold as needed.

[Julien - ping for review, 2015-07-15 17:14:25]

The flag propagation is fine, some comments.

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);
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)?

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
File Source/core/paint/PaintInfo.h (right):

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
Source/core/paint/PaintInfo.h:81: bool printing() const { return paintBehavior &
PaintBehaviorPrintAdaption; }
Our booleans are usually prefixed by is / did.

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
File Source/core/paint/PaintPhase.h (right):

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:87: // annotations for links.
Not sure what 'annotation for links' represents.

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:88: GlobalPaintPrintAdaption = 1 << 2
Not a huge fan of "print adaption" (first TIL that "adaption" is a word).

The adaption is what I find superfluous. It's really a GlobalPaintWhenPrinting
or something similar.

[fs, 2015-07-15 18:25:58]

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 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.

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
File Source/core/paint/PaintInfo.h (right):

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
Source/core/paint/PaintInfo.h:81: bool printing() const { return paintBehavior &
PaintBehaviorPrintAdaption; }
On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> Our booleans are usually prefixed by is / did.

Was trying to not stray too from the other method names (with the added plus of
slightly easier review - I hoped.) Will make it isPrinting (tomorrow.)

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
File Source/core/paint/PaintPhase.h (right):

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:87: // annotations for links.
On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> Not sure what 'annotation for links' represents.

When I wrote this I was thinking about:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(and the method below it.) Can you think of any term/formulation that would make
that more obvious to you? "(Hyper)Link metadata"?

https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:88: GlobalPaintPrintAdaption = 1 << 2
On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> Not a huge fan of "print adaption" (first TIL that "adaption" is a word).
> 
> The adaption is what I find superfluous. It's really a GlobalPaintWhenPrinting
> or something similar.

"Adaption" (or adaptation for the longer word) seemed fitting long enough [1] in
my head to make me change from the more blunt GlobalPaintPrinting =). Would the
latter be ok enough?

[1] Since what it achieves is essentially an adap(ta)tion from screen media to
print media through various tweaks.

[fs, 2015-07-16 10:47:41]

On 2015/07/15 18:25:58, fs wrote:
...
>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
> File Source/core/paint/PaintInfo.h (right):
> 
>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
> Source/core/paint/PaintInfo.h:81: bool printing() const { return paintBehavior
&
> PaintBehaviorPrintAdaption; }
> On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> > Our booleans are usually prefixed by is / did.
> 
> Was trying to not stray too from the other method names (with the added plus
of
> slightly easier review - I hoped.) Will make it isPrinting (tomorrow.)

Changed.

>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
> File Source/core/paint/PaintPhase.h (right):
> 
>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
> Source/core/paint/PaintPhase.h:87: // annotations for links.
> On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> > Not sure what 'annotation for links' represents.
> 
> When I wrote this I was thinking about:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> (and the method below it.) Can you think of any term/formulation that would
make
> that more obvious to you? "(Hyper)Link metadata"?

Changed this to "URL metadata for links".

>
https://codereview.chromium.org/1236183003/diff/80001/Source/core/paint/Paint...
> Source/core/paint/PaintPhase.h:88: GlobalPaintPrintAdaption = 1 << 2
> On 2015/07/15 17:14:25, Julien Chaffraix - PST wrote:
> > Not a huge fan of "print adaption" (first TIL that "adaption" is a word).
> > 
> > The adaption is what I find superfluous. It's really a
GlobalPaintWhenPrinting
> > or something similar.
> 
> "Adaption" (or adaptation for the longer word) seemed fitting long enough [1]
in
> my head to make me change from the more blunt GlobalPaintPrinting =). Would
the
> latter be ok enough?

Changed to GlobalPaintPrinting.

PTAL.

[Julien - ping for review, 2015-07-16 14:32:01]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-07-16 14:32:01]

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.

[commit-bot: I haz the power, 2015-07-16 14:32:31]

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

[commit-bot: I haz the power, 2015-07-16 14:36:17]

Committed patchset #4 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199034

[fs, 2015-07-16 14:45:00]

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 =)