Override repaintTreeAfterLayout for RenderSVGModelObject

Override repaintTreeAfterLayout for RenderSVGModelObject

RenderSVGModelObject needs to have an overload for said method, because
it inherits directly from RenderObject.
Also make sure RenderSVGBlock disables use of LayoutState.

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

[dsinclair, 2014-05-02 15:19:19]

https://codereview.chromium.org/260963012/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/260963012/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1534: 
I'm seeing a bunch of svg/dynamic-updates expectation changes for the change I
made locally. Do you need to add them here?

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGModelObject.cpp:145:
RenderObject::repaintTreeAfterLayout();
Does this need a LayoutStateDisabler?

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGModelObject.cpp:172:
RenderObject::repaintTreeAfterLayout();
Ditto, layout state disabler?

[fs, 2014-05-02 15:36:09]

https://codereview.chromium.org/260963012/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/260963012/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1534: 
On 2014/05/02 15:19:19, dsinclair wrote:
> I'm seeing a bunch of svg/dynamic-updates expectation changes for the change I
> made locally. Do you need to add them here?

Probably. Will see what the try-bots say.

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGModelObject.cpp:145:
RenderObject::repaintTreeAfterLayout();
On 2014/05/02 15:19:19, dsinclair wrote:
> Does this need a LayoutStateDisabler?

Maybe... Can't be used in it's current form though (requires/takes RenderBox).
Maybe that implies we need to overload this for RenderSVGRoot too?

[dsinclair, 2014-05-02 15:40:26]

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGModelObject.cpp:145:
RenderObject::repaintTreeAfterLayout();
On 2014/05/02 15:36:09, fs wrote:
> On 2014/05/02 15:19:19, dsinclair wrote:
> > Does this need a LayoutStateDisabler?
> 
> Maybe... Can't be used in it's current form though (requires/takes RenderBox).
> Maybe that implies we need to overload this for RenderSVGRoot too?


I don't think it has to take a RenderBox. It just uses the param to get the
view() which is on RenderObject. Locally I just changed the param to be
RenderObject, was going to see what leviw and jchaffraix though about that.

[fs, 2014-05-02 15:53:51]

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGModelObject.cpp:145:
RenderObject::repaintTreeAfterLayout();
On 2014/05/02 15:40:26, dsinclair wrote:
> On 2014/05/02 15:36:09, fs wrote:
> > On 2014/05/02 15:19:19, dsinclair wrote:
> > > Does this need a LayoutStateDisabler?
> > 
> > Maybe... Can't be used in it's current form though (requires/takes
RenderBox).
> > Maybe that implies we need to overload this for RenderSVGRoot too?
> 
> 
> I don't think it has to take a RenderBox. It just uses the param to get the
> view() which is on RenderObject. Locally I just changed the param to be
> RenderObject, was going to see what leviw and jchaffraix though about that.

Yeah, I wasn't sure if that would be frowned upon or not... I suspect it needs
to be sprinkled in one/a few other place though (RenderSVGBlock.)

[fs, 2014-05-02 16:09:57]

Uploaded version w/ state-disablers.

https://codereview.chromium.org/260963012/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/260963012/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1534: 
On 2014/05/02 15:36:09, fs wrote:
> On 2014/05/02 15:19:19, dsinclair wrote:
> > I'm seeing a bunch of svg/dynamic-updates expectation changes for the change
I
> > made locally. Do you need to add them here?
> 
> Probably. Will see what the try-bots say.

Looks like this is "moving target" ATM =/ (lots of svg/dynamic-updates
rebaselines still to be done.)

[dsinclair, 2014-05-02 18:01:01]

non-owner lgtm.

