Bug 72138 – [meta] Implement [Supplemental] IDL and modularize WebAudio and WebSocket

• Component: DOM
• Status: RESOLVED FIXED

[Kentaro Hara]

Implement [Supplemental] IDL, and move the webaudio declarations from DOMWindow.idl into a new IDL file in the WebCore/webaudio directory, which helps make webaudio a self-contained feature (aka a module).

The spec for [Supplemental] IDL: http://dev.w3.org/2006/webapi/WebIDL/#dfn-supplemental-interface

Our goal: If we prepare the following IDL file in the WebCore/webaudio/ directory,

interface [
   Conditional=WEB_AUDIO,
   Supplemental=DOMWindow
] DOMWindowWebAudio {
   attribute [JSCCustomGetter,EnabledAtRuntime] AudioContextConstructor webkitAudioContext;
   attribute AudioPannerNodeConstructor webkitAudioPannerNode;
   attribute AudioProcessingEventConstructor AudioProcessingEvent;
   attribute OfflineAudioCompletionEventConstructor OfflineAudioCompletionEvent;
};

then CodeGenerator{JS,V8}.pm adds those attributes to DOMWindow.idl during code generation, which means that we do not need to touch WebCore code when we add/remove/edit webautio APIs.

[Kentaro Hara]

Currently generate-bindings.pl reads an IDL file and then generates .h and .cpp files one by one, as follows:

foreach $idl (all IDL files) {
  generate-bindings.pl reads $idl;
  generate-bindings.pl generates .h and .cpp files for $idl;
}

On the other hand, in order to implement [Supplemental] IDL, somehow generate-bindings.pl needs to read all IDL files all at once, resolve Supplemental= dependencies and then generate .h and .cpp files, like this:

generate-bindings.pl reads all IDL files;
generate-bindings.pl resolves Supplemental=* dependencies;
generate-bindings.pl generates all .h and .cpp files;

It was easy to change generate-bindings.pl so that the above code works. However, I found that it is difficult to write build scripts so that the above code works. For example, in case of WebCore.gyp, we need to explicitly describe all output files of generate-bindings.pl (i.e. all .h and .cpp files) in WebCore.gyp, but I guess that there is no natural way to describe all the output files in the gyp file.


Then, the next approach I tried is as follows:

foreach $idl (all IDL files) {
  generate-bindings.pl reads all IDL files;
  generate-bindings.pl resolves Supplemental=$idl dependencies;
  generate-bindings.pl generates .h and .cpp files for $idl;
}

This approach requires no change in build scripts. However, in fact, reading all IDL files for __each__ IDL file was too heavy. While it takes 0.1 seconds to generate .h and .cpp files for one IDL file in the current trunk implementation, this approach takes 5 seconds. This is unacceptable.

So I am trying the next approach:

resolve-supplemental.pl (a new Perl script) reads all IDL files;
resolve-supplemental.pl resolves Supplemental=* dependencies;
resolve-supplemental.pl outputs the dependency graph to supplemental-dependency.graph;
foreach $idl (all IDL files) {
  generate-bindings.pl reads supplemental-dependency.graph;
  generate-bindings.pl generates .h and .cpp files for $idl;
}

In this approach, we need to change build scripts as follows; (1) introduce resolve-supplemental.pl, (2) resolve-supplemental.pl outputs supplemental-dependency.graph, and (3) generate-bindings.pl for each IDL file depends on supplemental-dependency.graph.

Any thoughts?

[Adam Barth]

Another consideration is whether touching one IDL will cause the build system to regenerate all the cpp and h files, which could lead to a larger incremental compile.

Maybe the way to handle that situation is instead of creating a single supplemental-dependency.graph, which would be a dependency of every IDL file, maybe resolve-supplemental.pl should create a separate output file for each IDL file.  That way each IDL file would just depend on it's supplemental information and not the entire graph.

[Kentaro Hara]

