Bug 96837 – We should make collecting metrics easier by adding an IDL attribute

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

We should make collecting metrics easier by adding an IDL attribute

[Adam Barth]

Created attachment 164243 [details]
Work in progress

[Adam Barth]

Created attachment 164244 [details]
Work in progress

[Eric Seidel (no email)]

I support this product and/or service.  Want to buy.

[Adam Barth]

Created attachment 164249 [details]
Patch

[Ojan Vafai]

Comment on attachment 164249 [details]
Patch

Non-bindings bits LGTM. Probably someone more familiar with V8 bindings should review that part though.

[Adam Barth]

Maybe japhet or dglazkov?  I believe haraken in on vacation.

[Kentaro Hara]

Comment on attachment 164249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164249&action=review

The V8 binding LGTM. Do you have any plan to support [MeasureAs] in JSC?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1828
> +END
> +    push(@implContent, GenerateFeatureObservation($function->signature->extendedAttributes->{"MeasureAs"}));
> +
> +    push(@implContent, <<END);

Nit: For readability, shall we avoid the END? You can store the result of GenerateFeatureObservation() to some variable before line 1822 and use the variable here.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2023
> +END
> +    push(@implContent, GenerateFeatureObservation($function->signature->extendedAttributes->{"MeasureAs"}));
> +
> +    push(@implContent, <<END);

Ditto.

[Ojan Vafai]

Comment on attachment 164249 [details]
Patch

Bikeshed: MeasureAs is a confusing term to me. MeasureUsage maybe?

[Ojan Vafai]

Comment on attachment 164249 [details]
Patch

Oh, never mind. I see now that it takes a name to measure as. Carry on. :)

[Adam Barth]

> The V8 binding LGTM. Do you have any plan to support [MeasureAs] in JSC?

Currently Chromium is the only port that has a backend for Histograms.  It doesn't seem worth supporting in JSC since it will just be overhead at this point.

[Kentaro Hara]

(In reply to comment #10)
> > The V8 binding LGTM. Do you have any plan to support [MeasureAs] in JSC?
> 
> Currently Chromium is the only port that has a backend for Histograms.  It doesn't seem worth supporting in JSC since it will just be overhead at this point.

Got it. Then [V8MeasureAs] might be a better name.

[Adam Barth]

Created attachment 164422 [details]
Patch for landing

[Adam Barth]

Comments addressed!  Thanks.

[WebKit Review Bot]

Comment on attachment 164422 [details]
Patch for landing

Clearing flags on attachment: 164422

Committed r128788: <http://trac.webkit.org/changeset/128788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.