Remove currentView, useCurrentView properties of SVGSVGElement and SVGViewSpec interface (issue 2163213007 by shanmuga.m@samsung.com)

[shanm...@samsung.com]

Reviewers: rwlbuis
CL: https://codereview.chromium.org/2163213007/

Message:
On 2016/07/21 12:54:21, Shanmuga Pandi wrote:
> mailto:shanm...@samsung.com changed reviewers:
> + mailto:rob....@samsung.com

PTAL!!
Thanks

Description:
Remove currentView, useCurrentView properties of SVGSVGElement and SVGViewSpec
interface

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Dfqld7wjHJM

BUG=629445

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+-16, -1578 lines):
M android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-preserveAspectRatio-expected.png
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-transform-expected.png
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-viewBox-expected.png
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-viewBox-expected.txt
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-viewBox-transform-expected.png
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-viewBox-transform-expected.txt
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-viewTarget-expected.png
D third_party/WebKit/LayoutTests/platform/linux/svg/custom/linking-a-03-b-zoomAndPan-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-preserveAspectRatio-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-preserveAspectRatio-expected.txt
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-transform-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-transform-expected.txt
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-viewBox-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-viewBox-transform-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-viewTarget-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-viewTarget-expected.txt
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-zoomAndPan-expected.png
D third_party/WebKit/LayoutTests/platform/mac/svg/custom/linking-a-03-b-zoomAndPan-expected.txt
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-preserveAspectRatio-expected.png
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-preserveAspectRatio-expected.txt
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-transform-expected.png
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-transform-expected.txt
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-viewBox-expected.png
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-viewBox-transform-expected.png
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-viewTarget-expected.png
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-viewTarget-expected.txt
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-zoomAndPan-expected.png
D third_party/WebKit/LayoutTests/platform/win/svg/custom/linking-a-03-b-zoomAndPan-expected.txt
D third_party/WebKit/LayoutTests/svg/animations/resources/viewspec-animated-viewbox.svg
D third_party/WebKit/LayoutTests/svg/animations/resources/viewspec-checkaspectparams.svg
D third_party/WebKit/LayoutTests/svg/animations/viewspec-animated-viewbox.html
D third_party/WebKit/LayoutTests/svg/animations/viewspec-animated-viewbox-expected.html
D third_party/WebKit/LayoutTests/svg/animations/viewspec-checkaspectparams.html
D third_party/WebKit/LayoutTests/svg/animations/viewspec-checkaspectparams-expected.html
D third_party/WebKit/LayoutTests/svg/as-image/svgview-identity-transform.html
D third_party/WebKit/LayoutTests/svg/as-image/svgview-identity-transform-expected.html
D third_party/WebKit/LayoutTests/svg/as-image/svgview-references.html
D third_party/WebKit/LayoutTests/svg/as-image/svgview-references-expected.html
D third_party/WebKit/LayoutTests/svg/as-image/svgview-references-use-counters.html
D third_party/WebKit/LayoutTests/svg/as-image/svgview-references-use-counters-expected.txt
M third_party/WebKit/LayoutTests/svg/custom/global-constructors-expected.txt
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-all.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-all-expected.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-preserveAspectRatio.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-transform.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-viewBox.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-viewBox-expected.txt
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-viewBox-transform.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-viewBox-transform-expected.txt
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-viewTarget.svg
D third_party/WebKit/LayoutTests/svg/custom/linking-a-03-b-zoomAndPan.svg
M third_party/WebKit/LayoutTests/svg/custom/script-tests/global-constructors.js
D third_party/WebKit/LayoutTests/svg/dom/SVGViewSpec.html
D third_party/WebKit/LayoutTests/svg/dom/SVGViewSpec-defaults.html
D third_party/WebKit/LayoutTests/svg/dom/SVGViewSpec-defaults-expected.txt
D third_party/WebKit/LayoutTests/svg/dom/SVGViewSpec-expected.txt
D third_party/WebKit/LayoutTests/svg/dom/SVGViewSpec-invalid-ref-crash.html
D third_party/WebKit/LayoutTests/svg/dom/SVGViewSpec-invalid-ref-crash-expected.txt
D third_party/WebKit/LayoutTests/svg/dom/complex-svgView-specification.html
D third_party/WebKit/LayoutTests/svg/dom/complex-svgView-specification-expected.html
D third_party/WebKit/LayoutTests/svg/dom/resources/viewspec-aspectparams.svg
D third_party/WebKit/LayoutTests/svg/dom/resources/viewspec-target.svg
D third_party/WebKit/LayoutTests/svg/dom/resources/viewspec-transformparams.svg
D third_party/WebKit/LayoutTests/svg/dom/resources/viewspec-viewboxparams.svg
D third_party/WebKit/LayoutTests/svg/dom/resources/viewspec-viewtargetparams.svg
D third_party/WebKit/LayoutTests/svg/dom/resources/viewspec-zoomandpanparams.svg
D third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGViewSpec.js
D third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGViewSpec-defaults.js
M third_party/WebKit/LayoutTests/svg/dom/svg2-inheritance.html
M third_party/WebKit/LayoutTests/svg/dom/svg2-inheritance-expected.txt
D third_party/WebKit/LayoutTests/svg/dom/viewspec-aspectparams.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-aspectparams-expected.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-transformparams.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-transformparams-expected.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-viewboxparams.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-viewboxparams-expected.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-viewtargetparams.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-viewtargetparams-expected.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-zoomandpanparams.html
D third_party/WebKit/LayoutTests/svg/dom/viewspec-zoomandpanparams-expected.html
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/Source/core/core.gypi
M third_party/WebKit/Source/core/svg/SVGSVGElement.cpp
M third_party/WebKit/Source/core/svg/SVGSVGElement.idl
M third_party/WebKit/Source/core/svg/SVGViewSpec.h
M third_party/WebKit/Source/core/svg/SVGViewSpec.cpp
D third_party/WebKit/Source/core/svg/SVGViewSpec.idl

