Experimental SVG 1.1 DOM runtime flag.

Experimental SVG 1.1 DOM runtime flag.

This adds UseCounters for SVG DOM, to see how much usage there is.

SVG DOM support is enabled by default, but it will be possible to disable it with a commandline flag (--disable-svg1dom), see https://codereview.chromium.org/577943002.

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

[Erik Dahlström (inactive), 2014-09-17 11:19:17]

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

[fs, 2014-09-17 13:04:31]

Should probably mention the split out SVGPathSegDOM counter in the
description/commit message.

I also wonder if:

[MeasureAs=Foo] const unsigned short bar;

actually works. If it's a no-op though I guess it's no biggy to let it be this
way. Should find out how/if it works though so we know.

https://codereview.chromium.org/474013004/diff/60001/Source/core/svg/SVGPathE...
File Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/474013004/diff/60001/Source/core/svg/SVGPathE...
Source/core/svg/SVGPathElement.idl:36: [RuntimeEnabled=SVG1DOM,
MeasureAs=SVG1DOM] SVGPathSegClosePath createSVGPathSegClosePath();
Should these - createSVGPathSeg* - be measured as SVGPathSegDOM instead?
(Because of their strong ties to SVGPathSegList and friends.)

[Mike West, 2014-09-17 13:10:01]

On 2014/09/17 13:04:31, fs wrote:
> I also wonder if:
> 
> [MeasureAs=Foo] const unsigned short bar;
> 
> actually works. If it's a no-op though I guess it's no biggy to let it be this
> way. Should find out how/if it works though so we know.

MeasureAs gets parsed when generating the bindings; the accessor method pokes at
the execution context's UseCounter.

See `Source/bindings/tests/idls/core/TestObject.idl` and
`Source/bindings/tests/results/core/V8TestObject.cpp` for the results. If it's
not working on attributes, it's a bug. :)

[Mike West, 2014-09-17 13:10:36]

On 2014/09/17 13:10:01, Mike West wrote:
> On 2014/09/17 13:04:31, fs wrote:
> > I also wonder if:
> > 
> > [MeasureAs=Foo] const unsigned short bar;
> > 
> > actually works. If it's a no-op though I guess it's no biggy to let it be
this
> > way. Should find out how/if it works though so we know.
> 
> MeasureAs gets parsed when generating the bindings; the accessor method pokes
at
> the execution context's UseCounter.
> 
> See `Source/bindings/tests/idls/core/TestObject.idl` and
> `Source/bindings/tests/results/core/V8TestObject.cpp` for the results. If it's
> not working on attributes, it's a bug. :)

(This CL LGTM, by the way. You'll still need a platform/ and web/ and public/web
OWNER to stamp it, though.)

[fs, 2014-09-17 13:18:25]

On 2014/09/17 13:10:01, Mike West wrote:
> On 2014/09/17 13:04:31, fs wrote:
> > I also wonder if:
> > 
> > [MeasureAs=Foo] const unsigned short bar;
> > 
> > actually works. If it's a no-op though I guess it's no biggy to let it be
this
> > way. Should find out how/if it works though so we know.
> 
> MeasureAs gets parsed when generating the bindings; the accessor method pokes
at
> the execution context's UseCounter.
> 
> See `Source/bindings/tests/idls/core/TestObject.idl` and
> `Source/bindings/tests/results/core/V8TestObject.cpp` for the results. If it's
> not working on attributes, it's a bug. :)

These are "IDL constants" though, which AFAIK take a different code-path in the
bindings generator (calling V8DOMConfiguration::installConstants, which just
install them on object without accessor methods.)

[Mike West, 2014-09-17 13:27:24]

On 2014/09/17 13:18:25, fs wrote:
> These are "IDL constants" though, which AFAIK take a different code-path in
the
> bindings generator (calling V8DOMConfiguration::installConstants, which just
> install them on object without accessor methods.)

Sorry, you're right. I thought you were referring to attributes. No. Constants
don't have accessor methods, and will be difficult to measure. Perhaps nbarth@
has some ideas?

[kouhei (in TOK), 2014-09-17 23:44:48]

Would it be possible to use a separate counter for
animatedtransformlist.baseval?
d3js is using SVGDOM only for this method afaik.
https://github.com/mbostock/d3/blob/48ad44fdeef32b518c6271bb99a9aed376c1a1d6/...

