Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

In particular, we were incorrectly calculating rects for out-of-flow
positioned elements.

Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
Committed: https://crrev.com/a7e6a571947df4ac9a93a8d43882c6d8588514b6
Cr-Commit-Position: refs/heads/master@{#404925}

[wkorman, 2016-06-15 22:07:15]

Description was changed from

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

The input rect to mapToVisualRectInAncestorSpace must already be flipped.

BUG=616600
==========

to

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

The input rect to mapToVisualRectInAncestorSpace must already be flipped.
Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
==========

[chrishtr, 2016-07-08 19:54:51]

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

[chrishtr, 2016-07-08 19:54:52]

https://codereview.chromium.org/2073563002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2073563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2302:
if (anchorLayoutObject->isBox() && !anchorLayoutObject->isLayoutView())
Why this change?

[wkorman, 2016-07-08 23:55:50]

https://codereview.chromium.org/2073563002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2073563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2302:
if (anchorLayoutObject->isBox() && !anchorLayoutObject->isLayoutView())
On 2016/07/08 at 19:54:51, chrishtr wrote:
> Why this change?

Good point, it looks like perhaps we can as you suggested delete 2302 - 2303.
The unit test which you added in http://crrev.com/1640363003 still passes as
does long-vertical-text.html from crbug.com/581264. I'm running another try job
to see if other tests are affected.

[wkorman, 2016-07-09 00:04:43]

Description was changed from

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

The input rect to mapToVisualRectInAncestorSpace must already be flipped.
Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
==========

to

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

In particular, we were incorrectly calculating rects for out-of-flow
positioned elements.

The input rect to mapToVisualRectInAncestorSpace must already be flipped.
Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
==========

[chrishtr, 2016-07-09 00:15:57]

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1001:
flipForWritingMode(rect);
Do you know what the code in this method was trying to do before? Does it have
to do with the special coordinate space that
doesn't flip blocks that mysteriously exists?

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2047: if
(container->isTableRow()) {
Add a comment explaining what's going on here: skip table row because cells and
rows are in the same coordinate space.
Also DCHECK(isTableCell()) to add clarity and enforce what we think is the
truth.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4463: {
Just noticed that you can put in an early out:

if (!style()->isFlippedBlocksWritingMode())
  return location()


and avoid computation of containingBlock for 99.99% of page loads.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:878: LayoutPoint
topLeftLocation(const LayoutBox* container = nullptr) const;
Add a comment saying that passing |container| causes flipped-block flipping
w.r.t. that container, or containingBlock() otherwise.

Also rename the arg to flippedBlocksContainer.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1573: return true;
Adjust the comment above to specify that it's in the parent's coordinate space,
except for container flip.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:392: if
(object.isBox())
Why is this necessary?

[wkorman, 2016-07-09 01:16:08]

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1001:
flipForWritingMode(rect);
On 2016/07/09 at 00:15:57, chrishtr wrote:
> Do you know what the code in this method was trying to do before? Does it have
to do with the special coordinate space that
> doesn't flip blocks that mysteriously exists?

No, I'm not sure what this was aiming to handle. Do we have test coverage for
IntersectionObserver logic? That's the main area I wonder about. Some of this
was added in http://crrev.com/1817693002 and others have been through rename
patches.

Or, what test can I manually put together to gain confidence re: this method. I
will investigate but if you have thought let me know.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2047: if
(container->isTableRow()) {
On 2016/07/09 at 00:15:57, chrishtr wrote:
> Add a comment explaining what's going on here: skip table row because cells
and rows are in the same coordinate space.
> Also DCHECK(isTableCell()) to add clarity and enforce what we think is the
truth.

Done.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4463: {
On 2016/07/09 at 00:15:57, chrishtr wrote:
> Just noticed that you can put in an early out:
> 
> if (!style()->isFlippedBlocksWritingMode())
>   return location()
> 
> 
> and avoid computation of containingBlock for 99.99% of page loads.

Wouldn't we need instead:

if (container && !container->style()->isFlippedBlocksWritingMode()) ...

Since we flip based on whether the container box has flipped blocks writing
mode, not 'this'.

So we still need to look up the containing block if it's not passed in. It could
still be worthwhile, let me know if you think we should do it. It would still
speed up the non-inline case for mapToVisualRectInAncestorSpace?

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:878: LayoutPoint
topLeftLocation(const LayoutBox* container = nullptr) const;
On 2016/07/09 at 00:15:57, chrishtr wrote:
> Add a comment saying that passing |container| causes flipped-block flipping
w.r.t. that container, or containingBlock() otherwise.
> 
> Also rename the arg to flippedBlocksContainer.

Done.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1573: return true;
On 2016/07/09 at 00:15:57, chrishtr wrote:
> Adjust the comment above to specify that it's in the parent's coordinate
space, except for container flip.

Done.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:392: if
(object.isBox())
On 2016/07/09 at 00:15:57, chrishtr wrote:
> Why is this necessary?

Don't we need to flip to respect object's writing mode before passing to
mapToVisualRectInAncestorSpace?

Tests that fail without this are:

fast/box-shadow/shadow-box-resize-writing-mode.html
fast/repaint/window-resize-vertical-writing-mode.html

And the box-shadow one looks visually broken, i.e. repaint rects are incorrect,
without this flip.

[chrishtr, 2016-07-11 17:35:56]

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1001:
flipForWritingMode(rect);
On 2016/07/09 at 01:16:08, wkorman wrote:
> On 2016/07/09 at 00:15:57, chrishtr wrote:
> > Do you know what the code in this method was trying to do before? Does it
have to do with the special coordinate space that
> > doesn't flip blocks that mysteriously exists?
> 
> No, I'm not sure what this was aiming to handle. Do we have test coverage for
IntersectionObserver logic? That's the main area I wonder about. Some of this
was added in http://crrev.com/1817693002 and others have been through rename
patches.
> 
> Or, what test can I manually put together to gain confidence re: this method.
I will investigate but if you have thought let me know.

Yes we have some test coverage for IntersectionObserver. Some of the code and
tests were added for that by Stefan, but 
as we have discovered, were implemented based on an incorrect assumption about
how flipped-blocks works.

Let's just leave the code as you have written it.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4463: {
On 2016/07/09 at 01:16:08, wkorman wrote:
> On 2016/07/09 at 00:15:57, chrishtr wrote:
> > Just noticed that you can put in an early out:
> > 
> > if (!style()->isFlippedBlocksWritingMode())
> >   return location()
> > 
> > 
> > and avoid computation of containingBlock for 99.99% of page loads.
> 
> Wouldn't we need instead:
> 
> if (container && !container->style()->isFlippedBlocksWritingMode()) ...
> 
> Since we flip based on whether the container box has flipped blocks writing
mode, not 'this'.
> 
> So we still need to look up the containing block if it's not passed in. It
could still be worthwhile, let me know if you think we should do it. It would
still speed up the non-inline case for mapToVisualRectInAncestorSpace?

Oh right. Yes seems worthwhile to do it just if container was passed in.

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2073563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:392: if
(object.isBox())
On 2016/07/09 at 01:16:08, wkorman wrote:
> On 2016/07/09 at 00:15:57, chrishtr wrote:
> > Why is this necessary?
> 
> Don't we need to flip to respect object's writing mode before passing to
mapToVisualRectInAncestorSpace?
> 
> Tests that fail without this are:
> 
> fast/box-shadow/shadow-box-resize-writing-mode.html
> fast/repaint/window-resize-vertical-writing-mode.html
> 
> And the box-shadow one looks visually broken, i.e. repaint rects are
incorrect, without this flip.

Let's discuss/debug offline. Maybe I'm misunderstanding something.

[wkorman, 2016-07-11 22:23:09]

Updated comments in PaintInvalidationState and unit tests from in person review.

[chrishtr, 2016-07-11 22:35:11]

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2068: if (!(isInline() &&
isLayoutInline()))
Add a TODO that we don't really understand why this conditional is present,
except that PaintLayer::updateLayerPosition also has it.

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/README.md (left):

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/README.md:31: 
Spurious edits in this file.

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:176: //
currently expects the input rect to be in "physical coordinates with
It's the other way around. They are expected to be in physical coordinate space,
*without* flipped block-flow direction.

[wkorman, 2016-07-11 22:46:17]

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2068: if (!(isInline() &&
isLayoutInline()))
On 2016/07/11 at 22:35:11, chrishtr wrote:
> Add a TODO that we don't really understand why this conditional is present,
except that PaintLayer::updateLayerPosition also has it.

Done.

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/README.md (left):

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/README.md:31: 
On 2016/07/11 at 22:35:11, chrishtr wrote:
> Spurious edits in this file.

Done.

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2073563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:176: //
currently expects the input rect to be in "physical coordinates with
On 2016/07/11 at 22:35:11, chrishtr wrote:
> It's the other way around. They are expected to be in physical coordinate
space, *without* flipped block-flow direction.

Ah, yes, I wrote as if documenting slowMapToVisualRectInAncestorSpace. Fixed.

[chrishtr, 2016-07-11 22:49:42]

https://codereview.chromium.org/2073563002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2073563002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:176: //
currently expects the input rect to be in "physical coordinates without
"physical coordinates (*not* "physical coordinates with flipped block-flow
direction")

[chrishtr, 2016-07-11 22:49:42]

https://codereview.chromium.org/2073563002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2073563002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:176: //
currently expects the input rect to be in "physical coordinates without
"physical coordinates (*not* "physical coordinates with flipped block-flow
direction")

[wkorman, 2016-07-11 22:54:51]

https://codereview.chromium.org/2073563002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp (right):

https://codereview.chromium.org/2073563002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:176: //
currently expects the input rect to be in "physical coordinates without
On 2016/07/11 at 22:49:42, chrishtr wrote:
> "physical coordinates (*not* "physical coordinates with flipped block-flow
direction")

Done.

[chrishtr, 2016-07-11 23:03:12]

lgtm

[wkorman, 2016-07-11 23:03:56]

Description was changed from

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

In particular, we were incorrectly calculating rects for out-of-flow
positioned elements.

The input rect to mapToVisualRectInAncestorSpace must already be flipped.
Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
==========

to

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

In particular, we were incorrectly calculating rects for out-of-flow
positioned elements.

Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
==========

[wkorman, 2016-07-11 23:26:50]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-07-11 23:28:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 00:52:23]

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

[commit-bot: I haz the power, 2016-07-12 00:52:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2016-07-12 00:56:10]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-07-12 00:57:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 01:33:23]

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

[commit-bot: I haz the power, 2016-07-12 01:33:25]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2016-07-12 17:47:25]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-07-12 17:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2073563002/#ps300001
(title: "Sync to head.")

[commit-bot: I haz the power, 2016-07-12 17:48:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 22:55:44]

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

[commit-bot: I haz the power, 2016-07-12 22:55:45]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2016-07-12 23:36:15]

The latest commit failures are ASAN re: inspector instrumentation and don't look
related to my change:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

I think there've been some bot/swarming issues recently. Will attempt commit
again a few more times before looking deeper.

https://codereview.chromium.org/2073563002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2073563002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:994: if
(UNLIKELY(hasFlippedBlocksWritingMode()))
For future work -- changes to this method appear to affect three layout tests:

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

fast/repaint/paged-with-overflowing-block-rl.html
fast/repaint/vertical-overflow-parent.html
fast/repaint/vertical-overflow-same.html

[wkorman, 2016-07-12 23:36:29]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-07-12 23:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 02:44:31]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-07-13 02:45:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 02:48:01]

Description was changed from

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

In particular, we were incorrectly calculating rects for out-of-flow
positioned elements.

Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600
==========

to

==========
Rework mapToVisualRectInAncestorSpace to handle flipped blocks correctly.

In particular, we were incorrectly calculating rects for out-of-flow
positioned elements.

Follows canonical logic in PaintLayer::updateLayerPosition().

BUG=616600

Committed: https://crrev.com/a7e6a571947df4ac9a93a8d43882c6d8588514b6
Cr-Commit-Position: refs/heads/master@{#404925}
==========

[commit-bot: I haz the power, 2016-07-13 02:48:02]

Patchset 16 (id:??) landed as
https://crrev.com/a7e6a571947df4ac9a93a8d43882c6d8588514b6
Cr-Commit-Position: refs/heads/master@{#404925}