Add support for [NoInterfaceObject] extended attribute to bindings generator

Add support for [NoInterfaceObject] extended attribute to bindings generator

*Constructor attributes on the global DOMWindow object are now automatically
generated for each non-callback interface that is not declared with the
[NoInterfaceObject] extended attribute. It is thus no longer needed for
developers to manually add *Constructor attributes for their new interfaces
unless they wish to register an "extra" constructor for their interface.

This patch does not modify at all the behavior on JS side. To achieve this,
[NoInterfaceObject] extended attribute is added to interfaces that should
not have it according to specification (e.g. History, Location, ...).
This can be improved later as the behavior should not change with this
patch.
    
BUG=233578
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149858

[haraken, 2013-04-22 11:44:50]

The patch looks really great!!

- Due to existing typos in DOMWindow.idl, there will be a couple of IDL
interfaces whose constructor names/behavior are changed by this change (e.g.
WebKitMediaSource, WebKitSourceBufferList, Location). Would you list up the
interfaces?

- Actually I don't much like the approach that opens all IDL files in
GetAllInterfaces(). One problem is performance (although I don't know how much
your change slows down builds). Another problem is that your change assumes that
DOMWindow.idl depends on all other IDL files even though the dependency is not
written in Makefile. For example, when you add [NoInterfaceObject] to File.idl,
DOMWindow.idl won't be rebuilt. One way to solve the problems would be to
resolve the [NoInterfaceObject] IDL attribute in the phase where we resolve
[Supplemental] IDL attribute. (See preprocess-idls.pl for more details.) At
least, we have to make sure that DOMWindow.idl is rebuilt when any IDL file is
touched.

https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGenerator.pm (right):

https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGenerator.pm:268: unless ($didCacheAllInterfaces) {
GetAllInterfaces() is called only once (i.e. from DOMWindow), so it wouldn't be
helpful to cache it, right?

https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/IDLPa...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/IDLPa...
Source/bindings/scripts/IDLParser.pm:827: die "typedef redefinition for " .
$name . " at " . $self->{Line} if (exists $typedefs{$name} && $typedef->type ne
$typedefs{$name}->type);
Nit: What is this change for?

[do-not-use, 2013-04-22 12:15:20]

On 2013/04/22 11:44:50, haraken wrote:
> The patch looks really great!!
> 
> - Due to existing typos in DOMWindow.idl, there will be a couple of IDL
> interfaces whose constructor names/behavior are changed by this change (e.g.
> WebKitMediaSource, WebKitSourceBufferList, Location). Would you list up the
> interfaces?

Will do.
 
> - Actually I don't much like the approach that opens all IDL files in
> GetAllInterfaces(). One problem is performance (although I don't know how much
> your change slows down builds). Another problem is that your change assumes
that
> DOMWindow.idl depends on all other IDL files even though the dependency is not
> written in Makefile. For example, when you add [NoInterfaceObject] to
File.idl,
> DOMWindow.idl won't be rebuilt. One way to solve the problems would be to
> resolve the [NoInterfaceObject] IDL attribute in the phase where we resolve
> [Supplemental] IDL attribute. (See preprocess-idls.pl for more details.) At
> least, we have to make sure that DOMWindow.idl is rebuilt when any IDL file is
> touched.

I understand this is not optimal. The current approach is a bit "brute" but I
was hoping to optimize later on as this patch is a pain to keep up-to-date with
upstream. Speed is not a big issue here as it will do the extra work only once,
when parsing DOMWindow.idl. I don't see much impact on my machine but it is of
course slower.

I will see what I can do to make sure to resolve NoInterfaceObject IDL attribute
beforehand (ala Supplemental).

> 
>
https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/CodeG...
> File Source/bindings/scripts/CodeGenerator.pm (right):
> 
>
https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/CodeG...
> Source/bindings/scripts/CodeGenerator.pm:268: unless ($didCacheAllInterfaces)
{
> GetAllInterfaces() is called only once (i.e. from DOMWindow), so it wouldn't
be
> helpful to cache it, right?

Right. I was planning to call it several times and then ended up optimizing. I
will get rid of this boolean.

>
https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/IDLPa...
> File Source/bindings/scripts/IDLParser.pm (right):
> 
>
https://codereview.chromium.org/14179013/diff/1/Source/bindings/scripts/IDLPa...
> Source/bindings/scripts/IDLParser.pm:827: die "typedef redefinition for " .
> $name . " at " . $self->{Line} if (exists $typedefs{$name} && $typedef->type
ne
> $typedefs{$name}->type);
> Nit: What is this change for?

This is for GLenum typedef that is defined twice, once in
Source/core/html/canvas/EXTDrawBuffers.idl and a second time in
Source/core/html/canvas/WebGLRenderingContext.idl.

[haraken, 2013-04-22 12:27:48]

At least, I'd like to guarantee correctness of the change (i.e. we want to
resolve the dependency problem).

In your approach, you have to guarantee that DOMWindow.idl is rebuilt when any
IDL file is changed. Can you add the logic to preprocess-idls.pl or somewhere
else?

(Also please keep in mind that these days we are sensitive to build performance
around IDL files (especially in Windows).)

[arv (Not doing code reviews), 2013-04-22 14:55:05]

This is a great change.

My only concern is how this will interact with workers. Right now you added
NoInterfaceObject to all the worker idl files. I don't think the specs for these
have NoInterfaceObject in them so it is likely that we will accidentally expose
a worker only interface to Window which will most likely cause issues/crashes.

[do-not-use, 2013-04-22 15:01:28]

On 2013/04/22 14:55:05, arv wrote:
> This is a great change.
> 
> My only concern is how this will interact with workers. Right now you added
> NoInterfaceObject to all the worker idl files. I don't think the specs for
these
> have NoInterfaceObject in them so it is likely that we will accidentally
expose
> a worker only interface to Window which will most likely cause issues/crashes.

I don't understand (yet) why my change would interact with workers. Before my
patch, Worker and SharedWorker were exposed on the DOMWindow object. After my
patch, this has not changed. My patch currently adds [NoInterfaceObject]
extended attributes to all interfaces that previously did not have an attribute
in DOMWindow.idl. This may not be according to their respective specifications
(For e.g. History) but at least my patch will not change the previous behavior.

We can see if NoInterfaceObject should be removed from some interfaces later on.
For this patch, I'm trying to not cause any behavior change.

[do-not-use, 2013-04-22 15:25:36]

On 2013/04/22 12:27:48, haraken wrote:
> At least, I'd like to guarantee correctness of the change (i.e. we want to
> resolve the dependency problem).
> 
> In your approach, you have to guarantee that DOMWindow.idl is rebuilt when any
> IDL file is changed. Can you add the logic to preprocess-idls.pl or somewhere
> else?
> 
> (Also please keep in mind that these days we are sensitive to build
performance
> around IDL files (especially in Windows).)

