Move all fast-path paint invalidation mapping into PaintInvalidationState

Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState.

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidationBacking().

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

- Let selection paint invalidation go through fast-path if possible.

BUG=591199
Committed: https://crrev.com/934f67a68da2054e526e13648c5452e2cf79c211
Cr-Commit-Position: refs/heads/master@{#383430}

[Xianzhu, 2016-03-22 23:02:58]

Patchset #8 (id:140001) has been deleted

[Xianzhu, 2016-03-22 23:03:03]

Patchset #7 (id:120001) has been deleted

[Xianzhu, 2016-03-22 23:03:13]

Patchset #6 (id:100001) has been deleted

[Xianzhu, 2016-03-22 23:21:25]

Description was changed from

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

BUG=591199
==========

to

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState;

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidaytionBacking();

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

BUG=591199
==========

[Xianzhu, 2016-03-22 23:27:37]

Description was changed from

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState;

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidaytionBacking();

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

BUG=591199
==========

to

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState.

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidaytionBacking().

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

- Let selection paint invalidation go through fast-path if possible.

BUG=591199
==========

[Xianzhu, 2016-03-22 23:32:40]

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

[Xianzhu, 2016-03-22 23:32:42]

The CL is big but all parts are required to work.

Please start review from LayoutObject::invalidatePaintIfNeeded(), then
PaintInvalidationRect::computePaintInvalidationRectInBacking(), and then
chrishtr please review the non-SVG path, pdr please review the SVG path.

https://codereview.chromium.org/1813383002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/fast/repaint/repaint-during-scroll-with-zoom-expected.txt
(left):

https://codereview.chromium.org/1813383002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/fast/repaint/repaint-during-scroll-with-zoom-expected.txt:16:
[2, 64, 135, 136],
The original rects were for LayoutView. They were viewRect() offset by view's
old and new scroll offsets, which is incorrect. We should not apply scroll
offset on viewRect().

We should cleanup LayoutView paint invalidation, especially after
rootLayerScrolls launches.

[chrishtr, 2016-03-23 20:25:14]

Description was changed from

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState.

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidaytionBacking().

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

- Let selection paint invalidation go through fast-path if possible.

BUG=591199
==========

to

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState.

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidationBacking().

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

- Let selection paint invalidation go through fast-path if possible.

BUG=591199
==========

[pdr., 2016-03-23 22:50:07]

This patch is too large to review thoroughly. Is it possible to separate the SVG
and HTML parts and land them separately? Maybe there are other parts that can be
cleaved off?

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.h:67: using
LayoutObject::localOverflowRectForPaintInvalidation;
What does this "using" statement do?

[Xianzhu, 2016-03-23 23:52:34]

I tried to split the CL into multiple parts, but didn't succeed.

However, you can review it as multiple parts one by one:

1. Change in LayoutObject.cpp (mainly in invalidatePaintIfNeeded()) which change
calls to the new methods in PaintInvalidationState;

2. PaintInvalidationState constructor which prepares mapping for the current
object;

3. PaintInvalidationState::updateForChildren() which prepares mapping for
children (mainly to apply clipping);

4. PaintInvalidationState::computePaintInvalidationRectInBackingForSVG() and
changes in SVGLayoutSupport.

5. The new methods of PaintInvalidationState:
   positionFromPaintInvalidationBacking()
   computePaintInvalidationRectInBacking()
   mapLocalRectToPaintInvalidationBacking()

6. Change about selection paint invalidation

7. Other passive changes

For ease of review, here are old and new call paths for calculating
paintInvalidationRect:

- Old path:
  LayoutObject::invalidatePaintIfNeeded()
   LayoutObject::boundsRectForPaintInvalidation()
    PaintLayer::computePaintInvalidationRect()
     LayoutObject::LayoutRect LayoutObject::computePaintInvalidationRect()
      LayoutObject::clippedOverflowRectForPaintInvalidation()
       get local paint invalidation rect (visual overflow, or SVG object's
paintInvalidationRectInLocalSVGCoordinates())
       LayoutObject::mapToVisibleRectInAncestor()
        fast path using PaintInvalidationContainer
        or slow path recursive mapToVisibleRectInAncestor()

- New path for normal objects:
  LayoutObject::invalidatePaintIfNeeded()
   PaintInvalidationState::computePaintInvalidationRectInBacking()
    LayoutObject::localOverflowRectForPaintInvalidation() / or SVG see below
    fast path using cached data in PaintInvalidationState
    or slow path using LayoutObject::mapToVisibleRectInAncestor()

- New path for SVG objects:
  LayoutObject::invalidatePaintIfNeeded()
   PaintInvalidationState::computePaintInvalidationRectInBacking()
    Fast path:
     SVGLayoutSupport::localOverflowRectForPaintInvalidation()
      SVGObject::paintInvalidationRectInLocalSVGCoordinates()
     then map using cached data in PaintInvalidationState
    Slow path:
     SVGLayoutSupport::clippedOverflowRectForPaintInvalidation()

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.h:67: using
LayoutObject::localOverflowRectForPaintInvalidation;
On 2016/03/23 22:50:07, pdr wrote:
> What does this "using" statement do?

It makes the method private not needing to redefine it with the same definition
(ASSERT_NOT_REACHED()).

[chrishtr, 2016-03-24 00:53:52]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1372: return
selectionRectForPaintInvalidation(view());
Delete the selectionRectForPaintInvalidation() function?

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1402: if
(!newSelectionRect.isEmpty()) {
Make mapLocalRectToPaintInvalidationBacking() early-out if isEmpty() is true on
the rect instead?

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:16: static
LayoutSize computeViewPaintOffset(const LayoutView& layoutView, const
LayoutBoxModelObject& paintInvalidationContainer)
computePaintOffsetFromViewToContainer.

Rename paintInvalidationContainer to container.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:142:
m_paintOffset = computeViewPaintOffset(toLayoutView(currentObject),
m_paintInvalidationContainer);
This is an O(tree depth) computation. Is there no way to make it O(1). e.g. add
the offsets that
are implemented in LayoutView's geometry code to the owning element offsets?

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:194: //
TODO(wangxianzhu): Replace localToAncestorPoint with fast path.
Looks like this is referring to the fast path I suggested above..

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:61: // if the
object doesn't have overflow at top/left or transformation.
How is it affected by clips?

[chrishtr, 2016-03-24 01:05:42]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1055: if
(style()->visibility() != VISIBLE)
Did you lose this optimization?

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:368: r.setY(r.y());
What about this line?

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.h:219: static void
setViewClippingAndScrollOffsetDisabled(bool b) {
s_viewClippingAndScrollOffsetDisabled = b; }
This is pretty weird. Is it really not worth an extra parameter, with a
LayoutView-specific overrride of localToAncestorPoint?

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:137:
m_svgTransform = AffineTransform(m_svgTransform *
currentObject.localToSVGParentTransform());
How about a collection of unittests for all of the special cases in this file,
that compares a version
that uses PaintInvalidationState to the one that uses the slow-path fallback?

Or debug code that ASSERTs the slow path equals the fast path for all layout
tests at least, if the above
is too onerous?

[chrishtr, 2016-03-24 16:58:53]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:368: r.setY(r.y());
On 2016/03/24 at 01:05:42, chrishtr wrote:
> What about this line?

Is this now compensated for in the code I commented on in
PaintInvalidationState?

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:33: static bool
supportsCachedOffsetsForChildren(const LayoutObject& object)
What's the purpose of distinguishing this from supportsCachedOffsets? It appears
the old code didn't do
that.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:99: //
Sometimes we create a new PaintInvalidationState from parentState on the same
object.
Are the cases the "horribly-slow" exceptions in LayoutBlock?

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:112: if
(m_cachedOffsetsEnabled) {
Can this just be (combining also logic from line 135):

m_cachedOffsetsEnabled = m_cachedOffsetsEnabled &&
supportsCachedOffsets(currentObject);

if (currentObject.isSVG()) {
  if (currentObject.isSVGRoot())
    m_svgTransform =
AffineTransform(toLayoutSVGRoot(currentObject).localToBorderBoxTransform());
  else
    m_svgTransform = AffineTransform(m_svgTransform *
currentObject.localToSVGParentTransform());
  return;
}

It appears harmless to set m_svgTransform if m_cachedOffsetsEnabled is false,
but that could also be added to the above conditional.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:137:
m_svgTransform = AffineTransform(m_svgTransform *
currentObject.localToSVGParentTransform());
On 2016/03/24 at 01:05:42, chrishtr wrote:
> How about a collection of unittests for all of the special cases in this file,
that compares a version
> that uses PaintInvalidationState to the one that uses the slow-path fallback?
> 
> Or debug code that ASSERTs the slow path equals the fast path for all layout
tests at least, if the above
> is too onerous?

Another option: make this method a helper method, and unittest it directly for
cases of views, table cells, fixed-position, etc.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:155:
m_paintOffset += toLayoutBox(currentObject).locationOffset();
This code looks different than the old code. The old code looks like it was
applying locationOffset
for everything except table rows.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:190: return;
The old code asserted that isSVG is true...why the difference?

[trchen, 2016-03-25 00:00:43]

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

[trchen, 2016-03-25 00:00:45]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:368: r.setY(r.y());
On 2016/03/24 01:05:42, chrishtr wrote:
> What about this line?

This looks like a no-op to me.

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

https://codereview.chromium.org/1813383002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1340:
invalidatePaintOfSubtreesIfNeeded(paintInvalidationState);
s/paintInvalidationState/newPaintInvalidationState/ ?

[pdr., 2016-03-25 01:08:02]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:48: void
updateForChildren();
There's quite a bit of logic to enforce the ordering here. Can this be switched
around so the paint invalidation state is the one that invalidates the subtree?
Something like:
void PaintInvalidationState::invalidatePaintOfSubtrees(LayoutObject& object)
{
    if (object doesn't have children)
        return;

    // updateForChildren() logic inlined here.

    object.invalidatePaintOfSubtreesIfNeeded(*this);
}

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:323: // This is an
open-coded aggregate of SVGLayoutSupport::localOverflowRectForPaintInvalidation,
This comment is no longer correct because mapToVisibleRectInAncestorSpace is not
called at the end.

Is this optimization needed anymore?

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:338:
contentPaintInvalidationRect.intersect(pixelSnappedBorderBoxRect());
It seems wrong that this is missing the shouldApplyViewportClip check from
mapToVisibleRectInAncestorSpace. Do you agree? If so, can you add a TODO to
investigate it?

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:88: // These are
quirks carried forward from the old paint invalidation infrastructure.
Lets just remove the comment since it's not helping. I'm not sure what it means
either.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:92: if (float
strokeWidthForHairlinePadding = object.isSVGShape() &&
object.styleRef().svgStyle().hasStroke()? toLayoutSVGShape(object).strokeWidth()
: 0) {
Nit: missing space before ?

This is a confusing conditional though. Maybe refactor as:
if (object.isSVGShape() && object.styleRef().svgStyle().hasVisibleStroke()) {
  float strokeWidthForHairlinePadding =
object.styleRef().svgStyle().strokeWidth();
  ...
}

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:124: const
LayoutSVGRoot& SVGLayoutSupport::mapRectToSVGRootForPaintInvalidation(const
LayoutObject& object,
Both callers of mapRectToSVGRootForPaintInvalidation also call
mapToVisibleRectInAncestorSpace. Can this logic be folded together into
something like:
LayoutRect SVGLayoutSupport::mapToVisibleRectInAncestorSpace(..., ancestor)
{
    AffineTransform rootBorderBoxTransform;
    const LayoutSVGRoot& svgRoot = computeTransformToSVGRoot(object,
rootBorderBoxTransform);
    LayoutRect rect = transformPaintInvalidationRect(object,
rootBorderBoxTransform, localPaintInvalidationRect);
    return svgRoot.mapToVisibleRectInAncestorSpace(ancestor, rect);
}

[trchen, 2016-03-25 01:08:49]

https://codereview.chromium.org/1813383002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1813383002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:101:
DontApplyFrameScrollOffsetAndClip = 1 << 4,
Just had an offline discussion with Xianzhu. I suggest that we name this flag
"InputIsInFrameCoordinate", and add a comment that the flag only applies when
the source object is a LayoutView.

The core problem of due to in traditional scrolling without RLS, there is no way
to specify frame coordinate space. Supposedly the frame scrolls but the
LayoutView doesn't, so the local border box space of LayoutView is equivalent to
absolute coordinate space. For frame scrollbars that kind of stuff, they paint
in frame coordinate space, but we can't pass the FrameView to the conversion
function because FrameView is not a LayoutObject.

"DontApplyFrameScrollOffsetAndClip" is unclear because it doesn't say which
scroll offset is getting skipped. Let's say we have 3 nested iframes, and we map
from the innermost to the outermost, do we also ignore scroll offset of the
middle frame?

[trchen, 2016-03-25 01:20:22]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:48: void
updateForChildren();
On 2016/03/25 01:08:01, pdr wrote:
> There's quite a bit of logic to enforce the ordering here. Can this be
switched
> around so the paint invalidation state is the one that invalidates the
subtree?
> Something like:
> void PaintInvalidationState::invalidatePaintOfSubtrees(LayoutObject& object)
> {
>     if (object doesn't have children)
>         return;
> 
>     // updateForChildren() logic inlined here.
> 
>     object.invalidatePaintOfSubtreesIfNeeded(*this);
> }

I can see why we need the complexity here, because we are looking at different
boxes at different places.

In a simplified model we have these boxes (There are actually more. You can
refer to the PaintPropertyTreeBuilder to see how boxes evolve with each CSS
property.):

parent scrolled box --(plus current box location)-->
current layout border box --(apply transform)-->
current paint border box --(apply overflow clip)-->
current scrolled box --(plus child box location)-->
child layout border box

For invalidating the current object, we want the current paint border box. Then
before we visit the children, we want to go to the current scrolled box so each
children can apply their own location and transforms.

We just need to document this better. :/

[pdr., 2016-03-25 01:51:46]

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

https://codereview.chromium.org/1813383002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1340:
invalidatePaintOfSubtreesIfNeeded(paintInvalidationState);
should this be newPaintInvalidationState?

[Xianzhu, 2016-03-25 16:35:55]

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1055: if
(style()->visibility() != VISIBLE)
On 2016/03/24 01:05:42, chrishtr wrote:
> Did you lose this optimization?

Done.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1372: return
selectionRectForPaintInvalidation(view());
On 2016/03/24 00:53:52, chrishtr wrote:
> Delete the selectionRectForPaintInvalidation() function?

Already deleted. (Actually, renamed to localSelectionRect for LayoutObject in
LayoutObject.h.)

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1402: if
(!newSelectionRect.isEmpty()) {
On 2016/03/24 00:53:52, chrishtr wrote:
> Make mapLocalRectToPaintInvalidationBacking() early-out if isEmpty() is true
on
> the rect instead?

I tried that but it broke some cases because sometimes we need to map an empty
rect to get its location.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:368: r.setY(r.y());
On 2016/03/24 01:05:42, chrishtr wrote:
> What about this line?

Isn't this a no-op?

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:16: static
LayoutSize computeViewPaintOffset(const LayoutView& layoutView, const
LayoutBoxModelObject& paintInvalidationContainer)
On 2016/03/24 00:53:52, chrishtr wrote:
> computePaintOffsetFromViewToContainer.
> 
> Rename paintInvalidationContainer to container.

Inlined this into the only call site after the other call site is changed to use
fast path.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:33: static bool
supportsCachedOffsetsForChildren(const LayoutObject& object)
On 2016/03/24 16:58:53, chrishtr wrote:
> What's the purpose of distinguishing this from supportsCachedOffsets? It
appears
> the old code didn't do
> that.

The old code didn't separate paintOffset adjustment for the current object (now
in the constructor) and for children (now in updateForChildren()), so it didn't
need to distinguish supportsCachedOffsetsForChildren().
FlippedBlocksWritingMode() doesn't affect application of paintOffset for the
current object.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:99: //
Sometimes we create a new PaintInvalidationState from parentState on the same
object.
On 2016/03/24 16:58:53, chrishtr wrote:
> Are the cases the "horribly-slow" exceptions in LayoutBlock?

Yes, and another case is LayoutView. FrameView/LayoutPart creates a
PaintInvalidationState for layoutView, and LayoutBlock::invalidateTreeIfNeeded()
will create another one on the same layoutView.

Included the cases in comment.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:112: if
(m_cachedOffsetsEnabled) {
On 2016/03/24 16:58:53, chrishtr wrote:
> Can this just be (combining also logic from line 135):
> 
> m_cachedOffsetsEnabled = m_cachedOffsetsEnabled &&
> supportsCachedOffsets(currentObject);
> 
> if (currentObject.isSVG()) {
>   if (currentObject.isSVGRoot())
>     m_svgTransform =
> AffineTransform(toLayoutSVGRoot(currentObject).localToBorderBoxTransform());
>   else
>     m_svgTransform = AffineTransform(m_svgTransform *
> currentObject.localToSVGParentTransform());
>   return;
> }
> 
> It appears harmless to set m_svgTransform if m_cachedOffsetsEnabled is false,
> but that could also be added to the above conditional.

Rearranged the code to set m_svgTransform regardless of m_cachedOffsetsEnabled.
This exposes a chance that we could use hybrid fast-path (for SVG) and slow-path
(for things above the SVGRoot) in some cases. Added a TODO in
computePaintInvalidationRectInBackingForSVG().

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:137:
m_svgTransform = AffineTransform(m_svgTransform *
currentObject.localToSVGParentTransform());
On 2016/03/24 01:05:42, chrishtr wrote:
> How about a collection of unittests for all of the special cases in this file,
> that compares a version
> that uses PaintInvalidationState to the one that uses the slow-path fallback?
> 
> Or debug code that ASSERTs the slow path equals the fast path for all layout
> tests at least, if the above
> is too onerous?

Added ASSERTs in debug mode.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:142:
m_paintOffset = computeViewPaintOffset(toLayoutView(currentObject),
m_paintInvalidationContainer);
On 2016/03/24 00:53:52, chrishtr wrote:
> This is an O(tree depth) computation. Is there no way to make it O(1). e.g.
add
> the offsets that
> are implemented in LayoutView's geometry code to the owning element offsets?

Done.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:155:
m_paintOffset += toLayoutBox(currentObject).locationOffset();
On 2016/03/24 16:58:53, chrishtr wrote:
> This code looks different than the old code. The old code looks like it was
> applying locationOffset
> for everything except table rows.

In this CL, paintOffset adjustment for the current object is moved from
updatePaintOffsetAndClipForChildren() into the constructor, so that in fast path
mapping, we don't need to apply the offsets of the current object again.

Previously paintOffset adjustment in PaintInvalidationState was for children
only, so LayoutTableRow should not add its locationOffset for child
LayoutTableCells. Now paintOffset adjustment here is for the current object, if
the current object is a LayoutTableRow, it needs to add locationOffset normally
for its own mapping; In the PaintInvalidationState for LayoutTableCell, the
row's offset should be excluded for the cell.

However, I moved the exclusion into updateForChildren() so that we can just
exclude once for a row instead of exclude for each cell.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:190: return;
On 2016/03/24 16:58:53, chrishtr wrote:
> The old code asserted that isSVG is true...why the difference?

Previously we didn't create new PaintInvalidationState for
non-LayoutBoxModelObject and non-SVG objects. (in old
LayoutObject::invalidateTreeIfNeeded), so there were only two cases. In that
way, we may reuse a PaintInvalidationState created for the parent on a child.

Now we need m_currentObject to be always the real current object, otherwise
we'll need to pass the real object into the new mapping methods.

The early return here is because PaintInvalidationState for such an object
doesn't need the following code.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:194: //
TODO(wangxianzhu): Replace localToAncestorPoint with fast path.
On 2016/03/24 00:53:52, chrishtr wrote:
> Looks like this is referring to the fast path I suggested above..

Done.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:48: void
updateForChildren();
On 2016/03/25 01:08:01, pdr wrote:
> There's quite a bit of logic to enforce the ordering here. Can this be
switched
> around so the paint invalidation state is the one that invalidates the
subtree?
> Something like:
> void PaintInvalidationState::invalidatePaintOfSubtrees(LayoutObject& object)
> {
>     if (object doesn't have children)
>         return;
> 
>     // updateForChildren() logic inlined here.
> 
>     object.invalidatePaintOfSubtreesIfNeeded(*this);
> }

I see the benefits like shorter code at call sites and enforced order.

However, tried this but found the following problems:
- m_currentObject is const, but calling its invalidatePaintOfSubtreesIfNeeded()
from PaintInvalidationState needs it to be non-const;
- we also need to make LayoutObject::invalidatePaintOfSubtreesIfNeeded() public.

The order is enforced by the ASSERTs, so I'd like to keep the current logic.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:61: // if the
object doesn't have overflow at top/left or transformation.
On 2016/03/24 00:53:52, chrishtr wrote:
> How is it affected by clips?

It's also affected by clips.

I think the first sentence is already clear enough, so removed the second
sentence.

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:323: // This is an
open-coded aggregate of SVGLayoutSupport::localOverflowRectForPaintInvalidation,
On 2016/03/25 01:08:01, pdr wrote:
> This comment is no longer correct because mapToVisibleRectInAncestorSpace is
not
> called at the end.
> 
> Is this optimization needed anymore?

Removed LayoutSVGRoot::mapToVisibleRectInAncestorSpace from the comment.

Added a TODO here for verifying the optimization. Don't want to expand the CL
any more :)

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:338:
contentPaintInvalidationRect.intersect(pixelSnappedBorderBoxRect());
On 2016/03/25 01:08:01, pdr wrote:
> It seems wrong that this is missing the shouldApplyViewportClip check from
> mapToVisibleRectInAncestorSpace. Do you agree? If so, can you add a TODO to
> investigate it?

Actually the shouldApplyViewportClip is wrong in
LayoutSVGRoot::mapToVisibleRectInAncestorSpace() because it applies to contents
only. The clip should be done by children before passing a rect in its space to
LayoutSVGRoot::mapToVisibleRectInAncestorSpace(). Similarly we should not apply
viewport clip here.

Filed crbug.com/597813.

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

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:88: // These are
quirks carried forward from the old paint invalidation infrastructure.
On 2016/03/25 01:08:01, pdr wrote:
> Lets just remove the comment since it's not helping. I'm not sure what it
means
> either.

Done.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:92: if (float
strokeWidthForHairlinePadding = object.isSVGShape() &&
object.styleRef().svgStyle().hasStroke()? toLayoutSVGShape(object).strokeWidth()
: 0) {
On 2016/03/25 01:08:01, pdr wrote:
> Nit: missing space before ?
> 
> This is a confusing conditional though. Maybe refactor as:
> if (object.isSVGShape() && object.styleRef().svgStyle().hasVisibleStroke()) {
>   float strokeWidthForHairlinePadding =
> object.styleRef().svgStyle().strokeWidth();
>   ...
> }

Done.

https://codereview.chromium.org/1813383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:124: const
LayoutSVGRoot& SVGLayoutSupport::mapRectToSVGRootForPaintInvalidation(const
LayoutObject& object,
On 2016/03/25 01:08:01, pdr wrote:
> Both callers of mapRectToSVGRootForPaintInvalidation also call
> mapToVisibleRectInAncestorSpace. Can this logic be folded together into
> something like:
> LayoutRect SVGLayoutSupport::mapToVisibleRectInAncestorSpace(..., ancestor)
> {
>     AffineTransform rootBorderBoxTransform;
>     const LayoutSVGRoot& svgRoot = computeTransformToSVGRoot(object,
> rootBorderBoxTransform);
>     LayoutRect rect = transformPaintInvalidationRect(object,
> rootBorderBoxTransform, localPaintInvalidationRect);
>     return svgRoot.mapToVisibleRectInAncestorSpace(ancestor, rect);
> }

Good idea. Done.

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

https://codereview.chromium.org/1813383002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1340:
invalidatePaintOfSubtreesIfNeeded(paintInvalidationState);
On 2016/03/25 00:00:45, trchen wrote:
> s/paintInvalidationState/newPaintInvalidationState/ ?

Good catch. This just happened to work because for non-LayoutBoxModelObject and
non-SVG objects newPaintInvalidationState and paintInvalidationState is the same
except m_currentObject.

Done.

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

https://codereview.chromium.org/1813383002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1340:
invalidatePaintOfSubtreesIfNeeded(paintInvalidationState);
On 2016/03/25 01:51:46, pdr wrote:
> should this be newPaintInvalidationState?

Yes. Fixed.

https://codereview.chromium.org/1813383002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1813383002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:101:
DontApplyFrameScrollOffsetAndClip = 1 << 4,
On 2016/03/25 01:08:49, trchen wrote:
> Just had an offline discussion with Xianzhu. I suggest that we name this flag
> "InputIsInFrameCoordinate", and add a comment that the flag only applies when
> the source object is a LayoutView.
> 
> The core problem of due to in traditional scrolling without RLS, there is no
way
> to specify frame coordinate space. Supposedly the frame scrolls but the
> LayoutView doesn't, so the local border box space of LayoutView is equivalent
to
> absolute coordinate space. For frame scrollbars that kind of stuff, they paint
> in frame coordinate space, but we can't pass the FrameView to the conversion
> function because FrameView is not a LayoutObject.
> 
> "DontApplyFrameScrollOffsetAndClip" is unclear because it doesn't say which
> scroll offset is getting skipped. Let's say we have 3 nested iframes, and we
map
> from the innermost to the outermost, do we also ignore scroll offset of the
> middle frame?

Thanks for the input. Renamed to InputIsInFrameCoordinates and updated comments.

https://codereview.chromium.org/1813383002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGContainerPainter.cpp (right):

https://codereview.chromium.org/1813383002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGContainerPainter.cpp:28: if
(!m_layoutSVGContainer.isSVGHiddenContainer()
This is needed because previously LayoutSVGHiddenContainer implemented
clippedOverflowRectForPaintInvalidation() (which returned empty rect) and
paintInvalidationRectInLocalSVGCoordinates() (which returned non-empty rect
covering its descendants) differently.

[Xianzhu, 2016-03-25 17:04:48]

I'm separating some simple parts of this CL out into other CLs. Please hold off
on review until I finish that.

[pdr., 2016-03-25 18:06:29]

On 2016/03/25 at 17:04:48, wangxianzhu wrote:
> I'm separating some simple parts of this CL out into other CLs. Please hold
off on review until I finish that.

FWIW, now that I've reviewed this patch I am pretty happy with it. I was ready
to l-g-t-m before you wrote this.

[Xianzhu, 2016-03-25 19:15:21]

Done separating out https://codereview.chromium.org/1837533002/ and
https://codereview.chromium.org/1829353004/ (landed).

Most of the previous comments still apply. Ptal.

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/svg/as-object/nested-embedded-svg-size-changes-no-layout-triggers-1-expected.txt
(left):

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/as-object/nested-embedded-svg-size-changes-no-layout-triggers-1-expected.txt:12:
[210, 10, 180, 180],
This change is because the original code didn't respect clipping above a
LayoutView.

[pdr., 2016-03-25 20:12:06]

On 2016/03/25 at 19:15:21, wangxianzhu wrote:
> Done separating out https://codereview.chromium.org/1837533002/ and
https://codereview.chromium.org/1829353004/ (landed).
> 
> Most of the previous comments still apply. Ptal.
> 
>
https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/svg/as-object/nested-embedded-svg-size-changes-no-layout-triggers-1-expected.txt
(left):
> 
>
https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/svg/as-object/nested-embedded-svg-size-changes-no-layout-triggers-1-expected.txt:12:
[210, 10, 180, 180],
> This change is because the original code didn't respect clipping above a
LayoutView.

LGTM for the SVG portions of this patch, and the high-level design. I verified
that this does not have any large (2%+) regressions on the SVG performance tests
as well.

[chrishtr, 2016-03-25 22:18:53]

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:517: 
 mode &= ~InputIsInFrameCoordinates here?

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.h (right):

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.h:55: // This method
should never be called. Should use paintInvalidationRectInLocalSVGCoordinates().
TODO to enforce? Not sure what this is for. What do these using declarations do?

[leviw_travelin_and_unemployed, 2016-03-25 22:26:29]

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

[leviw_travelin_and_unemployed, 2016-03-25 22:26:31]

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

https://codereview.chromium.org/1813383002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:301: #if
!defined(NDEBUG) && ENABLE(ASSERT)
Drive-by: isn't ENABLE(ASSERT) enough?

[Xianzhu, 2016-03-25 22:27:15]

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:517: 
On 2016/03/25 22:18:53, chrishtr wrote:
>  mode &= ~InputIsInFrameCoordinates here?

No need because we don't pass 'mode' further.

I'm thinking of making LayoutView::mapToVisibleRectInAncestorSpace() always
require the input rect in frame coordinates, to avoid the mode parameter. This
also applies to IsFixed. The caller should call
adjustOffsetForFixedPosition(rect) before calling
LayoutView::mapToVisibleRectInAncestorSpace().

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.h (right):

https://codereview.chromium.org/1813383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.h:55: // This method
should never be called. Should use paintInvalidationRectInLocalSVGCoordinates().
On 2016/03/25 22:18:53, chrishtr wrote:
> TODO to enforce? Not sure what this is for. What do these using declarations
do?

This 'using' hide LayoutObject::localOverflowRectForPaintInvalidation in private
section. and provide a chance to put the above comment. It's enforced by
ASSERT_NOT_REACHED() in LayoutObject::localOverflowRectForPaintInvalidtion.

Now I feel this seems not necessary and might be confusing. Going to remove
them, and put the comment for SVG in LayoutObject.h.

[chrishtr, 2016-03-25 23:28:08]

lgtm

[trchen, 2016-03-25 23:35:27]

lgtm too.

[Xianzhu, 2016-03-25 23:44:44]

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

https://codereview.chromium.org/1813383002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:301: #if
!defined(NDEBUG) && ENABLE(ASSERT)
On 2016/03/25 22:26:31, leviw wrote:
> Drive-by: isn't ENABLE(ASSERT) enough?

There are two tests crash because of different results of fast-path and
slow-path in debug build:
fast/block/geometry-map-assertion-with-tall-content.html
fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html

Both tests have very big geometry values which overflow LayoutUnit. Our
LayoutUnit uses saturated addition and subtraction, so '+', '-' may be not
interchangeable. It's impossible to make the test pass.

I'm going to make the comparison disabled by default with a local macro.

May consider to add some unit tests later.

[Xianzhu, 2016-03-25 23:51:40]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-03-25 23:51:41]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
chrishtr@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1813383002/#ps360001
(title: "Disable fast-path/slow-path comparison because of saturated operations
of LayoutUnit")

[commit-bot: I haz the power, 2016-03-25 23:52:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813383002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813383002/360001

[commit-bot: I haz the power, 2016-03-26 01:09:46]

Committed patchset #17 (id:360001)

[commit-bot: I haz the power, 2016-03-26 01:11:14]

Description was changed from

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState.

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidationBacking().

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

- Let selection paint invalidation go through fast-path if possible.

BUG=591199
==========

to

==========
Move all fast-path paint invalidation mapping into PaintInvalidationState

- Remove the optional const PaintInvalidationState* parameter from paint
  invalidation mapping methods and move fast-path mapping code into
  PaintInvalidationState.

- PaintLayer::computePaintInvalidationRect() is now
    PaintInvalidationState::computePaintInvalidationRectInBacking();
  PaintLayer::positionFromPaintInvalidationBacking() is now
    PaintInvalidationState::positionFromPaintInvalidationBacking();
  PaintLayer::mapRectToPaintInvalidationBacking() is now
    PaintInvalidationState::mapLocalRectToPaintInvalidationBacking().

- Change LayoutObject::clippedOverflowRectForPaintInvalidationBacking() to:
  * Non-SVG objects and LayoutSVGRoot:
    LayoutObject::localOverflowRectForPaintInvalidation() which doesn't
    map the rect to paint invalidation backing;
  * SVG objects except LayoutSVGRoot:
    Using existing paintInvalidationRectInLocalSVGCoordinates()

  PaintInvalidationState::computePaintInvalidationRectInBacking(),
  calls the above functions to get the local paint invalidation rect,
  and map to paintInvalidationContainer using fast path if possible,
  or slow path by calling LayoutObject::mapToVisibleRectToAncestor().

- Let selection paint invalidation go through fast-path if possible.

BUG=591199

Committed: https://crrev.com/934f67a68da2054e526e13648c5452e2cf79c211
Cr-Commit-Position: refs/heads/master@{#383430}
==========

[commit-bot: I haz the power, 2016-03-26 01:11:16]

Patchset 17 (id:??) landed as
https://crrev.com/934f67a68da2054e526e13648c5452e2cf79c211
Cr-Commit-Position: refs/heads/master@{#383430}