Turn FrameView's paintBehavior into a real global

Turn FrameView's paintBehavior into a real global

The code was super confusing as the flags were thought as a
special PaintBehavior instead of a different beast. Because
PaintBehavior is not propagated down the Frame tree, some
awkward propagation had to happen (we only propagated some
of the flags).

This change turns the 2 behaviors into their own enum, that
is reserved for global operations. PaintBehavior and
LayerPaintingFlags keep their meanings as subtree operations
and to keep track of which paint operations were applied.

Not changed as part of this is all the callers that used to
use PaintBehavior to check one of the 2 enums. I hope to
pass down the global operation flag in a follow-up change.

[Julien - ping for review, 2015-07-07 00:41:54]

jchaffraix@chromium.org changed reviewers:
+ leviw@chromium.org, pdr@chromium.org

[Julien - ping for review, 2015-07-07 00:41:54]

Simplification change in painting/. I think the direction is good but would like
some dissenting opinion. So +Levi/Philip as the painting duet that usually
disagrees with me :)

[leviw_travelin_and_unemployed, 2015-07-07 20:33:16]

I definitely support breaking this out of PaintBehaviorFlags. Made some notes
before the PDR hammer drops.

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
File Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
Source/core/paint/FramePainter.cpp:105: // TODO: Why is this here and done
globally once per paint phase?
TODO(jchaffraix) ;)

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
Source/core/paint/FramePainter.cpp:126: PaintBehavior paintBehavior =
PaintBehaviorNormal;
Why not just put PaintBehaviorNormal in both locations?

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

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:69: // TODO(jchaffraix): We should pass these as
a const variable
GoodFirstBug? :p

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:72: GlobalPaintNoFlag = 0,
What does this mean? How do we expect the system to behave with this value?

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:74: // flag bypasses a lot of the painting code.
TODO: What is the
More TODOs without your name.

Why this TODO? You specify the *intent* of the code in the preceding comment. As
for whether it's needed, simply turning it off would give you an idea, no? I
know we do special background handling for this. Are there parts that
specifically confuse you?

[pdr., 2015-07-07 21:24:34]

I like this patch.

I don't really understand GlobalPaintSelectionOnly. Is it needed?

The global approach may cause us problems in the future because of nested
frameviews (e.g., SVGImage). Most of the users have a PaintInfo or
DeprecatedPaintLayerPaintingInfo nearby... could these bits be passed down
through the PaintInfo instead of using global state?

[leviw_travelin_and_unemployed, 2015-07-07 21:39:46]

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

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:76: GlobalPaintSelectionOnly = 1 << 0,
We should probably rename this to GlobalPaintPaintingDragImage or something to
make it clearer?

[Julien - ping for review, 2015-07-07 23:54:45]

On 2015/07/07 at 21:24:34, pdr wrote:
> I like this patch.
> 
> I don't really understand GlobalPaintSelectionOnly. Is it needed?

I had a TODO that matched this question so I dug into the code. Crippling the
GlobalPaintSelectionOnly logic and we fail
paint/selection/selection-drag-image-in-iframe.html (which is about how we paint
a dragged selected image) so it is needed. Also it's the only trigger for
PaintPhaseSelection in the code, which covers quite some classes. I don't think
we can easily fold that under PaintPhaseSelection.

It's definitely a bigger change that this refactoring :)

> The global approach may cause us problems in the future because of nested
frameviews (e.g., SVGImage). Most of the users have a PaintInfo or
DeprecatedPaintLayerPaintingInfo nearby... could these bits be passed down
through the PaintInfo instead of using global state?

The functions setting the global state are never called recursively so it's not
a problem now but I share your concern. It is my intent to follow-up with
passing this information during paint time. I punted it to a follow-up patch
(with a TODO in PaintPhase.h) right now but happy to do it upfront if you
prefer.

[Julien - ping for review, 2015-07-08 01:16:30]

It seems like Android is going to force me into doing the refactoring upfront as
we can't access global variables across library boundaries.

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
File Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
Source/core/paint/FramePainter.cpp:105: // TODO: Why is this here and done
globally once per paint phase?
On 2015/07/07 at 20:33:16, leviw wrote:
> TODO(jchaffraix) ;)

