Support deprecation + use counters for constructor attributes.

Support deprecation + use counters for constructor attributes.

If a constructor attribute is annotated with either [MeasureAs=..] or
[DeprecateAs=..], then emit a needed callback wrapper that performs
the required reporting before looking up the constructor.

Use it to correctly handle use counters for the "webkitURL" properties
that both Window and WorkerGlobalScope currently provide; use counters
also added for these here.

(The above change also happens to fix use counter reporting
for the window.WebKitShadowRoot constructor.)

R=nbarth@chromium.org,haraken@chromium.org
BUG=348985
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168576

[sof, 2014-03-04 14:42:51]

Please take a look.

[haraken, 2014-03-04 15:24:11]

How about generating the constructor getter callback always?

For other DOM attributes/methods, we always generate callbacks to insert
UseCounter, TRACE_EVENT, etc.

https://codereview.chromium.org/186673002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/186673002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestObject.idl:61: [MeasureAs=TestFeature]attribute
TestSubObjConstructor TestSubObjMeasured;

Nit: Add one space after ].

[sof, 2014-03-04 15:32:13]

On 2014/03/04 15:24:11, haraken wrote:
> How about generating the constructor getter callback always?
> 
> For other DOM attributes/methods, we always generate callbacks to insert
> UseCounter, TRACE_EVENT, etc.
> 

I think that would be ok; any objections?

[sof, 2014-03-04 15:32:22]

https://codereview.chromium.org/186673002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/186673002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestObject.idl:61: [MeasureAs=TestFeature]attribute
TestSubObjConstructor TestSubObjMeasured;
On 2014/03/04 15:24:11, haraken wrote:
> 
> Nit: Add one space after ].

Done.

[haraken, 2014-03-04 15:35:40]

On 2014/03/04 15:32:13, sof wrote:
> On 2014/03/04 15:24:11, haraken wrote:
> > How about generating the constructor getter callback always?
> > 
> > For other DOM attributes/methods, we always generate callbacks to insert
> > UseCounter, TRACE_EVENT, etc.
> > 
> 
> I think that would be ok; any objections?

I think we just need to make sure that it won't bloat the binary size.

[sof, 2014-03-04 15:53:36]

On 2014/03/04 15:35:40, haraken wrote:
> On 2014/03/04 15:32:13, sof wrote:
> > On 2014/03/04 15:24:11, haraken wrote:
> > > How about generating the constructor getter callback always?
> > > 
> > > For other DOM attributes/methods, we always generate callbacks to insert
> > > UseCounter, TRACE_EVENT, etc.
> > > 
> > 
> > I think that would be ok; any objections?
> 
> I think we just need to make sure that it won't bloat the binary size.

Adds ~200k in a 1.64G debug binary (content_shell)

[sof, 2014-03-04 16:12:27]

On 2014/03/04 15:53:36, sof wrote:
> On 2014/03/04 15:35:40, haraken wrote:
> > On 2014/03/04 15:32:13, sof wrote:
> > > On 2014/03/04 15:24:11, haraken wrote:
> > > > How about generating the constructor getter callback always?
> > > > 
> > > > For other DOM attributes/methods, we always generate callbacks to insert
> > > > UseCounter, TRACE_EVENT, etc.
> > > > 
> > > 
> > > I think that would be ok; any objections?
> > 
> > I think we just need to make sure that it won't bloat the binary size.
> 
> Adds ~200k in a 1.64G debug binary (content_shell)

New patchset provides this.

[haraken, 2014-03-05 00:53:39]

> > > I think that would be ok; any objections?
> > 
> > I think we just need to make sure that it won't bloat the binary size.
> 
> Adds ~200k in a 1.64G debug binary (content_shell)

hmm, it's a surprise to me that just adding the callback indirection increases
200KB. (This implies that we'll be able to reduce the binary size if we could
remove callback indirection in DOM attributes/methods.)

IMHO, 200KB sounds a bit too large.

nbarth@: What do you think?

[sof, 2014-03-05 06:25:01]

On 2014/03/05 00:53:39, haraken wrote:
> > > > I think that would be ok; any objections?
> > > 
> > > I think we just need to make sure that it won't bloat the binary size.
> > 
> > Adds ~200k in a 1.64G debug binary (content_shell)
> 
> hmm, it's a surprise to me that just adding the callback indirection increases
> 200KB. (This implies that we'll be able to reduce the binary size if we could
> remove callback indirection in DOM attributes/methods.)
> 
> IMHO, 200KB sounds a bit too large.
> 
> nbarth@: What do you think?

This was in a debug build.

These identical static code bodies would be link-time ICF[1] optimization
candidates, I'm quite sure.