(In reply to comment #2)
> Another consideration is whether touching one IDL will cause the build system to regenerate all the cpp and h files, which could lead to a larger incremental compile.
> 
> Maybe the way to handle that situation is instead of creating a single supplemental-dependency.graph, which would be a dependency of every IDL file, maybe resolve-supplemental.pl should create a separate output file for each IDL file.  That way each IDL file would just depend on it's supplemental information and not the entire graph.

Thank you, Adam!

Sorry for the confusion, but I noticed that, in the first place, we cannot make resolve-supplemental.pl depend on all IDLs because we should avoid listing all IDLs as gyp inputs for resolve-supplemental.pl.

Consequently, we need to _always_ (i.e. no matter if there is an update for any IDLs or not) run resolve-supplemental.pl and generate supplemental-dependency.graph. However, as you mentioned, this will cause the build system to regenerate all .h and .cpp files.

So my suggestion is as follows:

- resolve-supplemental.pl depends on nothing.
- resolve-supplemental.pl always runs.
- resolve-supplemental.pl reads all IDLs.
- resolve-supplemental.pl generates the dependency graph, and if and only if the graph is different from the current supplemental-dependency.graph, resolve-supplemental.pl updates supplemental-dependency.graph.
- generate-bindings.pl for each IDL depends on supplemental-dependency.graph.
- foreach $idl (all IDL files) {
      generate-bindings.pl reads supplemental-dependency.graph;
      generate-bindings.pl generates .h and .cpp files for $idl;
  }

In this case, I think that supplemental-dependency.graph can be a single file for simplicity. Any comments?

[Adam Barth]

> In this case, I think that supplemental-dependency.graph can be a single file for simplicity. Any comments?

I see.  You're saying that the dependency graph itself won't change very often so it's ok to rebuild all the IDLs when it changes.  I think that's fair.

[Dominic Cooney]

(In reply to comment #4)
> > In this case, I think that supplemental-dependency.graph can be a single file for simplicity. Any comments?
> 
> I see.  You're saying that the dependency graph itself won't change very often so it's ok to rebuild all the IDLs when it changes.  I think that's fair.

We could create one dependency file per IDL later if excessive rebuilding proves to be a problem.

[Kentaro Hara]

Created attachment 116555 [details]
WIP patch

[Kentaro Hara]

> Sorry for the confusion, but I noticed that, in the first place, 
> we cannot make resolve-supplemental.pl depend on all IDLs 
> because we should avoid listing all IDLs as gyp inputs for resolve-supplemental.pl.

Sorry for the confusion again:-) All IDL filies are already listed in gyp. In conclusion, the WIP patch implemented the following build flow:

resolve-supplemental.pl depends on all IDL files;
resolve-supplemental.pl reads all IDL files;
resolve-supplemental.pl resolves the dependency of [Supplemental=XXXX];
resolve-supplemental.pl outputs supplemental_dependency.tmp;
foreach $idl (all IDL files) {
    generate-bindings.pl depends on $idl and supplemental_dependency.tmp;
    generate-bindings.pl reads $idl;
    generate-bindings.pl reads supplemental_dependency.tmp;
    generate-bindings.pl generates .h and .cpp files for $idl, including all attributes in IDL files which are supplementing $idl;
}

[Kentaro Hara]

I uploaded a looong WIP patch. I am planning to commit it in the following sub-patches. I would be happy if you just take a rough look at ChangeLog of the WIP patch and detailed reviews on the upcoming sub-patches:

[1] Bug-fix: In run-bindings-tests, replace subprocess.call() with subprocess.Popen().
[2] Bug-fix: Change the current working directory of run-bindings-tests from WebCore/ to WebCore/Source/.
[3] Bug-fix: Let run-bindings-tests check the results for a newly added test IDL file.
[4] Refactoring: Make output messages of run-bindings-tests more readable.
[5] Refactoring: Refactor CodeGenerator*.pm to suppress style-check error messages for newly added test IDL files.
[6] In IDLParser.pm, move global variables' initialization into InitializeGlobalData().
[7] Highlight: Make a change on resolve-supplemental.pl, generate-bindings.pl and run-bindings-tests to support the [Supplemental] IDL. Add TestSupplemental.idl.
[8] Remove rule_binding.py.
[9] Highlight: Add the [Supplemented] IDL to webaudio attributes in DOMWindow.idl. Add the [Supplemental=DOMWindow] to DOMWindowWebAudio.idl. Make a change on WebCore.gyp and action_derivedsourcesallinone.py.
[10-15?] Not yet investigated how to do it: Make a change on other build scripts. i.e. DerivedSources.make, DerivedSources.pri, GNUmakefile.am, PlatformBlackBerry.cmake, UseJSC.cmake, UseV8.cmake, WebCore.vcproj/MigrateScripts, WebCore.vcproj/WebCore.vcproj, bindings/gobject/GNUmakefile.am.
[16] Remove the [Supplemented] IDL completely.

Comments are appreciated!

[WebKit Review Bot]

Attachment 116555 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/V8/V8TestInterface.h:50:  The parameter name "info" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:127:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterface.cpp:31:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Collabora GTK+ EWS bot]