[f...@opera.com]

https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):

https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp#oldcode661
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:661: if
(fragmentIdentifier.startsWith("svgView(")) {
This is still in the spec AFAIK. the only thing that should be removed
here is:

* the SVGViewSpec interface
* the SVGSVGElement.useCurrentView and SVGSVGElement.currentView

i.e the DOM exposed bits. Any cleanup that isn't obvious "JS only" (like
removing the ScriptWrappable and related bits; *FromJavascript methods;
obviously dead code) is best done as a follow-up (and the removal here
would not be subject to that.)

https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

Ok. Thank you..
I misunderstood "svgView".
I will rework it again.


https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

https://codereview.chromium.org/2163213007/

[foo...@chromium.org]

https://codereview.chromium.org/2163213007/

[foo...@chromium.org]

https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):

https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp#oldcode661
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:661: if
(fragmentIdentifier.startsWith("svgView(")) {
On 2016/07/21 13:08:27, fs wrote:
> This is still in the spec AFAIK. the only thing that should be removed
here is:
>
> * the SVGViewSpec interface
> * the SVGSVGElement.useCurrentView and SVGSVGElement.currentView
>
> i.e the DOM exposed bits. Any cleanup that isn't obvious "JS only"
(like
> removing the ScriptWrappable and related bits; *FromJavascript
methods;
> obviously dead code) is best done as a follow-up (and the removal here
would not
> be subject to that.)

Hmm, so this gets at the same thing I'm wondering about on blink-dev. To
remove only the web-exposed bits of this but keeping all the internals,
has any meaningful simplification been achieved?

I looked around in cs.chromium.org but am not really familiar with any
of the history, so what would happen if SVGViewSpec and SVGViewElement
were removed entirely?

(I added davve@ and pdr@ in case they have thoughts on the matter.)

https://codereview.chromium.org/2163213007/

[f...@opera.com]

https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):

https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp#oldcode661
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:661: if
(fragmentIdentifier.startsWith("svgView(")) {

On 2016/07/22 at 04:44:39, foolip wrote:
> On 2016/07/21 13:08:27, fs wrote:
> > This is still in the spec AFAIK. the only thing that should be
removed here is:
> >
> > * the SVGViewSpec interface
> > * the SVGSVGElement.useCurrentView and SVGSVGElement.currentView
> >
> > i.e the DOM exposed bits. Any cleanup that isn't obvious "JS only"
(like
> > removing the ScriptWrappable and related bits; *FromJavascript
methods;
> > obviously dead code) is best done as a follow-up (and the removal
here would not
> > be subject to that.)
>
> Hmm, so this gets at the same thing I'm wondering about on blink-dev.
To remove only the web-exposed bits of this but keeping all the
internals, has any meaningful simplification been achieved?
>
> I looked around in cs.chromium.org but am not really familiar with any
of the history, so what would happen if SVGViewSpec and SVGViewElement
were removed entirely?

Just of the top of my head, some simplifications we could do:

* Simpler types. Since we no longer need to expose the "exact"
semantics of SVGTransformList et.c, we could use a more efficient (and
thus simpler) internal representation.

* Slightly simpler lifetimes. Oilpan fixed this quite a bit already
(and there are bits that are dead already from that - like
SVGViewSpec::detachContextElement.)

* Removing SVGViewElement probably means we can simplify the setup of
the (now internal-only) SVGViewSpec (less need for things like
inheritViewAttributesFromElement, although removing the SVGViewSpec
interface is likely the bigger help there.)

* With both gone we'd only have a single user of SVGZoomAndPan, and
it's possible we do something with that. (I'd be a bit surprised if
anyone actually used the functionality that is exposes though...)

When you start pulling the strings, it's quite possible more things fall
off as well.

https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