Deal!

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
Source/core/paint/FramePainter.cpp:126: PaintBehavior paintBehavior =
PaintBehaviorNormal;
On 2015/07/07 at 20:33:16, leviw wrote:
> Why not just put PaintBehaviorNormal in both locations?

Because variables are so 90's! Removed.

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

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:72: GlobalPaintNoFlag = 0,
On 2015/07/07 at 20:33:16, leviw wrote:
> What does this mean? How do we expect the system to behave with this value?

The flags are used to change the way we paint in a deep way, with this value
being the default (ie normal painting). Renamed to GlobalPaintNormalPhase as it
should fit the intent better.

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:74: // flag bypasses a lot of the painting code.
TODO: What is the
On 2015/07/07 at 20:33:16, leviw wrote:
> More TODOs without your name.
> 
> Why this TODO? You specify the *intent* of the code in the preceding comment.
As for whether it's needed, simply turning it off would give you an idea, no?

Sure but looking for breakage will not yield to better designs and that's really
what the intent of this TODO was.

> I know we do special background handling for this. Are there parts that
specifically confuse you?

My reply to Philip should shed some light here. I removed this comment as the
phase seems needed and it's unclear if there is a better design.

https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
Source/core/paint/PaintPhase.h:76: GlobalPaintSelectionOnly = 1 << 0,
On 2015/07/07 at 21:39:46, leviw wrote:
> We should probably rename this to GlobalPaintPaintingDragImage or something to
make it clearer?

It's actually really about painting only selection so it's not misnamed. It's
also part of drag-image so we could potentially combine them but there is really
nothing drag-image related in the flag itself.

[leviw_travelin_and_unemployed, 2015-07-08 19:30:25]

On 2015/07/08 at 01:16:30, jchaffraix wrote:
> It seems like Android is going to force me into doing the refactoring upfront
as we can't access global variables across library boundaries.
> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
> File Source/core/paint/FramePainter.cpp (right):
> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
> Source/core/paint/FramePainter.cpp:105: // TODO: Why is this here and done
globally once per paint phase?
> On 2015/07/07 at 20:33:16, leviw wrote:
> > TODO(jchaffraix) ;)
> 
> Deal!
> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
> Source/core/paint/FramePainter.cpp:126: PaintBehavior paintBehavior =
PaintBehaviorNormal;
> On 2015/07/07 at 20:33:16, leviw wrote:
> > Why not just put PaintBehaviorNormal in both locations?
> 
> Because variables are so 90's! Removed.
> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> File Source/core/paint/PaintPhase.h (right):
> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> Source/core/paint/PaintPhase.h:72: GlobalPaintNoFlag = 0,
> On 2015/07/07 at 20:33:16, leviw wrote:
> > What does this mean? How do we expect the system to behave with this value?
> 
> The flags are used to change the way we paint in a deep way, with this value
being the default (ie normal painting). Renamed to GlobalPaintNormalPhase as it
should fit the intent better.

Ahh yes. I like that more :)

> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> Source/core/paint/PaintPhase.h:74: // flag bypasses a lot of the painting
code. TODO: What is the
> On 2015/07/07 at 20:33:16, leviw wrote:
> > More TODOs without your name.
> > 
> > Why this TODO? You specify the *intent* of the code in the preceding
comment. As for whether it's needed, simply turning it off would give you an
idea, no?
> 
> Sure but looking for breakage will not yield to better designs and that's
really what the intent of this TODO was.
> 
> > I know we do special background handling for this. Are there parts that
specifically confuse you?
> 
> My reply to Philip should shed some light here. I removed this comment as the
phase seems needed and it's unclear if there is a better design.
> 
>
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> Source/core/paint/PaintPhase.h:76: GlobalPaintSelectionOnly = 1 << 0,
> On 2015/07/07 at 21:39:46, leviw wrote:
> > We should probably rename this to GlobalPaintPaintingDragImage or something
to make it clearer?
> 
> It's actually really about painting only selection so it's not misnamed. It's
also part of drag-image so we could potentially combine them but there is really
nothing drag-image related in the flag itself.