[Erik Dahlström (inactive), 2014-09-18 07:23:03]

On 2014/09/17 23:44:48, kouhei wrote:
> Would it be possible to use a separate counter for
> animatedtransformlist.baseval?
> d3js is using SVGDOM only for this method afaik.
>
https://github.com/mbostock/d3/blob/48ad44fdeef32b518c6271bb99a9aed376c1a1d6/...

Np, I'll add that. I do think that we want to try to get rid of all of
baseVal/animVal though, it would be sad if ended up with keeping baseVal just
because of that one.

[Erik Dahlström (inactive), 2014-09-18 14:10:19]

ed@opera.com changed reviewers:
+ abarth@chromium.org

[Erik Dahlström (inactive), 2014-09-18 14:42:57]

ed@opera.com changed reviewers:
+ nbarth@chromium.org

[Erik Dahlström (inactive), 2014-09-19 09:12:36]

Awaiting reviews for Source/platform/, public/web and Source/web.

https://codereview.chromium.org/474013004/diff/60001/Source/core/svg/SVGPathE...
File Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/474013004/diff/60001/Source/core/svg/SVGPathE...
Source/core/svg/SVGPathElement.idl:36: [RuntimeEnabled=SVG1DOM,
MeasureAs=SVG1DOM] SVGPathSegClosePath createSVGPathSegClosePath();
On 2014/09/17 13:04:31, fs wrote:
> Should these - createSVGPathSeg* - be measured as SVGPathSegDOM instead?
> (Because of their strong ties to SVGPathSegList and friends.)

Done.

[Nils Barth (inactive), 2014-09-22 16:24:36]

On 2014/09/17 13:27:24, Mike West (OOO until 24th) wrote:
> Sorry, you're right. I thought you were referring to attributes. No. Constants
> don't have accessor methods, and will be difficult to measure. Perhaps nbarth@
> has some ideas?

Hi Mike,
correct, constants can't currently be measured;
this is Bug 358506
http://crbug.com/358506

1. As a rule, we prefer to *not* include nop extended attributes.
(B/c looking at the IDL suggests that they *are* doing something.)
Instead, I'd suggest omitting the ExtAttr and having a FIXME on the previous
line, as in:
// FIXME: should be [MeasureAs=Foo] but [MeasureAs] not supported on constants:
http://crbug.com/358506
const unsigned short bar;

I've updated the docs accordingly:
http://chromium.org/blink/webidl/blink-idl-extended-attributes

2. Adding support for measuring constants is non-trivial, but not conceptually
hard:
just generate accessor for these constants, as we do for attributes.
bashi or Jens would be able to do this, but don't know about their priorities;
this is at least the second time this has come up.
(More discussion at the bug.)

[Erik Dahlström (inactive), 2014-09-23 11:00:33]

ed@opera.com changed reviewers:
+ dglazkov@chromium.org
- nbarth@chromium.org

[Erik Dahlström (inactive), 2014-09-23 11:04:02]

On 2014/09/22 16:24:36, Nils Barth (inactive) wrote:
> On 2014/09/17 13:27:24, Mike West (OOO until 24th) wrote:
> > Sorry, you're right. I thought you were referring to attributes. No.
Constants
> > don't have accessor methods, and will be difficult to measure. Perhaps
nbarth@
> > has some ideas?
> 
> Hi Mike,
> correct, constants can't currently be measured;
> this is Bug 358506
> http://crbug.com/358506
> 
> 1. As a rule, we prefer to *not* include nop extended attributes.
> (B/c looking at the IDL suggests that they *are* doing something.)
> Instead, I'd suggest omitting the ExtAttr and having a FIXME on the previous
> line, as in:
> // FIXME: should be [MeasureAs=Foo] but [MeasureAs] not supported on
constants:
> http://crbug.com/358506
> const unsigned short bar;
> 
> I've updated the docs accordingly:
> http://chromium.org/blink/webidl/blink-idl-extended-attributes
> 
> 2. Adding support for measuring constants is non-trivial, but not conceptually
> hard:
> just generate accessor for these constants, as we do for attributes.
> bashi or Jens would be able to do this, but don't know about their priorities;
> this is at least the second time this has come up.
> (More discussion at the bug.)

