Moving HitTest function to DeprecatedPaintLayerStackingNode.

Moved HitTest function to DeprecatedPaintLayerStackingNode.

This is the first step of ultimately minimizing the code in DeprecatedPaintLayer,
so that it can be easily removed later.
See
https://docs.google.com/document/d/1YApHVA0hJDWeFGxHPpJ6J5JM5Wcz2h3s24KPSFp8VMg/edit?pli=1#heading=h.j2l17vvztg48.

BUG=496770
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196690

[chadarmstrong, 2015-06-04 02:42:10]

chadarmstrong@google.com changed reviewers:
+ jchaffraix@chromium.org

[chadarmstrong, 2015-06-04 15:41:56]

chadarmstrong@google.com changed reviewers:
+ dsinclair@chromium.org

[chadarmstrong, 2015-06-04 17:00:40]

In an effort to ultimately eliminate DeprecatedPaintLayer I am moving HitTest
functionality out of DeprecatedPaintLayer into DeprecatedPaintLayerStackingNode.

I think it would ultimately be better design to have StackingNode return only an
ordered vector of stacking context roots and have LayoutView iterate over the
vector and traverse each stacking context tree for hit testing. That way
StackingNode would remain ignorant of painting/hit testing, and the LayoutView
could handle painting by just taking the same array and traversing it backwards.

For now, however, I am keeping the painting and hit testing code separate.

[dsinclair, 2015-06-04 17:39:02]

Can you add a BUG= link to the tracing bug? And expand the description a bit
with what you're doing in this CL?

[chadarmstrong, 2015-06-04 18:11:41]

On 2015/06/04 17:39:02, dsinclair wrote:
> Can you add a BUG= link to the tracing bug? And expand the description a bit
> with what you're doing in this CL?

Done

[dsinclair, 2015-06-04 18:21:23]

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:341: // I do believe it
would be better to have one function here that just returned a vector
Can you make this a TODO(chadarmstrong) and link to a bug to track this change?

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:343: // and painting
abstracted from this class.
What's stopping us from doing this approach? Is it a lot more work?

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right):

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.h:104: 
nit: remove blank line.

[chadarmstrong, 2015-06-04 18:43:39]

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:341: // I do believe it
would be better to have one function here that just returned a vector
On 2015/06/04 18:21:22, dsinclair wrote:
> Can you make this a TODO(chadarmstrong) and link to a bug to track this
change?

Acknowledged.

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:343: // and painting
abstracted from this class.
On 2015/06/04 18:21:22, dsinclair wrote:
> What's stopping us from doing this approach? Is it a lot more work?

Julien believes that allocating an extra array and traversing the tree in two
steps might not be worth the code simplification. I left this comment here to
get other people's thoughts on this as well.

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right):

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.h:104: 
On 2015/06/04 18:21:22, dsinclair wrote:
> nit: remove blank line.

Done.

[Julien - ping for review, 2015-06-05 00:46:55]

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1158673008/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:343: // and painting
abstracted from this class.
On 2015/06/04 at 18:43:38, chadarmstrong wrote:
> On 2015/06/04 18:21:22, dsinclair wrote:
> > What's stopping us from doing this approach? Is it a lot more work?
> 
> Julien believes that allocating an extra array and traversing the tree in two
steps might not be worth the code simplification. I left this comment here to
get other people's thoughts on this as well.