I tweaked Source/bindings/derived_sources.gyp to make sure DOMWindow.idl is
reprocessed if any idl file changes. It works but this may not be the best way
to do it (also I don't know gyp well).

[haraken, 2013-04-22 15:31:28]

Doesn't this mean that DOMWindow.idl is always rebuilt and it scans all IDL
files, even if there is no change in IDL files?

[do-not-use, 2013-04-22 15:35:53]

On 2013/04/22 15:31:28, haraken wrote:
> Doesn't this mean that DOMWindow.idl is always rebuilt and it scans all IDL
> files, even if there is no change in IDL files?

Based on my local testing, DOMWindow.idl is rebuilt *only* is an IDL file was
changed.

[do-not-use, 2013-04-22 15:39:10]

https://codereview.chromium.org/14179013/diff/7001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14179013/diff/7001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:99: 'idl_files_list': '<|(idl_files_list.tmp
<@(idl_files))',
I would need to get rid of DOMWindow.idl in idl_files list. I tried using
"Exclusing Lists" [1], but I cannot seem to get it right.

Otherwise, we see such warning when building:
Circular third_party/WebKit/Source/core/page/DOMWindow.idl <-
third_party/WebKit/Source/core/page/DOMWindow.idl dependency dropped.

[1] https://code.google.com/p/gyp/wiki/InputFormatReference#Exclusion_Lists_(!)

[haraken, 2013-04-22 15:41:00]

thakis, tony: Would this change be OK from the perspective of build performance?

What we want to do is:

- We need to rebuild DOMWindow.idl when any IDL file is updated.

- When we build DOMWindow.idl, we open all IDL files and check which IDL files
contain [NoInterfaceObject] IDL attribute.

[do-not-use, 2013-04-22 15:47:19]