Looks like http://crbug.com/358506 is progressing nicely, so I posted patchset
#8 to add MeasureAs for the constants again.

Still waiting for reviews for: Source/platform/, public/web and Source/web.

[Erik Dahlström (inactive), 2014-10-16 14:18:42]

ed@opera.com changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[Erik Dahlström (inactive), 2014-10-16 14:22:06]

On 2014/09/23 11:04:02, Erik Dahlström wrote:
> On 2014/09/22 16:24:36, Nils Barth (inactive) wrote:
> > On 2014/09/17 13:27:24, Mike West (OOO until 24th) wrote:
> > > Sorry, you're right. I thought you were referring to attributes. No.
> Constants
> > > don't have accessor methods, and will be difficult to measure. Perhaps
> nbarth@
> > > has some ideas?
> > 
> > Hi Mike,
> > correct, constants can't currently be measured;
> > this is Bug 358506
> > http://crbug.com/358506
> > 
> > 1. As a rule, we prefer to *not* include nop extended attributes.
> > (B/c looking at the IDL suggests that they *are* doing something.)
> > Instead, I'd suggest omitting the ExtAttr and having a FIXME on the previous
> > line, as in:
> > // FIXME: should be [MeasureAs=Foo] but [MeasureAs] not supported on
> constants:
> > http://crbug.com/358506
> > const unsigned short bar;
> > 
> > I've updated the docs accordingly:
> > http://chromium.org/blink/webidl/blink-idl-extended-attributes
> > 
> > 2. Adding support for measuring constants is non-trivial, but not
conceptually
> > hard:
> > just generate accessor for these constants, as we do for attributes.
> > bashi or Jens would be able to do this, but don't know about their
priorities;
> > this is at least the second time this has come up.
> > (More discussion at the bug.)
> 
> Looks like http://crbug.com/358506 is progressing nicely, so I posted patchset
> #8 to add MeasureAs for the constants again.
> 
> Still waiting for reviews for: Source/platform/, public/web and Source/web.

Ping - @mkwst I see that you've recently been added as owner of
Source/platform/, public/web and Source/web, please have another look at this.

[Mike West, 2014-10-16 15:12:05]

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

[Mike West, 2014-10-16 15:12:07]

Can you rebase this onto ToT? I'm happy to take another look once the bots are
happy.

[Erik Dahlström (inactive), 2014-10-17 10:26:38]

On 2014/10/16 15:12:07, Mike West wrote:
> Can you rebase this onto ToT? I'm happy to take another look once the bots are
> happy.

Done, the linux_blink_dbg failures look like (unrelated) flakes to me.

[Mike West, 2014-10-17 10:41:04]

LGTM.

Once you have numbers, please do bring this to blink-dev@ for discussion.

[Erik Dahlström (inactive), 2014-10-17 10:48:10]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-10-17 10:48:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/474013004/160001

[commit-bot: I haz the power, 2014-10-17 11:57:33]

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

[commit-bot: I haz the power, 2014-10-17 11:57:35]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 538.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
1f39a73328f72979893d58e365169ae498def3ad..1ed7dfc0b61374748c838a3a7859b1ee54f1b0b7
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -538,6 +538,11 @@ public:
         CSSXGetComputedStyleQueries = 566,
         QuotedAnimationName = 567,
         QuotedKeyframesRule = 568,
+        SVG1DOM = 569,
+        SVGPathSegDOM = 570,
+        SVGTransformListConsolidate = 571,
+        SVGAnimatedTransformListBaseVal = 572,
+
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         // Also, run update_use_counter_feature_enum.py in
chromium/src/tools/metrics/histograms/

[commit-bot: I haz the power, 2014-10-17 11:57:47]

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

[Erik Dahlström (inactive), 2014-10-17 12:28:22]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-10-17 12:29:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/474013004/180001

[Erik Dahlström (inactive), 2014-10-17 13:31:51]

The CQ bit was unchecked by ed@opera.com

[Erik Dahlström (inactive), 2014-10-17 13:42:47]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-10-17 13:43:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/474013004/200001

[commit-bot: I haz the power, 2014-10-17 16:05:11]

Committed patchset #11 (id:200001) as 183898