[1] - e.g. http://research.google.com/pubs/pub36912.html

[haraken, 2014-03-05 06:54:25]

> > > > I think we just need to make sure that it won't bloat the binary size.
> > > 
> > > Adds ~200k in a 1.64G debug binary (content_shell)
> > 
> > hmm, it's a surprise to me that just adding the callback indirection
increases
> > 200KB. (This implies that we'll be able to reduce the binary size if we
could
> > remove callback indirection in DOM attributes/methods.)
> > 
> > IMHO, 200KB sounds a bit too large.
> > 
> > nbarth@: What do you think?
> 
> This was in a debug build.

How much does it affect the binary size of release builds?

[sof, 2014-03-05 06:58:13]

On 2014/03/05 06:54:25, haraken wrote:
> > > > > I think we just need to make sure that it won't bloat the binary size.
> > > > 
> > > > Adds ~200k in a 1.64G debug binary (content_shell)
> > > 
> > > hmm, it's a surprise to me that just adding the callback indirection
> increases
> > > 200KB. (This implies that we'll be able to reduce the binary size if we
> could
> > > remove callback indirection in DOM attributes/methods.)
> > > 
> > > IMHO, 200KB sounds a bit too large.
> > > 
> > > nbarth@: What do you think?
> > 
> > This was in a debug build.
> 
> How much does it affect the binary size of release builds?

I haven't investigated.

[Nils Barth (inactive), 2014-03-05 07:26:40]

On 2014/03/05 00:53:39, haraken wrote:
> > > > I think that would be ok; any objections?
> > > 
> > > I think we just need to make sure that it won't bloat the binary size.
> > 
> > Adds ~200k in a 1.64G debug binary (content_shell)
> 
> IMHO, 200KB sounds a bit too large.
> 
> nbarth@: What do you think?

To concur:
* 200 kB is big enough that it's worth having a flag...
* ...*if* this is present in Release (as Sigbjørn notes).

As a general rule, I'd say:
* We don't care about Debug binary size at all.
* We do care about Release binary size, but we trade that off against:
  * speed (most important) and
  * compiler complexity
* So if a variable in Python/Jinja saves 100 kB in Release, it's worth having,
* ...but if it saves 10 kB, it's not.
* ...and in any case unnecessary code (for simplicity) should have a comment,
like
# Increases binary size: we don't always need this, but it's simpler to always
generate
(This is given ~1.5 MB for bindings: ~10% certainly worth doing, <1% probably
not.)

Does this sound right?


> (This implies that we'll be able to reduce the binary size if we could
> remove callback indirection in DOM attributes/methods.)

Yes, this sounds plausible; I'm guessing the callbacks are "similar but not
identical",
hence aren't being compressed away.
A potential technique would be passing in a function pointer, as the called
function
is the only thing that changes in most of these, so then the callbacks would be
identical.
(That might be too much indirection though.)

[Nils Barth (inactive), 2014-03-05 07:27:51]