https://codereview.chromium.org/14179013/diff/7001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14179013/diff/7001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:99: 'idl_files_list': '<|(idl_files_list.tmp
<@(idl_files))',
On 2013/04/22 15:39:11, cdumez wrote:
> I would need to get rid of DOMWindow.idl in idl_files list. I tried using
> "Exclusing Lists" [1], but I cannot seem to get it right.
> 
> Otherwise, we see such warning when building:
> Circular third_party/WebKit/Source/core/page/DOMWindow.idl <-
> third_party/WebKit/Source/core/page/DOMWindow.idl dependency dropped.
> 
> [1]
https://code.google.com/p/gyp/wiki/InputFormatReference#Exclusion_Lists_%28%21)

I tried the following:
http://pastebin.com/kJs4aAT1

However it fails with:
ValueError: idl_files_list.tmp <@(other_idl_files) key _other_idl_files must be
list, not list when applying regex while loading dependencies of
third_party/WebKit/Source/core/core.gyp/core.gyp

[Nico, 2013-04-22 17:10:35]

Since you're asking about build performance: What's the build performance impact
of this? How much slower do things get?

https://codereview.chromium.org/14179013/diff/4002/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14179013/diff/4002/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:90: # extended attribute set.
This looks like it's emulating a build system in gyp. Can't you just make all
idl files inputs of the edge the generates DOMWindow.idl?

[do-not-use, 2013-04-22 17:36:23]

On 2013/04/22 17:10:35, Nico wrote:
> Since you're asking about build performance: What's the build performance
impact
> of this? How much slower do things get?
> 
>
https://codereview.chromium.org/14179013/diff/4002/Source/bindings/derived_so...
> File Source/bindings/derived_sources.gyp (right):
> 
>
https://codereview.chromium.org/14179013/diff/4002/Source/bindings/derived_so...
> Source/bindings/derived_sources.gyp:90: # extended attribute set.
> This looks like it's emulating a build system in gyp. Can't you just make all
> idl files inputs of the edge the generates DOMWindow.idl?

Processing DOMWindow.idl takes 8 more seconds on my machine (8 wallclock secs (
2.96 usr  0.42 sys +  0.02 cusr  0.02 csys =  3.42 CPU) using perl Benchmark).
There is no impact for other idl files.

Note that I don't know yet how to make DOMWindow.idl depend on other IDL files
in gyp (except by using touch as I did).

[do-not-use, 2013-04-22 19:46:26]

On 2013/04/22 17:36:23, cdumez wrote:
> On 2013/04/22 17:10:35, Nico wrote:
> > Since you're asking about build performance: What's the build performance
> impact
> > of this? How much slower do things get?
> > 
> >
>
https://codereview.chromium.org/14179013/diff/4002/Source/bindings/derived_so...
> > File Source/bindings/derived_sources.gyp (right):
> > 
> >
>
https://codereview.chromium.org/14179013/diff/4002/Source/bindings/derived_so...
> > Source/bindings/derived_sources.gyp:90: # extended attribute set.
> > This looks like it's emulating a build system in gyp. Can't you just make
all
> > idl files inputs of the edge the generates DOMWindow.idl?
> 
> Processing DOMWindow.idl takes 8 more seconds on my machine (8 wallclock secs
(
> 2.96 usr  0.42 sys +  0.02 cusr  0.02 csys =  3.42 CPU) using perl Benchmark).
> There is no impact for other idl files.
> 
> Note that I don't know yet how to make DOMWindow.idl depend on other IDL files
> in gyp (except by using touch as I did).

Down to 6 extra seconds by not parsing interfaces with NoInterfaceObjects when
processing DOMWindow.idl.

[Nico, 2013-04-22 19:49:44]

6s sounds fairly long to me, and if I was working on that code I'd be annoyed if
I had to wait that long after touching that code (6s is enough for check
dependencies, compile, and link after a change in UI land, and that's already a
bit slow). I don't work on that code however, and on a full build scale it's not
too bad.

Dependency tracking is however something that should be done by the build system
and not manually in gyp. If you list inputs as inputs explicitly, gyp should do
the right thing. There's some gyp documentation on the gyp wiki pages, look for
actions and rules examples.

[do-not-use, 2013-04-23 07:33:05]