Comment on attachment 116555 [details]
WIP patch

Attachment 116555 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10642307

[Adam Barth]

Thanks for the update.  I'll take a look in detail tomorrow afternoon.  In the meantime, please feel free to post your patches.  Your plan sounds reasonable on first glance.

[Kentaro Hara]

(In reply to comment #8)
> I uploaded a looong WIP patch. I am planning to commit it in the following sub-patches. I would be happy if you just take a rough look at ChangeLog of the WIP patch and detailed reviews on the upcoming sub-patches:
> 
> [1] Bug-fix: In run-bindings-tests, replace subprocess.call() with subprocess.Popen().
> [2] Bug-fix: Change the current working directory of run-bindings-tests from WebCore/ to WebCore/Source/.
> [3] Bug-fix: Let run-bindings-tests check the results for a newly added test IDL file.
> [4] Refactoring: Make output messages of run-bindings-tests more readable.
> [5] Refactoring: Refactor CodeGenerator*.pm to suppress style-check error messages for newly added test IDL files.
> [6] In IDLParser.pm, move global variables' initialization into InitializeGlobalData().
> [7] Highlight: Make a change on resolve-supplemental.pl, generate-bindings.pl and run-bindings-tests to support the [Supplemental] IDL. Add TestSupplemental.idl.
> [8] Remove rule_binding.py.
> [9] Highlight: Add the [Supplemented] IDL to webaudio attributes in DOMWindow.idl. Add the [Supplemental=DOMWindow] to DOMWindowWebAudio.idl. Make a change on WebCore.gyp and action_derivedsourcesallinone.py.
> [10-15?] Not yet investigated how to do it: Make a change on other build scripts. i.e. DerivedSources.make, DerivedSources.pri, GNUmakefile.am, PlatformBlackBerry.cmake, UseJSC.cmake, UseV8.cmake, WebCore.vcproj/MigrateScripts, WebCore.vcproj/WebCore.vcproj, bindings/gobject/GNUmakefile.am.
> [16] Remove the [Supplemented] IDL completely.
> 
> Comments are appreciated!

Thank you for the quick reviews, Adam.

Current status:
[1][2][4][6][8]: Fixed.
[5]: review? bug 73115
[3]: Please ignore this. I was wrong.

After [5] is landed, I will post [7], and then [9], and then [10-].

[Kentaro Hara]

(In reply to comment #12)
> (In reply to comment #8)
> > [10-15?] Not yet investigated how to do it: Make a change on other build scripts. i.e. DerivedSources.make, DerivedSources.pri, GNUmakefile.am, PlatformBlackBerry.cmake, UseJSC.cmake, UseV8.cmake, WebCore.vcproj/MigrateScripts, WebCore.vcproj/WebCore.vcproj, bindings/gobject/GNUmakefile.am.
> > [16] Remove the [Supplemented] IDL completely.

Adam: I am not familiar with build scripts but my plan is as follows:

[10] Make a change on DerivedSources.make. (for Mac)
[11] Make a change on GNUmakefile.am and bindings/gobject/GNUmakefile.am. (for GTK)
[12] Make a change on DerivedSources.pri. (for Qt)
[13] Make a change on WebCore.vcproj/MigrateScripts and WebCore.vcproj/WebCore.vcproj. (for Win)
[14] Make a change on UseJSC.cmake, UseV8.cmake and PlatformBlackBerry.cmake. (for Efl, WinCE and BlackBerry)

However, before [10], if possible, I would like to add the [Supplemental] IDL to something (other than webaudio) which is being enabled by all platforms. This is because webaudio is disabled in platforms but Chromium, and thus we cannot confirm that the change to the build scripts works well or not. Any idea?

[Adam Barth]

> However, before [10], if possible, I would like to add the [Supplemental] IDL to something (other than webaudio) which is being enabled by all platforms. This is because webaudio is disabled in platforms but Chromium, and thus we cannot confirm that the change to the build scripts works well or not. Any idea?

WebAudio is enabled on Mac, but I understand your point.  Here are some good candidates that should be widely enabled:

On DOMWindow:

attribute [Conditional=SQL_DATABASE] SQLExceptionConstructor SQLException;
[EnabledAtRuntime] Database openDatabase(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize, in [Callback, Optional] DatabaseCallback creationCallback)
    raises(DOMException);

On Navigator:

readonly attribute [EnabledAtRuntime] Geolocation geolocation;

[Kentaro Hara]

(In reply to comment #14)
> > However, before [10], if possible, I would like to add the [Supplemental] IDL to something (other than webaudio) which is being enabled by all platforms. This is because webaudio is disabled in platforms but Chromium, and thus we cannot confirm that the change to the build scripts works well or not. Any idea?
> 
> WebAudio is enabled on Mac, but I understand your point.  Here are some good candidates that should be widely enabled:
> 
> On DOMWindow:
> 
> attribute [Conditional=SQL_DATABASE] SQLExceptionConstructor SQLException;
> [EnabledAtRuntime] Database openDatabase(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize, in [Callback, Optional] DatabaseCallback creationCallback)
>     raises(DOMException);
> 
> On Navigator:
> 
> readonly attribute [EnabledAtRuntime] Geolocation geolocation;

Adam: Thanks. I found that

- ENABLE_GEOLOCATION is enabled on Chromium, AppleWebKit, Gtk and BlackBerry only.
- ENABLE_SQL_DATABASE is enabled on all platforms, but it requires [Supplemental] IDL on methods (i.e. openDatabase()), which I've not yet implemented.

Then, how about adding [Supplemental] IDL on window.WebSocket?

    attribute [JSCCustomGetter,EnabledAtRuntime] WebSocketConstructor WebSocket

ENABLE_WEB_SOCKETS is enabled on all platforms and it just requires [Supplemental] IDL on attributes, which I've already implemented.

[Adam Barth]

Sure.

[Kentaro Hara]

Created attachment 118525 [details]
Patch

[Kentaro Hara]

Adam: The current progress is as follows.

Done:
WebCore.gyp/WebCore.gyp (for Chromium)
DerivedSources.make (for Mac)
GNUmakefile.am (for GTK)
DerivedSources.pri (for Qt)
UseJSC.cmake and UseV8.cmake (for Efl and WinCE)

Not yet:
WebCore.vcproj/MigrateScripts (for Win)
WebCore.vcproj/WebCore.vcproj (for Win)
bindings/gobject/GNUmakefile.am (for GTK)
PlatformBlackBerry.cmake (for BlackBerry)

My concern is that I am not sure if I can make similar changes on the above four build scripts, since neither do they have build bots nor I have local build environments for them. It appears that bindings/gobject/GNUmakefile.am is not used for the gtk-ews build bot. The win-ews build bot appears to be using (not WebCore.vcproj/MigrateScripts and WebCore.vcproj/WebCore.vcproj but) DerivedSources.make.

Any idea? (May I ask for help in webkit-dev?)

Note: In order to remove the temporary [Supplemented] IDL from DOMWindow.idl, we need to enable the [Supplemental] IDL on *all* build systems which use generate-bindings.pl.

[Adam Barth]

> Any idea? (May I ask for help in webkit-dev?)

Asking for help on webkit-dev is probably a good idea.  We might want to include a bit of an explanation about [Supplemental] to give folks some more context.

I have an EC2 instance that I can boot up that can build the AppleWin port (or at least used to be able to).  It's a bit slow, but it might be helpful.  For AppleWin, specifically, we should talk with aroben.  He might be able to make this change fairly easily.  Emailing webkit-dev is likely to lead to useful contacts for the remaining build systems as well.

Generally, if ports don't have buildbots, it's hard for the project to keep them building.  We should certainly give folks some notice so they know that this change is going to happen, but at some point we need to move forward and make this a requirement.

Thanks for all your hard work so far.  We seem to be quite close.

[Kentaro Hara]

Adam: Let me update the current progress.

> WebCore.vcproj/MigrateScripts (for AppleWin)
> WebCore.vcproj/WebCore.vcproj (for AppleWin)

I first thought that we have not yet enabled the [Supplemental] IDL in AppleWin, but actually we have done it at the bug 74900. I uploaded a follow-up patch (bug 75412).

> DerivedSources.make (for Mac)

I uploaded a follow-up patch (bug 75426).

> bindings/gobject/GNUmakefile.am (for GTK)

I have learned that GTK/GObject is being built on the GTK build bot, i.e. we can confirm that my change is correct by watching the GTK build bot result. I uploaded a patch for enabling the [Supplemental] IDL on GTK/GObject (bug 75411).

> PlatformBlackBerry.cmake (for BlackBerry)

I uploaded a patch for enabling the [Supplemental] IDL on BlackBerry (bug 75413). Since I have not yet confirmed that the BlackBerry build passes, we need to ask someone if the patch is correct or not.


After committing all these patches, I think

[1] we can remove all temporary code (e.g. [Supplemented] IDL) that I inserted to implement the [Supplemental] IDL incrementally.
[2] we can support the [Supplemental] IDL for methods.

Thanks for reviewing!

[Adam Barth]

> After committing all these patches, I think
> 
> [1] we can remove all temporary code (e.g. [Supplemented] IDL) that I inserted to implement the [Supplemental] IDL incrementally.
> [2] we can support the [Supplemental] IDL for methods.

Fantastic!  Thanks for all your hard work.

[Kentaro Hara]

Created attachment 121801 [details]
WIP patch to see if build passes

[Kentaro Hara]

abarth: I uploaded a patch that implements the [Supplemental] IDL on methods and uses it in SQLDatabase. (ENABLE_SQL_DATABASE is enabled on all build systems.) After confirming that it works fine in all build systems, I'll commit the patch in the following steps:

[1] Make a change on generate-bindings.pl and TestSupplemental.idl
[2] Use the [Supplemental] IDL in SQLDatabase

[Kentaro Hara]

Created attachment 121802 [details]
WIP patch to see if build passes

[WebKit Review Bot]

Attachment 121802 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:80:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:85:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

(In reply to comment #23)
> abarth: I uploaded a patch that implements the [Supplemental] IDL on methods and uses it in SQLDatabase. (ENABLE_SQL_DATABASE is enabled on all build systems.) After confirming that it works fine in all build systems, I'll commit the patch in the following steps:
> 
> [1] Make a change on generate-bindings.pl and TestSupplemental.idl
> [2] Use the [Supplemental] IDL in SQLDatabase

abarth: The patch for [1]: bug 75944

[Adam Barth]

I think we can retire this meta bug.