Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

The unneeded LayoutSVGViewportContainer::paint override has also been removed.

BUG=645667
Committed: https://crrev.com/60965ced99bdf9522cf55c0fbbe83bda653ae085
Cr-Commit-Position: refs/heads/master@{#438349}

[pdr., 2016-12-13 03:54:19]

Description was changed from

==========
Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

BUG=645667
==========

to

==========
Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

The unneeded LayoutSVGViewportContainer::paint override has also been removed.

BUG=645667
==========

[pdr., 2016-12-13 03:54:19]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[pdr., 2016-12-13 03:55:32]

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

[commit-bot: I haz the power, 2016-12-13 03:55:54]

Dry run: 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-12-13 05:42:25]

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

[commit-bot: I haz the power, 2016-12-13 05:42:25]

Dry run: This issue passed the CQ dry run.

[fs, 2016-12-13 10:11:24]

LGTM w/ nits and (further) ramblings

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

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp:49: // Allow
LayoutSVGTransformableContainer to update its transform.
Nit: Maybe update this comment too (like in the header.)

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

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp:59:
CHECK(element && isSVGSVGElement(*element));
This looks a bit excessive. DCHECK(isSVGSVGElement(element))? (Or element ->
element() even...)

My spider senses tells me there's legacy abound here... We could probably clean
this up more to make it more obvious only SVGSVGElements instantiate these
(codesearch seems to support that...) Oh well, that'll be another CL...

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:291:
layoutObject->setNeedsBoundariesUpdate();
This is a bugfix? (Well, I guess the next line is too...) Is it coupled with the
TODO in LayoutSVGViewportContainer?

[pdr., 2016-12-13 19:48:54]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-12-13 19:48:55]

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

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp:49: // Allow
LayoutSVGTransformableContainer to update its transform.
On 2016/12/13 at 10:11:23, fs wrote:
> Nit: Maybe update this comment too (like in the header.)

Done. Went with a generic "Update the local transform in subclasses".

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

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp:59:
CHECK(element && isSVGSVGElement(*element));
On 2016/12/13 at 10:11:23, fs wrote:
> This looks a bit excessive. DCHECK(isSVGSVGElement(element))? (Or element ->
element() even...)
> 
> My spider senses tells me there's legacy abound here... We could probably
clean this up more to make it more obvious only SVGSVGElements instantiate these
(codesearch seems to support that...) Oh well, that'll be another CL...

Good idea, done.

I agree that this can be cleaned up although it's not immediately clear how. One
slightly strange case here is that SVGSVGElement is the only class to create
viewport containers, but SVGSVGElement only does this for non-root SVG elements.
Maybe something along the names of LayoutNonRootSVGContainer?

Another area of cleanup is removing the call to calculateLocalTransform in favor
of overriding layout.

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:291:
layoutObject->setNeedsBoundariesUpdate();
On 2016/12/13 at 10:11:23, fs wrote:
> This is a bugfix? (Well, I guess the next line is too...) Is it coupled with
the TODO in LayoutSVGViewportContainer?

This was me jumping the gun on that TODO. I've reverted the changes in this
file.

[pdr., 2016-12-13 19:48:55]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2573653003/#ps80001
(title: "Actually revert SVGSVGElement changes")

[commit-bot: I haz the power, 2016-12-13 19:50:00]

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-12-13 21:45:58]

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

[commit-bot: I haz the power, 2016-12-13 21:45:59]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pdr., 2016-12-13 23:16:58]

On 2016/12/13 at 21:45:59, commit-bot wrote:
> Try jobs failed on following builders:
>   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

:<

[pdr., 2016-12-13 23:17:02]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-12-13 23:17:32]

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-12-14 00:07:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481671022654740,
"parent_rev": "1a75f8d1bd3bbd1a3862873cac9498afa0968d9c", "commit_rev":
"d34f5de3515a83d0374eceecbf8d3d078cb527f1"}

[commit-bot: I haz the power, 2016-12-14 00:07:35]

Description was changed from

==========
Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

The unneeded LayoutSVGViewportContainer::paint override has also been removed.

BUG=645667
==========

to

==========
Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

The unneeded LayoutSVGViewportContainer::paint override has also been removed.

BUG=645667

Review-Url: https://codereview.chromium.org/2573653003
==========

[commit-bot: I haz the power, 2016-12-14 00:07:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-14 00:09:59]

Description was changed from

==========
Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

The unneeded LayoutSVGViewportContainer::paint override has also been removed.

BUG=645667

Review-Url: https://codereview.chromium.org/2573653003
==========

to

==========
Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

This patch removes some subclass knowledge about viewport containers
from LayoutSVGContainer. LayoutSVGViewportContainer::calcViewport has
been moved into LayoutSVGViewportContainer::updateLocalTransform. Similarly,
LayoutSVGViewportContainer::pointIsInsideViewportClip has been removed in
favor of implementing viewport clip logic in LayoutSVGViewportContainer.

The unneeded LayoutSVGViewportContainer::paint override has also been removed.

BUG=645667

Committed: https://crrev.com/60965ced99bdf9522cf55c0fbbe83bda653ae085
Cr-Commit-Position: refs/heads/master@{#438349}
==========

[commit-bot: I haz the power, 2016-12-14 00:10:00]

Patchset 5 (id:??) landed as
https://crrev.com/60965ced99bdf9522cf55c0fbbe83bda653ae085
Cr-Commit-Position: refs/heads/master@{#438349}

[fs, 2016-12-14 12:24:45]

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

https://codereview.chromium.org/2573653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp:59:
CHECK(element && isSVGSVGElement(*element));
On 2016/12/13 at 19:48:55, pdr. wrote:
> On 2016/12/13 at 10:11:23, fs wrote:
> > This looks a bit excessive. DCHECK(isSVGSVGElement(element))? (Or element ->
element() even...)
> > 
> > My spider senses tells me there's legacy abound here... We could probably
clean this up more to make it more obvious only SVGSVGElements instantiate these
(codesearch seems to support that...) Oh well, that'll be another CL...
> 
> Good idea, done.
> 
> I agree that this can be cleaned up although it's not immediately clear how.
One slightly strange case here is that SVGSVGElement is the only class to create
viewport containers, but SVGSVGElement only does this for non-root SVG elements.
Maybe something along the names of LayoutNonRootSVGContainer?

I think what I'd like to clean up first is to remove anything that suggests that
this is a "multi-element" LayoutObject - i.e remove any isSVGSVGElement(...) and
just nail down that assumption. The 1:N from the SVGSVGElement I worry less
about.
 
> Another area of cleanup is removing the call to calculateLocalTransform in
favor of overriding layout.

This I've pondered too now and then, but not really seen anything that suggest
it to be worth it.