On 2013/04/22 19:49:44, Nico wrote:
> 6s sounds fairly long to me, and if I was working on that code I'd be annoyed
if
> I had to wait that long after touching that code (6s is enough for check
> dependencies, compile, and link after a change in UI land, and that's already
a
> bit slow). I don't work on that code however, and on a full build scale it's
not
> too bad.
> 
> Dependency tracking is however something that should be done by the build
system
> and not manually in gyp. If you list inputs as inputs explicitly, gyp should
do
> the right thing. There's some gyp documentation on the gyp wiki pages, look
for
> actions and rules examples.

Ok, I have updated my patch to stop using 'touch' to cause DOMWindow.idl to be
reprocessed. Instead, I have added a new 'bindings_derived_sources_domwindow'
target with a rule that has all IDL files as inputs. Hopefully this is closer to
what Nico had in mind.

[do-not-use, 2013-04-23 12:10:54]

On 2013/04/23 07:33:05, cdumez wrote:
> On 2013/04/22 19:49:44, Nico wrote:
> > 6s sounds fairly long to me, and if I was working on that code I'd be
annoyed
> if
> > I had to wait that long after touching that code (6s is enough for check
> > dependencies, compile, and link after a change in UI land, and that's
already
> a
> > bit slow). I don't work on that code however, and on a full build scale it's
> not
> > too bad.
> > 
> > Dependency tracking is however something that should be done by the build
> system
> > and not manually in gyp. If you list inputs as inputs explicitly, gyp should
> do
> > the right thing. There's some gyp documentation on the gyp wiki pages, look
> for
> > actions and rules examples.
> 
> Ok, I have updated my patch to stop using 'touch' to cause DOMWindow.idl to be
> reprocessed. Instead, I have added a new 'bindings_derived_sources_domwindow'
> target with a rule that has all IDL files as inputs. Hopefully this is closer
to
> what Nico had in mind.

I'm working on making DOMWindow.idl processing faster. Also, Callback interfaces
should not need NoInterface object.

I'll update the patch soon.

[do-not-use, 2013-04-23 13:39:37]

On 2013/04/23 12:10:54, cdumez wrote:
> On 2013/04/23 07:33:05, cdumez wrote:
> > On 2013/04/22 19:49:44, Nico wrote:
> > > 6s sounds fairly long to me, and if I was working on that code I'd be
> annoyed
> > if
> > > I had to wait that long after touching that code (6s is enough for check
> > > dependencies, compile, and link after a change in UI land, and that's
> already
> > a
> > > bit slow). I don't work on that code however, and on a full build scale
it's
> > not
> > > too bad.
> > > 
> > > Dependency tracking is however something that should be done by the build
> > system
> > > and not manually in gyp. If you list inputs as inputs explicitly, gyp
should
> > do
> > > the right thing. There's some gyp documentation on the gyp wiki pages,
look
> > for
> > > actions and rules examples.
> > 
> > Ok, I have updated my patch to stop using 'touch' to cause DOMWindow.idl to
be
> > reprocessed. Instead, I have added a new
'bindings_derived_sources_domwindow'
> > target with a rule that has all IDL files as inputs. Hopefully this is
closer
> to
> > what Nico had in mind.
> 
> I'm working on making DOMWindow.idl processing faster. Also, Callback
interfaces
> should not need NoInterface object.
> 
> I'll update the patch soon.

- Down to 4 seconds overhead by parsing only the extended attributes (we don't
need to parse the rest).
- interfaces with "Callback" extended attribute do not generate a property on
the global DOMWindow. Therefore, they no longer require having NoInterfaceObject
extended attribute.

[abarth-chromium, 2013-04-23 16:56:38]

I don't think this CL takes a good approach to this problem.

We also shouldn't mix subtle functional changes with massive file edits like
this CL.

This CL is going to require a significant re-think before it'll be ready to
land.

https://codereview.chromium.org/14179013/diff/25001/Source/bindings/derived_s...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14179013/diff/25001/Source/bindings/derived_s...
Source/bindings/derived_sources.gyp:283: '--include',
'<(SHARED_INTERMEDIATE_DIR)/webkit',
This is really super ugly.  You've just copy-and-pasted a bunch of build system
goop.

[do-not-use, 2013-04-25 08:47:41]