[leviw_travelin_and_unemployed, 2014-05-02 18:30:15]

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:130: // Note: This is a
reduced version of RenderBox::repaintTreeAfterLayout().
This makes me sad, but I don't have a better idea off hand.

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:150: if
(onlyNeededPositionedMovementLayout() && compositingState() !=
PaintsIntoOwnBacking) {
nit: don't need braces

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:163: if (enclosingLayer()) {
This block of code should be moved into a shared function.

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:173: LayoutStateDisabler
layoutStateDisabler(*this);
Should we just have one of these at the top of the function instead of 2?

[fs, 2014-05-05 08:27:11]

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:150: if
(onlyNeededPositionedMovementLayout() && compositingState() !=
PaintsIntoOwnBacking) {
On 2014/05/02 18:30:15, leviw wrote:
> nit: don't need braces

Dropped.

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:163: if (enclosingLayer()) {
On 2014/05/02 18:30:15, leviw wrote:
> This block of code should be moved into a shared function.

Relocated to RenderObject::repaintScrollbarsIfNeeded.

https://codereview.chromium.org/260963012/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:173: LayoutStateDisabler
layoutStateDisabler(*this);
On 2014/05/02 18:30:15, leviw wrote:
> Should we just have one of these at the top of the function instead of 2?

Makes sense. Hoisted and combined.

[fs, 2014-05-05 15:41:47]

leviw: PTAL.

[Julien - ping for review, 2014-05-05 16:34:36]

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:912: crbug.com/369486
svg/dynamic-updates/SVGForeignObjectElement-dom-x-attr.html [ NeedsRebaseline ]
What are the differences? Extra repainting or that we are just correctly
repainting now?

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/svg/repaint/...
File LayoutTests/svg/repaint/shape-transform-change.html (right):

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/svg/repaint/...
LayoutTests/svg/repaint/shape-transform-change.html:16: <svg width="200"
height="200">
I prefer tests that are self-explanatory so could you include in this test:
- what we are testing (description of the test)
- what are the conditions for passing / failing.

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1526: if (RenderLayerScrollableArea* area
= enclosingLayer() ? enclosingLayer()->scrollableArea() : 0) {
Nit: I would use an early return here to avoid the extra indentation.

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:129: // Note: This is a
reduced version of RenderBox::repaintTreeAfterLayout().
This should probably be a FIXME about sharing code between the 2 but it is
orthogonal.

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:150:
setShouldDoFullRepaintAfterLayout(true);
Can we set positioned movement only flag on SVG renderer?

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:158:
repaintOverflowIfNeeded();
Wouldn't this be a no-op for SVG?

[fs, 2014-05-05 17:33:06]

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:912: crbug.com/369486
svg/dynamic-updates/SVGForeignObjectElement-dom-x-attr.html [ NeedsRebaseline ]
On 2014/05/05 16:34:37, Julien Chaffraix - PST wrote:
> What are the differences? Extra repainting or that we are just correctly
> repainting now?

In general correctly repainting (i.e. the shapes/et.c that would not "emit" any
repaint rectangles now do.) It's not always obvious of course, but I've not seen
any case where there's an extra repainting (which is sort of expected, since
previously luck was the only way to get anything repainted for essentially
everything but the SVG-root.)

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/svg/repaint/...
File LayoutTests/svg/repaint/shape-transform-change.html (right):

https://codereview.chromium.org/260963012/diff/80002/LayoutTests/svg/repaint/...
LayoutTests/svg/repaint/shape-transform-change.html:16: <svg width="200"
height="200">
On 2014/05/05 16:34:37, Julien Chaffraix - PST wrote:
> I prefer tests that are self-explanatory so could you include in this test:
> - what we are testing (description of the test)
> - what are the conditions for passing / failing.

It's self-explanatory in the sense that if you see any red it's a fail =).
Anyway, added notes for the above.

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1526: if (RenderLayerScrollableArea* area
= enclosingLayer() ? enclosingLayer()->scrollableArea() : 0) {
On 2014/05/05 16:34:37, Julien Chaffraix - PST wrote:
> Nit: I would use an early return here to avoid the extra indentation.

Done.

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:129: // Note: This is a
reduced version of RenderBox::repaintTreeAfterLayout().
On 2014/05/05 16:34:37, Julien Chaffraix - PST wrote:
> This should probably be a FIXME about sharing code between the 2 but it is
> orthogonal.

Added FIXME as well.

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:150:
setShouldDoFullRepaintAfterLayout(true);
On 2014/05/05 16:34:37, Julien Chaffraix - PST wrote:
> Can we set positioned movement only flag on SVG renderer?

Not AFAIK. Dropping these two lines. (Had kept them initially to maybe help
later code-consolidation.)

https://codereview.chromium.org/260963012/diff/80002/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:158:
repaintOverflowIfNeeded();
On 2014/05/05 16:34:37, Julien Chaffraix - PST wrote:
> Wouldn't this be a no-op for SVG?

Yes (same as above.) Dropping these two lines too. Same goes for the next call,
so I'll undo that change too.

[leviw_travelin_and_unemployed, 2014-05-05 18:36:48]

LGTM.

https://codereview.chromium.org/260963012/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/260963012/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderView.h:366: LayoutStateDisabler(const RenderObject&
root)
This will end up conflicting with https://codereview.chromium.org/264713015/
(currently in the CQ)

[Julien - ping for review, 2014-05-05 18:39:07]

lgtm

https://codereview.chromium.org/260963012/diff/110001/LayoutTests/svg/repaint...
File LayoutTests/svg/repaint/shape-transform-change.html (right):

https://codereview.chromium.org/260963012/diff/110001/LayoutTests/svg/repaint...
LayoutTests/svg/repaint/shape-transform-change.html:18: <!-- Pass if there's a
green 200x200 (px) rectangle, and no red visible. -->
My view on comments (or using <title> for that matter) in test is that they are
useless:
- either dump it in the test output as it's useful for someone trying to debug
the test.
- just remove it if they don't help.

Here it is useful so it should be dumped in the text output.

[fs, 2014-05-06 07:53:38]

https://codereview.chromium.org/260963012/diff/110001/LayoutTests/svg/repaint...
File LayoutTests/svg/repaint/shape-transform-change.html (right):

https://codereview.chromium.org/260963012/diff/110001/LayoutTests/svg/repaint...
LayoutTests/svg/repaint/shape-transform-change.html:18: <!-- Pass if there's a
green 200x200 (px) rectangle, and no red visible. -->
On 2014/05/05 18:39:07, Julien Chaffraix - PST wrote:
> My view on comments (or using <title> for that matter) in test is that they
are
> useless:
> - either dump it in the test output as it's useful for someone trying to debug
> the test.
> - just remove it if they don't help.
> 
> Here it is useful so it should be dumped in the text output.

Modified to be dumpable. (<!--...  --> => <p>...</p>)

https://codereview.chromium.org/260963012/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/260963012/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderView.h:366: LayoutStateDisabler(const RenderObject&
root)
On 2014/05/05 18:36:48, leviw wrote:
> This will end up conflicting with https://codereview.chromium.org/264713015/
> (currently in the CQ)

Thanks for the heads-up. Trivial though (git rebase took care of it.)

[fs, 2014-05-06 09:18:55]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-05-06 09:19:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fs@opera.com/260963012/150001

[commit-bot: I haz the power, 2014-05-06 10:11:47]

Change committed as 173372