Don't un-flip if we're only mapping for one object.

Don't un-flip if we're only mapping for one object.

BUG=644122
Committed: https://crrev.com/397fa7cad90b8ca7537de21e18949fada5d83c32
Cr-Commit-Position: refs/heads/master@{#417141}

[wkorman, 2016-09-07 21:21:59]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 21:22:37]

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

[wkorman, 2016-09-07 22:44:57]

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

[wkorman, 2016-09-07 22:44:58]

Sending this in favor of http://crrev.com/2311183004/ as discussed there and in
person. It's about to clear the CQ dry run for patch set 2.

The one existing test that this patch rebaselines:

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

looks more correct with this patch. It ran a sequence of resizes on a window
with flipped-block content. Two of the previous repaint rects looked wrong.
There's a failure you can see diff for off of patch set 1:

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

Distillation of the diff from my scrutiny, pink is previous and green is new:

(now encompasses content region w/ text, previously only covered ~half of it)
<div style="background-color: pink; opacity: 0.5; position: absolute; left:
339px; top: 235px; width: 600px; height: 265px;"></div>
<div style="background-color: green; opacity: 0.5; position: absolute; left:
339px; top: 0px; width: 600px; height: 500px;"></div>

(now encompasses content region w/ text, previously was totally out of view)
<div style="background-color: pink; opacity: 0.5; position: absolute; left:
939px; top: 9px; width: 339px; height: 235px;"></div>
<div style="background-color: green; opacity: 0.5; position: absolute; left:
0px; top: 0px; width: 939px; height: 235px;"></div>

[chrishtr, 2016-09-07 22:47:23]

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:419: // produce
the final result.
"because the output of this method must be in the "physical coordinates with
flipped block-flow direction" also

An even simpler way to write this patch could be:

if (object == ancestor)
  return;

[wkorman, 2016-09-07 22:52:29]

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:419: // produce
the final result.
On 2016/09/07 22:47:23, chrishtr wrote:
> "because the output of this method must be in the "physical coordinates with
> flipped block-flow direction" also
> 
> An even simpler way to write this patch could be:
> 
> if (object == ancestor)
>   return;

Conceptually yes (and I think we'd need an isBox() check?), but there is a small
bit of add'l logic we'd be skipping:

LayoutView::mapToVisualRectInAncestorSpace:
- adjust offset for fixed position (applies scroll/scrolledContent offset, I
think this would apply in some cases)
- apply transform for full page zooming (but only if !ancestor which isn't the
case)

LayoutBox::mapToVisualRectInAncestorSpace:
- inflate for reflection and filter (I think this would apply in some cases)

[chrishtr, 2016-09-07 22:54:02]

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:419: // produce
the final result.
On 2016/09/07 at 22:52:28, wkorman wrote:
> On 2016/09/07 22:47:23, chrishtr wrote:
> > "because the output of this method must be in the "physical coordinates with
> > flipped block-flow direction" also
> > 
> > An even simpler way to write this patch could be:
> > 
> > if (object == ancestor)
> >   return;
> 
> Conceptually yes (and I think we'd need an isBox() check?), but there is a
small bit of add'l logic we'd be skipping:
> 
> LayoutView::mapToVisualRectInAncestorSpace:
> - adjust offset for fixed position (applies scroll/scrolledContent offset, I
think this would apply in some cases)
> - apply transform for full page zooming (but only if !ancestor which isn't the
case)
> 
> LayoutBox::mapToVisualRectInAncestorSpace:
> - inflate for reflection and filter (I think this would apply in some cases)

Right ok. Makes sense, these are indeed important.

[chrishtr, 2016-09-07 23:08:32]

lgtm

LGTM with adjusted comment.

[wkorman, 2016-09-07 23:13:52]

On 2016/09/07 23:08:32, chrishtr wrote:
> lgtm
> 
> LGTM with adjusted comment.

What was the adjustment, or are you referencing my comment update in PS2?

[wkorman, 2016-09-07 23:18:18]

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2318033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:419: // produce
the final result.
On 2016/09/07 22:47:23, chrishtr wrote:
> "because the output of this method must be in the "physical coordinates with
> flipped block-flow direction" also
> 
> An even simpler way to write this patch could be:
> 
> if (object == ancestor)
>   return;

Updated comment.

[wkorman, 2016-09-07 23:18:25]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-09-07 23:18: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/2318033003/#ps60001
(title: "Update comment.")

[commit-bot: I haz the power, 2016-09-07 23:19:10]

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-09-08 01:12:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 01:16:30]

Description was changed from

==========
Don't un-flip if we're only mapping for one object.

BUG=644122
==========

to

==========
Don't un-flip if we're only mapping for one object.

BUG=644122

Committed: https://crrev.com/397fa7cad90b8ca7537de21e18949fada5d83c32
Cr-Commit-Position: refs/heads/master@{#417141}
==========

[commit-bot: I haz the power, 2016-09-08 01:16:31]

Patchset 4 (id:??) landed as
https://crrev.com/397fa7cad90b8ca7537de21e18949fada5d83c32
Cr-Commit-Position: refs/heads/master@{#417141}