On 2013/04/23 16:56:38, abarth wrote:
> I don't think this CL takes a good approach to this problem.
> 
> We also shouldn't mix subtle functional changes with massive file edits like
> this CL.
> 
> This CL is going to require a significant re-think before it'll be ready to
> land.
> 
>
https://codereview.chromium.org/14179013/diff/25001/Source/bindings/derived_s...
> File Source/bindings/derived_sources.gyp (right):
> 
>
https://codereview.chromium.org/14179013/diff/25001/Source/bindings/derived_s...
> Source/bindings/derived_sources.gyp:283: '--include',
> '<(SHARED_INTERMEDIATE_DIR)/webkit',
> This is really super ugly.  You've just copy-and-pasted a bunch of build
system
> goop.

Adam, just to be clear, you're not opposed to adding support for the
NoInterfaceObject extended attribute or automatically generating the Constructor
attributes on the DOMWindow, right?

You're merely not satisfied with the need for parsing all other IDL files when
processing DOMWindow.idl? I'm trying to clarify as I'm not quite sure how to
proceed now.

I can certainly work on improving the gyp part although I'm struggling a bit
with it to be honest. I can also get rid of the subtle behavior changes. I would
just like to understand the "significant re-think" part.

[abarth-chromium, 2013-04-25 20:10:31]

On 2013/04/25 08:47:41, cdumez wrote:
> Adam, just to be clear, you're not opposed to adding support for the
> NoInterfaceObject extended attribute or automatically generating the
Constructor
> attributes on the DOMWindow, right?

Correct.  I think that would be good to support.

> You're merely not satisfied with the need for parsing all other IDL files when
> processing DOMWindow.idl? I'm trying to clarify as I'm not quite sure how to
> proceed now.

Yeah, I'm just not excited about how this integrated into the build system.

> I can certainly work on improving the gyp part although I'm struggling a bit
> with it to be honest. I can also get rid of the subtle behavior changes. I
would
> just like to understand the "significant re-think" part.

As an example, perhaps DOMWindow.idl should be autogenerated rather than trying
to add the properties after parsing.  (GeneratedSettings.idl is an example of
that.)  We still end up with the problem that touching any IDL file will cause a
rebuild of V8DOMWindow, which is a widely included file, but that's less
worrying.

The main thing I didn't like about this CL was all the build system goop.  We
should be able to do it with less goop.  :)

[do-not-use, 2013-04-25 20:12:34]

On 2013/04/25 20:10:31, abarth wrote:
> On 2013/04/25 08:47:41, cdumez wrote:
> > Adam, just to be clear, you're not opposed to adding support for the
> > NoInterfaceObject extended attribute or automatically generating the
> Constructor
> > attributes on the DOMWindow, right?
> 
> Correct.  I think that would be good to support.
> 
> > You're merely not satisfied with the need for parsing all other IDL files
when
> > processing DOMWindow.idl? I'm trying to clarify as I'm not quite sure how to
> > proceed now.
> 
> Yeah, I'm just not excited about how this integrated into the build system.
> 
> > I can certainly work on improving the gyp part although I'm struggling a bit
> > with it to be honest. I can also get rid of the subtle behavior changes. I
> would
> > just like to understand the "significant re-think" part.
> 
> As an example, perhaps DOMWindow.idl should be autogenerated rather than
trying
> to add the properties after parsing.  (GeneratedSettings.idl is an example of
> that.)  We still end up with the problem that touching any IDL file will cause
a
> rebuild of V8DOMWindow, which is a widely included file, but that's less
> worrying.
> 
> The main thing I didn't like about this CL was all the build system goop.  We
> should be able to do it with less goop.  :)

Ok, thanks for clarifying. I'll rework the patch soon.

[do-not-use, 2013-05-04 00:38:36]

I took Adam's feedback into consideration. There is no longer ANY behavior
change on JS side. Also, a partial interface for DOMWindow is now generated with
all the Constructor attributes in it. This simplifies the gyp part as well.

[abarth-chromium, 2013-05-06 18:06:47]

This approach looks much better.  Haraken should do the official review.  One
question below.

https://codereview.chromium.org/14179013/diff/32001/Source/bindings/scripts/p...
File Source/bindings/scripts/preprocess-idls.pl (right):

https://codereview.chromium.org/14179013/diff/32001/Source/bindings/scripts/p...
Source/bindings/scripts/preprocess-idls.pl:60: my $document =
$parser->Parse($fullPath, $defines, $preprocessor);
How does this effect compile time?  Previously, we just grepped the IDL file in
getPartialInterfaceNameFromIDLFile.  Now we're doing a full parse...