I guess you can see it two ways, and the way you're looking at it is fine. You
could have flags for painting 'normally', one for printing, and one for writing
to the drag image, or you could have these. The only purpose of painting just
the selection (I believe?) is to fill in the contents of the drag image buffer.
That's all I'm trying to say.

[leviw_travelin_and_unemployed, 2015-07-08 19:32:41]

On 2015/07/08 at 19:30:25, leviw wrote:
> On 2015/07/08 at 01:16:30, jchaffraix wrote:
> > It seems like Android is going to force me into doing the refactoring
upfront as we can't access global variables across library boundaries.
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
> > File Source/core/paint/FramePainter.cpp (right):
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
> > Source/core/paint/FramePainter.cpp:105: // TODO: Why is this here and done
globally once per paint phase?
> > On 2015/07/07 at 20:33:16, leviw wrote:
> > > TODO(jchaffraix) ;)
> > 
> > Deal!
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Frame...
> > Source/core/paint/FramePainter.cpp:126: PaintBehavior paintBehavior =
PaintBehaviorNormal;
> > On 2015/07/07 at 20:33:16, leviw wrote:
> > > Why not just put PaintBehaviorNormal in both locations?
> > 
> > Because variables are so 90's! Removed.
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> > File Source/core/paint/PaintPhase.h (right):
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> > Source/core/paint/PaintPhase.h:72: GlobalPaintNoFlag = 0,
> > On 2015/07/07 at 20:33:16, leviw wrote:
> > > What does this mean? How do we expect the system to behave with this
value?
> > 
> > The flags are used to change the way we paint in a deep way, with this value
being the default (ie normal painting). Renamed to GlobalPaintNormalPhase as it
should fit the intent better.
> 
> Ahh yes. I like that more :)
> 
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> > Source/core/paint/PaintPhase.h:74: // flag bypasses a lot of the painting
code. TODO: What is the
> > On 2015/07/07 at 20:33:16, leviw wrote:
> > > More TODOs without your name.
> > > 
> > > Why this TODO? You specify the *intent* of the code in the preceding
comment. As for whether it's needed, simply turning it off would give you an
idea, no?
> > 
> > Sure but looking for breakage will not yield to better designs and that's
really what the intent of this TODO was.
> > 
> > > I know we do special background handling for this. Are there parts that
specifically confuse you?
> > 
> > My reply to Philip should shed some light here. I removed this comment as
the phase seems needed and it's unclear if there is a better design.
> > 
> >
https://codereview.chromium.org/1224893004/diff/40001/Source/core/paint/Paint...
> > Source/core/paint/PaintPhase.h:76: GlobalPaintSelectionOnly = 1 << 0,
> > On 2015/07/07 at 21:39:46, leviw wrote:
> > > We should probably rename this to GlobalPaintPaintingDragImage or
something to make it clearer?
> > 
> > It's actually really about painting only selection so it's not misnamed.
It's also part of drag-image so we could potentially combine them but there is
really nothing drag-image related in the flag itself.
> 
> I guess you can see it two ways, and the way you're looking at it is fine. You
could have flags for painting 'normally', one for printing, and one for writing
to the drag image, or you could have these. The only purpose of painting just
the selection (I believe?) is to fill in the contents of the drag image buffer.
That's all I'm trying to say.

It's also not immediately clear here that painting just the selection *always*
sets the flatten bit as well, as it wouldn't make sense to have one without the
other for the drag image.

[leviw_travelin_and_unemployed, 2015-07-08 19:33:37]

Looking at the patch, I think this is an improvement as is. LGTM. I'm happy to
hear what PDR thinks as well.

[pdr., 2015-07-08 21:09:48]

Julien and I talked offline and he's going to switch to putting the data in
PaintInfo and friends due to the android compile issue.

[chrishtr, 2015-07-12 13:56:21]

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

[chrishtr, 2015-07-12 13:56:22]

It's not good to introduce new global variables. Is it really so onerous to pass
down this state somehow?

[Julien - ping for review, 2015-07-29 18:33:27]

On 2015/07/12 at 13:56:22, chrishtr wrote:
> It's not good to introduce new global variables. Is it really so onerous to
pass down this state somehow?

This approach was superseeded by passing down the state (and refactoring our
flags as I touched this code). See crbug.com/509073 for details.