I strongly believe that this will lead to worse runtime and memory complexity
and unless the code simplification is significant (we discussed this with Chad
and I wasn't convinced in the end), no sane reviewer would accept the change. I
won't hold the change if you see something I missed.

Experimental alternative designs do not belong in a TODO IMHO: we should reserve
TODO for changes we have a high chance of pursuing. They do belong to a
crbug.com bug though.

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.h (right):

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.h:531: DeprecatedPaintLayer*
hitTestLayer(DeprecatedPaintLayer* rootLayer, DeprecatedPaintLayer*
containerLayer, HitTestResult&,
I would assume we would want to pass |const DeprecatedPaintLayer*| in this
function. Any reason why it's not the case? (this is a follow-up change btw)

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:343: // and painting
abstracted from this class.
This is better in a bug IMHO (see other comment).

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:351:
ASSERT(layer()->isSelfPaintingLayer() ||
layer()->hasSelfPaintingLayerDescendant());
The hit testing code only calls this function on LayoutView's layer so this
ASSERT is trivially true. Not sure about the accessibility code but I would
expect that they must be calling this on LayoutView or else it wouldn't be
correct.

If we assume this can only be called from LayoutView, then this code belongs to
LayoutView.

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right):

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.h:100: // The hitTest()
method looks for mouse events by walking layers that intersect the point from
front to back.
This ancient comment could definitely be improved upon.

Chad, describing in more details what you've learned about hit testing here
would be super helpful. Especially since this is the actual starting point of
hit testing.

[chadarmstrong, 2015-06-05 17:24:35]

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.h (right):

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.h:531: DeprecatedPaintLayer*
hitTestLayer(DeprecatedPaintLayer* rootLayer, DeprecatedPaintLayer*
containerLayer, HitTestResult&,
On 2015/06/05 00:46:54, Julien Chaffraix - PST wrote:
> I would assume we would want to pass |const DeprecatedPaintLayer*| in this
> function. Any reason why it's not the case? (this is a follow-up change btw)

Acknowledged.

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:343: // and painting
abstracted from this class.
On 2015/06/05 00:46:55, Julien Chaffraix - PST wrote:
> This is better in a bug IMHO (see other comment).

Added bug 497219

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:351:
ASSERT(layer()->isSelfPaintingLayer() ||
layer()->hasSelfPaintingLayerDescendant());
On 2015/06/05 00:46:55, Julien Chaffraix - PST wrote:
> The hit testing code only calls this function on LayoutView's layer so this
> ASSERT is trivially true. Not sure about the accessibility code but I would
> expect that they must be calling this on LayoutView or else it wouldn't be
> correct.
> 
> If we assume this can only be called from LayoutView, then this code belongs
to
> LayoutView.

That should be a different CL

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right):

https://codereview.chromium.org/1158673008/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.h:100: // The hitTest()
method looks for mouse events by walking layers that intersect the point from
front to back.
On 2015/06/05 00:46:55, Julien Chaffraix - PST wrote:
> This ancient comment could definitely be improved upon.
> 
> Chad, describing in more details what you've learned about hit testing here
> would be super helpful. Especially since this is the actual starting point of
> hit testing.

I was planning on it, but I would rather hold off until all the changes are
done.

[Julien - ping for review, 2015-06-05 20:19:28]

lgtm

The description should follow git's - slightly modified - guidelines:
- first line is a short description, ideally less than 50 lines.
- one empty line.
- long description should wrap at 80 characters. I like to advise a 72
characters' limit (see the rationale here
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY).
URLs longer than the limit ends up on their own line.

[chadarmstrong, 2015-06-05 22:47:30]

The CQ bit was checked by chadarmstrong@google.com

[commit-bot: I haz the power, 2015-06-05 22:47:47]

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

[commit-bot: I haz the power, 2015-06-05 22:54:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:54:01]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[chadarmstrong, 2015-06-05 23:27:16]

chadarmstrong@google.com changed reviewers:
+ dmazzoni@chromium.org

[chadarmstrong, 2015-06-05 23:30:26]

I need a reviewer from accessibility. There is no real change on that end.

[dmazzoni, 2015-06-08 17:46:44]

lgtm

[chadarmstrong, 2015-06-08 17:47:37]

The CQ bit was checked by chadarmstrong@google.com

[commit-bot: I haz the power, 2015-06-08 17:48:49]

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

[commit-bot: I haz the power, 2015-06-08 18:48:23]

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

[chadarmstrong, 2015-06-09 16:56:10]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1175623002/ by chadarmstrong@google.com.

The reason for reverting is: We just learned that Slimming Paint will also be
modifying the Hit Testing code, and it would therefore be better to leave the
code as is for now..