[do-not-use, 2013-05-06 18:34:38]

On 2013/05/06 18:06:47, abarth wrote:
> This approach looks much better.  Haraken should do the official review.  One
> question below.
> 
>
https://codereview.chromium.org/14179013/diff/32001/Source/bindings/scripts/p...
> File Source/bindings/scripts/preprocess-idls.pl (right):
> 
>
https://codereview.chromium.org/14179013/diff/32001/Source/bindings/scripts/p...
> Source/bindings/scripts/preprocess-idls.pl:60: my $document =
> $parser->Parse($fullPath, $defines, $preprocessor);
> How does this effect compile time?  Previously, we just grepped the IDL file
in
> getPartialInterfaceNameFromIDLFile.  Now we're doing a full parse...

Yes, IDL preprocessing is now 7 seconds slower due to the full parse.
The reason I am doing a full parse now is because we do a bit more than simply
checking if the NoInterfaceObject extended attribute is present. We also need to
parse other extended attributes (e.g. ConstructorPrefix, NamedConstructor,
Conditional, EnabledAtRuntime, EnabledPerContext).

[abarth-chromium, 2013-05-06 19:49:43]

I'm not sure this feature is worth a 7 second regression in build speed.  Is
there a way to avoid the build-time regression?

[do-not-use, 2013-05-06 20:40:06]

On 2013/05/06 19:49:43, abarth wrote:
> I'm not sure this feature is worth a 7 second regression in build speed.  Is
> there a way to avoid the build-time regression?

Ok, there is no visible build time impact if I use regular expressions. I will
reupload a new patch.

[do-not-use, 2013-05-06 20:45:59]

On 2013/05/06 20:40:06, Christophe Dumez wrote:
> On 2013/05/06 19:49:43, abarth wrote:
> > I'm not sure this feature is worth a 7 second regression in build speed.  Is
> > there a way to avoid the build-time regression?
> 
> Ok, there is no visible build time impact if I use regular expressions. I will
> reupload a new patch.

Patch Set 10 uses regular expressions only. There is no longer any visible build
time impact.

I also rebased the patch against master.

[abarth-chromium, 2013-05-06 21:40:54]

Thanks.  LGTM, but we should have haraken take a look as well.

[haraken, 2013-05-07 01:12:06]

In my understanding, this CL doesn't at all change behavior (i.e. doesn't change
a list of exposed constructors), right?

https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLAttributes.txt (right):

https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLAttributes.txt:31: ConstructorPrefix=*

Can we use [ImplementedAs] instead of introducing a new IDL attribute?

https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/p...
File Source/bindings/scripts/preprocess-idls.pl (right):

https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/p...
Source/bindings/scripts/preprocess-idls.pl:82:
$supplementalDependencies{$windowConstructorsSupplementalFile} = "DOMWindow";

Clever!

[do-not-use, 2013-05-07 04:18:15]

On 2013/05/07 01:12:06, haraken wrote:
> In my understanding, this CL doesn't at all change behavior (i.e. doesn't
change
> a list of exposed constructors), right?

That is correct. However, I think it would be a good idea to land (beforehand) a
test case that documents all the DOMWindow constructors to validate this. If
this is OK with you, I can upload it in a separate CL.

>
https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/I...
> File Source/bindings/scripts/IDLAttributes.txt (right):
> 
>
https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/I...
> Source/bindings/scripts/IDLAttributes.txt:31: ConstructorPrefix=*
> 
> Can we use [ImplementedAs] instead of introducing a new IDL attribute?

I will check.

[do-not-use, 2013-05-07 04:32:27]

On 2013/05/07 04:18:15, Christophe Dumez wrote:
> On 2013/05/07 01:12:06, haraken wrote:
> > In my understanding, this CL doesn't at all change behavior (i.e. doesn't
> change
> > a list of exposed constructors), right?
> 
> That is correct. However, I think it would be a good idea to land (beforehand)
a
> test case that documents all the DOMWindow constructors to validate this. If
> this is OK with you, I can upload it in a separate CL.

I uploaded such a test case in:
https://codereview.chromium.org/14705010/

I make sure the list of exposed constructors does not change locally but I think
it would be a good idea to have such layout test before landing this
NoInterfaceObject change.

[do-not-use, 2013-05-07 04:45:25]