Few style points, but otherwise CG and IDL changes LGTM.
(Still need to decide whether to always generate callbacks.)

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:185:
{##############################################################################}
Could you add an extra blank line before this?
(See other comment: 2 blank lines.)

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:186: {% macro
attribute_constructor_callback(attribute, world_suffix) %}
This macro looks fine;
there's overlap with the getter_callback,
but it's clearer to have a separate one,
instead of cramming them together.

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (left):

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/interface.cpp:77: 
Could you leave this line?
The convention I've been using (and will write up) is:
2 blank lines between blocks/macros,
just as with Python top-level functions.

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/interface.cpp:10:
'%sV8Internal::%sConstructorGetterCallback' %
Minor point:
could we give these the same name?
I.e., in both cases use:
'%sV8Internal::%sAttributeGetterCallback'
?
This shouldn't cause overlap, as we're generating these in mutually exclusive
loops.
Might be simpler/cleaner, or might cause confusion.

[Nils Barth (inactive), 2014-03-05 07:34:01]

On 2014/03/05 07:27:51, Nils Barth wrote:
> Few style points, but otherwise CG and IDL changes LGTM.
> (Still need to decide whether to always generate callbacks.)

Sorry, 'ok' this was a bit premature, since this is a pure CG change;
deciding whether to always generate callbacks is question to answer first.
(I'm used to being CC'ed on Blink CLs that just touch on CG+IDL.)
(Anyway still need core owner = haraken.)

[sof, 2014-03-05 11:04:25]

On 2014/03/05 06:54:25, haraken wrote:
> > > > > I think we just need to make sure that it won't bloat the binary size.
> > > > 
> > > > Adds ~200k in a 1.64G debug binary (content_shell)
> > > 
> > > hmm, it's a surprise to me that just adding the callback indirection
> increases
> > > 200KB. (This implies that we'll be able to reduce the binary size if we
> could
> > > remove callback indirection in DOM attributes/methods.)
> > > 
> > > IMHO, 200KB sounds a bit too large.
> > > 
> > > nbarth@: What do you think?
> > 
> > This was in a debug build.
> 
> How much does it affect the binary size of release builds?

With manually enabling icf (icf=safe) and the gold linker, I see an 150k
increase in release build size. (e.g., when building out/Release/content_shell.)

I'm quite sure that a full release build and MSVC would do better, but I don't
know how to trigger such full chrome release builds (or know what options they
do use here.)

If you want to push for using callback wrappers all the time, perhaps handle
that separately?

[Nils Barth (inactive), 2014-03-05 11:56:54]

On 2014/03/05 11:04:25, sof wrote:
> With manually enabling icf (icf=safe) and the gold linker, I see an 150k
> increase in release build size. (e.g., when building
out/Release/content_shell.)
> 
> I'm quite sure that a full release build and MSVC would do better, but I don't
> know how to trigger such full chrome release builds (or know what options they
> do use here.)

Understood, and in any case we're most concerned about binary size on mobile,
but that's a bit involved to build.


> If you want to push for using callback wrappers all the time, perhaps handle
> that separately?

Agreed; sounds like there's enough evidence that it causes a non-negligible
increase in binary size, so it's worth the complexity to not have them.
(i.e., could you go back to only making them when needed?)

Thanks for checking this out!

[sof, 2014-03-05 12:45:48]

Thanks much for the detailed review & comments.

Dropped back to the selective generation of constructor callbacks + addressed
the review comments.

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:185:
{##############################################################################}
On 2014/03/05 07:27:52, Nils Barth wrote:
> Could you add an extra blank line before this?
> (See other comment: 2 blank lines.)

Done.

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (left):

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/interface.cpp:77: 
On 2014/03/05 07:27:52, Nils Barth wrote:
> Could you leave this line?
> The convention I've been using (and will write up) is:
> 2 blank lines between blocks/macros,
> just as with Python top-level functions.

Accidental line removal reverted.

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/186673002/diff/30001/Source/bindings/template...
Source/bindings/templates/interface.cpp:10:
'%sV8Internal::%sConstructorGetterCallback' %
On 2014/03/05 07:27:52, Nils Barth wrote:
> Minor point:
> could we give these the same name?
> I.e., in both cases use:
> '%sV8Internal::%sAttributeGetterCallback'
> ?
> This shouldn't cause overlap, as we're generating these in mutually exclusive
> loops.
> Might be simpler/cleaner, or might cause confusion.

Switched to using "AttributeGetterCallback" as suffix for these also.

[haraken, 2014-03-05 14:34:56]

LGTM

Though I'm a bit surprised that generating the constructor getter callbacks all
the time increases the binary size so much. I think that constructor getters
would be used only in Window and WorkerGlobalScope...

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:403:
contents['needs_constructor_callback'] = contents['measure_as'] or
contents['deprecate_as']

needs_constructor_callback => needs_constructor_getter_callback

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:187: {% macro
attribute_constructor_callback(attribute, world_suffix) %}

attribute_constructor_callback => constructor_getter_callback ?

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:189: static void
{{attribute.name}}AttributeGetterCallback{{world_suffix}}(v8::Local<v8::String>
property, const v8::PropertyCallbackInfo<v8::Value>& info)

AttributeGetterCallback => ConstructorGetterCallback

[sof, 2014-03-05 14:56:45]

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:403:
contents['needs_constructor_callback'] = contents['measure_as'] or
contents['deprecate_as']
On 2014/03/05 14:34:57, haraken wrote:
> 
> needs_constructor_callback => needs_constructor_getter_callback

Done.

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:187: {% macro
attribute_constructor_callback(attribute, world_suffix) %}
On 2014/03/05 14:34:57, haraken wrote:
> 
> attribute_constructor_callback => constructor_getter_callback ?

Done.

https://codereview.chromium.org/186673002/diff/70001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:189: static void
{{attribute.name}}AttributeGetterCallback{{world_suffix}}(v8::Local<v8::String>
property, const v8::PropertyCallbackInfo<v8::Value>& info)
On 2014/03/05 14:34:57, haraken wrote:
> 
> AttributeGetterCallback => ConstructorGetterCallback

Done.

[sof, 2014-03-05 14:56:50]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-05 14:57:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/186673002/90001

[commit-bot: I haz the power, 2014-03-05 20:59:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/186673002/90001

[commit-bot: I haz the power, 2014-03-06 05:51:26]

Change committed as 168576