On 2016/07/22 08:31:38, fs (OoO until Aug 15) wrote:
>
https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp
> File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):
>
>
https://codereview.chromium.org/2163213007/diff/1/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp#oldcode661
> third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:661: if
> (fragmentIdentifier.startsWith("svgView(")) {
> On 2016/07/22 at 04:44:39, foolip wrote:
> > On 2016/07/21 13:08:27, fs wrote:
> > > This is still in the spec AFAIK. the only thing that should be removed
here
> is:
> > >
> > > * the SVGViewSpec interface
> > > * the SVGSVGElement.useCurrentView and SVGSVGElement.currentView
> > >
> > > i.e the DOM exposed bits. Any cleanup that isn't obvious "JS only" (like
> > > removing the ScriptWrappable and related bits; *FromJavascript methods;
> > > obviously dead code) is best done as a follow-up (and the removal here
would
> not
> > > be subject to that.)
> >
> > Hmm, so this gets at the same thing I'm wondering about on blink-dev. To
> remove only the web-exposed bits of this but keeping all the internals, has
any
> meaningful simplification been achieved?
> >

> > I looked around in http://cs.chromium.org but am not really familiar with

any of the
> history, so what would happen if SVGViewSpec and SVGViewElement were removed
> entirely?
>
> Just of the top of my head, some simplifications we could do:
>
> * Simpler types. Since we no longer need to expose the "exact" semantics of
> SVGTransformList et.c, we could use a more efficient (and thus simpler)
internal
> representation.
>
> * Slightly simpler lifetimes. Oilpan fixed this quite a bit already (and
there
> are bits that are dead already from that - like
> SVGViewSpec::detachContextElement.)
>
> * Removing SVGViewElement probably means we can simplify the setup of the
(now
> internal-only) SVGViewSpec (less need for things like
> inheritViewAttributesFromElement, although removing the SVGViewSpec interface
is
> likely the bigger help there.)
>
> * With both gone we'd only have a single user of SVGZoomAndPan, and it's
> possible we do something with that. (I'd be a bit surprised if anyone actually
> used the functionality that is exposes though...)
>
> When you start pulling the strings, it's quite possible more things fall off
as
> well.

As the first step, shall I remove the SVGViewSpec interface and the
SVGSVGElement.useCurrentView and SVGSVGElement.currentView as follwing the spec?
And remove deadCode if any.
I will keep #svgView() and SVGViewElement now.

https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

PTAL!!

https://codereview.chromium.org/2163213007/

[f...@opera.com]

SGTM. This CL looks good to me barring that the intent process completes.

https://codereview.chromium.org/2163213007/

[f...@opera.com]

https://codereview.chromium.org/2163213007/diff/40001/third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGViewSpec-defaults.js
File
third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGViewSpec-defaults.js
(left):

https://codereview.chromium.org/2163213007/diff/40001/third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGViewSpec-defaults.js#oldcode1
third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGViewSpec-defaults.js:1:
description("This test checks the SVGViewSpec API");
This test probably has a corresponding -expected.txt file that should be
removed too?

https://codereview.chromium.org/2163213007/diff/40001/third_party/WebKit/Source/core/svg/SVGViewSpec.h
File third_party/WebKit/Source/core/svg/SVGViewSpec.h (right):

https://codereview.chromium.org/2163213007/diff/40001/third_party/WebKit/Source/core/svg/SVGViewSpec.h#newcode42
third_party/WebKit/Source/core/svg/SVGViewSpec.h:42: // JS API
Nit: Drop this comment

https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

Done!!

https://codereview.chromium.org/2163213007/

[foo...@chromium.org]

Can you also remove the entries inUseCounter.h that become unused due to the IDL
file changes? (I'm still trying to figure out what to do in the intent thread,
thanks for your patience...)

https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

On 2016/07/29 02:07:59, foolip wrote:
> Can you also remove the entries inUseCounter.h that become unused due to the
IDL
> file changes? (I'm still trying to figure out what to do in the intent thread,
> thanks for your patience...)

Removed the entries!!
Thank you for your comments!

https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

PTAL!!

I have deprecated instead Remove!!

Thanks

https://codereview.chromium.org/2163213007/

[foo...@chromium.org]

lgtm



https://codereview.chromium.org/2163213007/

[shanm...@samsung.com]

On 2016/08/01 11:41:09, foolip wrote:
> lgtm

Thank you..
Shall I proceed to commit, Since We got enough LGTMs on Blink-dev thread ?

https://codereview.chromium.org/2163213007/

[foo...@chromium.org]

On 2016/08/01 13:22:06, Shanmuga Pandi wrote:
> On 2016/08/01 11:41:09, foolip wrote:
> > lgtm
>
> Thank you..
> Shall I proceed to commit, Since We got enough LGTMs on Blink-dev thread ?

Yes, please go ahead.

https://codereview.chromium.org/2163213007/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2163213007/100001


https://codereview.chromium.org/2163213007/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #6 (id:100001)

https://codereview.chromium.org/2163213007/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 6 (id:??) landed as
https://crrev.com/11cc1db033d902c13c429f8bee0a13b6381c8e4e
Cr-Commit-Position: refs/heads/master@{#408958}

https://codereview.chromium.org/2163213007/