On 2013/05/07 01:12:06, haraken wrote:
>
https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/I...
> File Source/bindings/scripts/IDLAttributes.txt (right):
> 
>
https://codereview.chromium.org/14179013/diff/41001/Source/bindings/scripts/I...
> Source/bindings/scripts/IDLAttributes.txt:31: ConstructorPrefix=*
> 
> Can we use [ImplementedAs] instead of introducing a new IDL attribute?

[ImplementedAs] is not currently used for interfaces. Also, the meaning is a bit
different in my opinion. However, I'm fine reusing this existing IDL extended
attribute if you believe this is a good idea.

My previous patches were using [InterfaceName] which but this caused some
behavior change as it would not only change the constructor name.

Alternatively, since this affects only 5 interfaces. Maybe we could simply mark
those as [NoInterfaceObject] and manually add a DOMWindow attribute for those? I
think this is the simplest for now.

[haraken, 2013-05-07 04:51:25]

> Alternatively, since this affects only 5 interfaces. Maybe we could simply
mark
> those as [NoInterfaceObject] and manually add a DOMWindow attribute for those?
I
> think this is the simplest for now.

Sounds like a good idea for this patch. Please add a FIXME about it.

Ideally, I think that the goal would be:

- For all interfaces, a constructor name is equal to an interface name

- [InterfaceName] is replaced with [ImplementedAs]. (Note: this is not a
mechanical replacement. Whereas we currently write [InterfaceName=Window] in
DOMWindow.idl, in the future we want to write [ImplementedAs=DOMWindow] in
Window.idl.)

[do-not-use, 2013-05-07 05:08:24]

On 2013/05/07 04:51:25, haraken wrote:
> > Alternatively, since this affects only 5 interfaces. Maybe we could simply
> mark
> > those as [NoInterfaceObject] and manually add a DOMWindow attribute for
those?
> I
> > think this is the simplest for now.
> 
> Sounds like a good idea for this patch. Please add a FIXME about it.
> 
> Ideally, I think that the goal would be:
> 
> - For all interfaces, a constructor name is equal to an interface name
> 
> - [InterfaceName] is replaced with [ImplementedAs]. (Note: this is not a
> mechanical replacement. Whereas we currently write [InterfaceName=Window] in
> DOMWindow.idl, in the future we want to write [ImplementedAs=DOMWindow] in
> Window.idl.)

This makes sense, thanks for explaining.
I have uploaded Patch Set 12 to remove [ConstructorPrefix].

[haraken, 2013-05-07 05:10:10]

LGTM. Great patch!

[haraken, 2013-05-07 05:33:54]

https://codereview.chromium.org/14179013/diff/55002/Source/bindings/scripts/p...
File Source/bindings/scripts/preprocess-idls.pl (right):

https://codereview.chromium.org/14179013/diff/55002/Source/bindings/scripts/p...
Source/bindings/scripts/preprocess-idls.pl:31: my
$windowConstructorsSupplementalFile;
Nit: Shall we rename $windowConstructorsSupplementalFile to
$windowConstructorsFile ? It's unrelated to Supplemental.

[do-not-use, 2013-05-07 05:40:04]

On 2013/05/07 05:33:54, haraken wrote:
>
https://codereview.chromium.org/14179013/diff/55002/Source/bindings/scripts/p...
> File Source/bindings/scripts/preprocess-idls.pl (right):
> 
>
https://codereview.chromium.org/14179013/diff/55002/Source/bindings/scripts/p...
> Source/bindings/scripts/preprocess-idls.pl:31: my
> $windowConstructorsSupplementalFile;
> Nit: Shall we rename $windowConstructorsSupplementalFile to
> $windowConstructorsFile ? It's unrelated to Supplemental.

Sure. Done in Patch Set 13.

[commit-bot: I haz the power, 2013-05-07 06:20:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14179013/62001

[commit-bot: I haz the power, 2013-05-07 06:33:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-07 06:43:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14179013/58004

[commit-bot: I haz the power, 2013-05-07 06:56:26]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-07 07:03:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14179013/73002

[commit-bot: I haz the power, 2013-05-07 07:42:42]

Change committed as 149858

[do-not-use, 2013-05-08 06:48:32]

I added [NoInterfaceObject] to the Blink IDL wiki:
https://sites.google.com/a/chromium.org/dev/blink/webidl?pli=1#TOC-NoInterfac...