Bug 73394 – Enable the [Supplemental] IDL in Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kentaro Hara]

Using the [Supplemental] IDL, we can move the attribute declarations of webaudio from DOMWindow.idl to webaudio/DOMWindowWebAudio.idl, which helps make webaudio a self-contained feature (a.k.a. module).

In order to avoid regressions, for the time being, we modify only the Chromium build script to use the [Supplemental] IDL. The patches for other build scripts will be followed.

[Kentaro Hara]

Created attachment 117118 [details]
Patch

[Adam Barth]

Comment on attachment 117118 [details]
Patch

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

This looks great, but I'm not sure it will compile.

> Source/WebCore/WebCore.gyp/WebCore.gyp:456
> +            # exceed OS limites.

limites => limits

> Source/WebCore/WebCore.gyp/WebCore.gyp:466
> +            'supplemental_dependency.tmp',

Do we need to specify a directory for this file, such as the shared intermediate directory?

> Source/WebCore/webaudio/DOMWindowWebAudio.idl:25
> +    ] DOMWindowWebAudio {

We don't need to add this file to bindings_idl_files ?

> Source/WebCore/webaudio/DOMWindowWebAudio.idl:29
> +        attribute [JSCCustomGetter, EnabledAtRuntime] AudioContextConstructor webkitAudioContext;
> +        attribute AudioPannerNodeConstructor webkitAudioPannerNode;
> +        attribute AudioProcessingEventConstructor AudioProcessingEvent;
> +        attribute OfflineAudioCompletionEventConstructor OfflineAudioCompletionEvent;

Do we need new C++ implementations of these functions as statics on DOMWindowWebAudio ?

[Kentaro Hara]

Comment on attachment 117118 [details]
Patch

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

>> Source/WebCore/WebCore.gyp/WebCore.gyp:456
>> +            # exceed OS limites.
> 
> limites => limits

Oops, I'll fix it. (I copied it from another place though:-)

>> Source/WebCore/WebCore.gyp/WebCore.gyp:466
>> +            'supplemental_dependency.tmp',
> 
> Do we need to specify a directory for this file, such as the shared intermediate directory?

Maybe. If we do not specify the directory, this file is generated under WebCore/WebCore.gyp/. I am not sure if this is good, but I did so because idl_files_list.tmp is being generated under WebCore/WebCore.gyp/ too. Should we change this to another temporary directory?

>> Source/WebCore/webaudio/DOMWindowWebAudio.idl:25
>> +    ] DOMWindowWebAudio {
> 
> We don't need to add this file to bindings_idl_files ?

I think we need. DOMWindowWebAudio.idl should be written somewhere in WebCore.gypi, and I think that bindings_idl_files is a natural place for it. In our current implementation,

- The IDL files written in bindings_idl_files become an input for resolve-supplemental.pl.
- resolve-supplemental.pl reads all IDL files including DOMWindowWebAudio.idl and generates supplemental-dependency.tmp.
- generate-bindings.pl skips generating .h and .cpp files for DOMWindowWebAudio.idl, based on supplemental-dependency.tmp.

>> Source/WebCore/webaudio/DOMWindowWebAudio.idl:29
>> +        attribute OfflineAudioCompletionEventConstructor OfflineAudioCompletionEvent;
> 
> Do we need new C++ implementations of these functions as statics on DOMWindowWebAudio ?

I think no, in case of DOMWindowWebAudio (All tests in webaudio/ are passing with my patch). The type of these attributes is XXXXConstructor. In case of XXXXConstructor, the generated code directly calls back to V8XXXX.cpp without passing through DOMWindowWebAudio.cpp. Specifically, the generated code for webkitAudioPannerNode is as follows:

#if ENABLE(WEB_AUDIO)
    {"webkitAudioPannerNode", DOMWindowInternal::DOMWindowConstructorGetter, 0, &V8AudioPannerNode::info, static_cast<v8\
::AccessControl>(v8::DEFAULT), static_cast<v8::PropertyAttribute>(v8::ReadOnly), 0 /* on instance */},
#endif // ENABLE(WEB_AUDIO)

static v8::Handle<v8::Value> DOMWindowConstructorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    INC_STATS("DOM.DOMWindow.constructors._get");
    v8::Handle<v8::Value> data = info.Data();
    ASSERT(data->IsExternal() || data->IsNumber());
    WrapperTypeInfo* type = WrapperTypeInfo::unwrap(data);
    return V8DOMWrapper::getConstructor(type, V8DOMWindow::toNative(info.Holder()));                                     
}

[Adam Barth]

> >> Source/WebCore/WebCore.gyp/WebCore.gyp:466
> >> +            'supplemental_dependency.tmp',
> > 
> > Do we need to specify a directory for this file, such as the shared intermediate directory?
> 
> Maybe. If we do not specify the directory, this file is generated under WebCore/WebCore.gyp/. I am not sure if this is good, but I did so because idl_files_list.tmp is being generated under WebCore/WebCore.gyp/ too. Should we change this to another temporary directory?

The shared intermediate directory is probably the right place for it since it is an intermediate build product.  idl_files_list.tmp is slightly different because it is created during the GYP generation phase rather than the compile phase.

[Kentaro Hara]

Created attachment 117144 [details]
rebased patch for commit

[Kentaro Hara]

(In reply to comment #4)
> > >> Source/WebCore/WebCore.gyp/WebCore.gyp:466
> > >> +            'supplemental_dependency.tmp',
> > > 
> > > Do we need to specify a directory for this file, such as the shared intermediate directory?
> > 
> > Maybe. If we do not specify the directory, this file is generated under WebCore/WebCore.gyp/. I am not sure if this is good, but I did so because idl_files_list.tmp is being generated under WebCore/WebCore.gyp/ too. Should we change this to another temporary directory?
> 
> The shared intermediate directory is probably the right place for it since it is an intermediate build product.  idl_files_list.tmp is slightly different because it is created during the GYP generation phase rather than the compile phase.

Done. Thanks.

[WebKit Review Bot]

Comment on attachment 117144 [details]
rebased patch for commit

Rejecting attachment 117144 [details] from commit-queue.

New failing tests:
webaudio/biquadfilternode-basic.html
webaudio/audiochannelmerger-stereo.html
webaudio/delaynode.html
webaudio/delaynode-scheduling.html
webaudio/audionode.html
webaudio/audiobuffersource-channels.html
fast/replaced/width100percent-textarea.html
Full output: http://queues.webkit.org/results/10716003

[Kentaro Hara]

Hmm... commit-queue is still reporting webaudio/ errors, __non-deterministically__.

http://queues.webkit.org/patch/117144

- chromium-ews always passes all webaudio/ tests. 
- My local chromium environment always passes all webaudio/ tests.
- commit-queue fails some of webaudio/ tests non-deterministically. Some webaudio/ tests sometimes go TIMEOUT. Some webaudio/ tests sometimes go MISSING.

[WebKit Review Bot]

Comment on attachment 117144 [details]
rebased patch for commit

Clearing flags on attachment: 117144

Committed r101669: <http://trac.webkit.org/changeset/101669>

[Kentaro Hara]

Reverted r101669 for reason:

Win build and Mac build are failing

Committed r101672: <http://trac.webkit.org/changeset/101672>

[Kentaro Hara]

Created attachment 117513 [details]
patch for commit

[WebKit Review Bot]

Comment on attachment 117513 [details]
patch for commit

Clearing flags on attachment: 117513

Committed r101737: <http://trac.webkit.org/changeset/101737>

[Darin Adler]

The title of the bug in the patch says “for Chromium”. Is this Chromium-specific? What effect does this have on other platforms?

[Kentaro Hara]

(In reply to comment #13)
> The title of the bug in the patch says “for Chromium”. Is this Chromium-specific? What effect does this have on other platforms?

Yes, this patch changes only the build flow of Chromium so that it uses the [Supplemental] IDL. Other platforms are still ignoring the [Supplemental] IDL and thus are not affected by this patch (hopefully).

I am planning to make the change to other platforms in the coming patches one by one.

[Kentaro Hara]

Reverted r101737 for reason:

Chromium/Mac and Chromium/Win build are broken

Committed r101740: <http://trac.webkit.org/changeset/101740>

[Kentaro Hara]

I rolled out the patch since Chromium/Mac and Chromium/Win builds are broken. But the errors look strange.

Chromium/Mac error: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/3014/steps/compile-webkit/logs/stdio

Chromium/Win error: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/21330/steps/compile-webkit/logs/stdio

In WebCore.gyp, we described the following dependency:

- resolve-supplemental.pl depends on all IDL files.
- resolve-supplemental.pl outputs supplemental-dependency.tmp.
- action_derivedsourcesallinone.py depends on supplemental-dependency.tmp.

And this patch changed the list of all IDL files (i.e. added DOMWindowWebAudio.idl).

However, the errors are saying:

- resolve-supplemental.pl is not executed.
- action_derivedsourcesallinone.py cannot find supplemental-dependency.tmp.


What is happening?? For now, I am investigating if I can reproduce the problem in my local Win and Mac environments.

[Adam Barth]

> What is happening?? For now, I am investigating if I can reproduce the problem in my local Win and Mac environments.

I've added Tony to the CC list.  He might have some insight into what's going wrong in the build system.

[Kentaro Hara]

(In reply to comment #17)
> > What is happening?? For now, I am investigating if I can reproduce the problem in my local Win and Mac environments.
> 
> I've added Tony to the CC list.  He might have some insight into what's going wrong in the build system.

I noticed that we might need to add dependencies between target_names. Specifically,

  {
    'target_name': 'webcore_bindings_sources',
    'type': 'none',
    'hard_dependency': 1,
        'dependencies': [
            'generate_supplemental_dependency',
        ],
        ...;

to indicate that the actions of generate-bindings.py and action_derivedsourcesallinone.py (which are written inside the target 'webcore_bindings_sources') depend on the result of the action of resolve-supplemental.py (which is written in the target 'generate_supplemental_dependency').

I am trying to confirm if it works in my local Chromium/Mac environment.

[Kentaro Hara]

Created attachment 117577 [details]
patch for commit

[Kentaro Hara]

(In reply to comment #19)
> Created an attachment (id=117577) [details]
> patch for commit

I confirmed that build succeeds and webaudio/ tests pass in my Chromium/Mac environment.

[WebKit Review Bot]

Comment on attachment 117577 [details]
patch for commit

Clearing flags on attachment: 117577

Committed r101783: <http://trac.webkit.org/changeset/101783>

[Kentaro Hara]

Sadly, we rolled it out again...

While Chromium/Mac build succeeded but Chromium/Win build has been still failing: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/17132/steps/compile/logs/stdio

It says that IDL files are not found by derived_sources_all_in_one. I am investigating it.

[Kentaro Hara]

(In reply to comment #22)
> Sadly, we rolled it out again...
> 
> While Chromium/Mac build succeeded but Chromium/Win build has been still failing: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/17132/steps/compile/logs/stdio
> 
> It says that IDL files are not found by derived_sources_all_in_one. I am investigating it.

I found the reason for the Chromium/Windows build failure, but I am not sure how we should fix it. It is related to a bad relationship between Perl and Python on Windows/Cygwin environment.

[What is the failure?]
action_derivedsourcesallinone.py fails to open "/cygdrive/c/.../....idl".

[The cause of the failure]
Python cannot understand "/cygdrive/c/.../....idl"-style paths. It should instead be "C:/cygwin/.../....idl"-style paths.

[The cause of the failure (more in detail)]
- resolve-supplemental.pl calls Cwd::realpath() for all IDL files and writes the results to supplemental-dependency.tmp.
- supplemental-dependency.tmp is later used by generate-bindings.pl and action_derivedsourcesallinone.py, which try to open the IDL files described in supplemental-dependency.tmp.

- Perl executable is /usr/bin/perl and Python executable is /usr/bin/python.
- Perl's Cwd::realpath() returns "/cygdrive/c/.../....idl"-style paths, no matter if Perl is invoked from Cygwin or DOS-Prompt.
- Perl can open both "/cygdrive/c/.../....idl"-style paths and "C:/cygwin/.../....idl"-style paths, no matter if Perl is invoked from Cygwin or DOS-Prompt.
- Python can open both "/cygdrive/c/.../....idl"-style paths and "C:/cygwin/.../....idl"-style paths, if Python is invoked from Cygwin. However, Python can open "C:/cygwin/.../....idl"-style paths but cannot open "/cygdrive/c/.../....idl"-style paths, if Python is invoked from DOS-Prompt.

- It seems that in Chromium/Windows build, Perl and Python are invoked from DOS-Prompt. In this way, "/cygdrive/c/.../....idl"-style paths are written down in supplemental-dependency.tmp.
- generate-bindings.pl runs fine with supplemental-dependency.tmp, since Perl can open "/cygdrive/c/.../....idl"-style paths.
- action_derivedsourcesallinone.py cannot run with supplemental-dependency.tmp, since Python invoked from DOS-Prompt cannot open "/cygdrive/c/.../....idl"-style paths.

[Possible solutions]
[A] Perl behaves as we expected even if it is invoked from Cygwin or DOS-Prompt. Therefore, we can rewrite action_derivedsourcesallinone.py in Perl.
[B] We can make resolve-supplemental.pl output two kinds of supplemental-dependency.tmp. One is "/cygdrive/c/.../....idl"-style and the other is "C:/cygwin/.../....idl"-style (obtained by cygpath command).

I would prefer [A] rather than [B], because [A] is simpler. In [B], resolve-supplemental.pl has to behave differently depending on whether cygpath command exists or not, and two kinds of supplemental-dependency.tmp can be generated. 

WDYT?

[Adam Barth]

[A] sounds fine to me.  Tony might have some thoughts on the matter though.

[Tony Chang]

Sorry, just saw this bug.

> (In reply to comment #22)
> - Python can open both "/cygdrive/c/.../....idl"-style paths and "C:/cygwin/.../....idl"-style paths, if Python is invoked from Cygwin. However, Python can open "C:/cygwin/.../....idl"-style paths but cannot open "/cygdrive/c/.../....idl"-style paths, if Python is invoked from DOS-Prompt.

Maybe you have 2 versions of python installed on your system?  I didn't think it mattered where you invoked python from.  If this is the only problem, maybe you can just add a dependency to <(chromium_src_dir)/build/win/system.gyp:cygwin for the webcore_bindings_sources target?

[A] also sounds OK to me, but another possibility (perhaps less work than [A]) would be to call cygpath from action_derivedsourcesallinone.py if on Windows (see ToolsScripts/webkitpy/common/system/path.py) to convert from /cygpath/... to C:\...

[Kentaro Hara]

(In reply to comment #25)
> > (In reply to comment #22)
> [A] also sounds OK to me, but another possibility (perhaps less work than [A]) would be to call cygpath from action_derivedsourcesallinone.py if on Windows (see ToolsScripts/webkitpy/common/system/path.py) to convert from /cygpath/... to C:\...

Thank you very much for the information. I implemented [A] (bug 73824) since [A] sounds like a clearer approach.

> > - Python can open both "/cygdrive/c/.../....idl"-style paths and "C:/cygwin/.../....idl"-style paths, if Python is invoked from Cygwin. However, Python can open "C:/cygwin/.../....idl"-style paths but cannot open "/cygdrive/c/.../....idl"-style paths, if Python is invoked from DOS-Prompt.
> 
> Maybe you have 2 versions of python installed on your system?  I didn't think it mattered where you invoked python from.  

(Although now we don't have to dig into this issue, just FYI, ) it appears the same version of Python is being used.

On DOS-Prompt:
$ which python      ### I've put "C:\cygwin\bin" into PATH.
/usr/bin/python
$ python
>>> import os;
>>> os.path.exists('C:/cygwin/')
True
>>> os.path.exists('/cygdrive/c/')
False

On Cygwin:
$ which python
/usr/bin/python
$ python
>>> import os;
>>> os.path.exists('C:/cygwin/')
True
>>> os.path.exists('/cygdrive/c/')
True

[Tony Chang]

> If this is the only problem, maybe you can just add a dependency to <(chromium_src_dir)/build/win/system.gyp:cygwin for the webcore_bindings_sources target?

Did this not work?

I asked if you were getting the right version of python because I don't think the bots have /usr/bin/python installed (cygwin python).  They should only have ActivePython installed which doesn't understand cygwin paths.

[Kentaro Hara]

(In reply to comment #27)
> > If this is the only problem, maybe you can just add a dependency to <(chromium_src_dir)/build/win/system.gyp:cygwin for the webcore_bindings_sources target?
> 
> Did this not work?

Tony: It worked! Then, would it be better to solve the issue not by rewriting Python to Perl but by "<(chromium_src_dir)/build/win/system.gyp:cygwin"? (I am not at all hesitating to discard the rewriting patch.)

> I asked if you were getting the right version of python because I don't think the bots have /usr/bin/python installed (cygwin python).  They should only have ActivePython installed which doesn't understand cygwin paths.

Ah, I got it.

[Tony Chang]

(In reply to comment #28)
> (In reply to comment #27)
> > > If this is the only problem, maybe you can just add a dependency to <(chromium_src_dir)/build/win/system.gyp:cygwin for the webcore_bindings_sources target?
> > 
> > Did this not work?
> 
> Tony: It worked! Then, would it be better to solve the issue not by rewriting Python to Perl but by "<(chromium_src_dir)/build/win/system.gyp:cygwin"? (I am not at all hesitating to discard the rewriting patch.)

Yes, that seems like the easiest/simplest thing to do.

[Kentaro Hara]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > > If this is the only problem, maybe you can just add a dependency to <(chromium_src_dir)/build/win/system.gyp:cygwin for the webcore_bindings_sources target?
> > > 
> > > Did this not work?
> > 
> > Tony: It worked! Then, would it be better to solve the issue not by rewriting Python to Perl but by "<(chromium_src_dir)/build/win/system.gyp:cygwin"? (I am not at all hesitating to discard the rewriting patch.)
> 
> Yes, that seems like the easiest/simplest thing to do.

Tony: Sorry for the confusion. Actually, I double confirmed that It did not work. Specifically, WebCore.gyp looks like this. Am I missing something?

    {
      'target_name': 'webcore_bindings_sources',
      'type': 'none',
      'hard_dependency': 1,
      'conditions': [
        ['OS=="win"', {
          'dependencies': [
            'generate_supplemental_dependency',
            '<(chromium_src_dir)/build/win/system.gyp:cygwin',
          ]
        }],
        ['OS!="win"', {
          'dependencies': [
            'generate_supplemental_dependency',
          ]
        }],
      ],


(The reason why I thought it did work yesterday is that I was just checking if action_derivedsourcesallinone.py outputs no errors. Yesterday action_derivedsourcesallinone.py output no errors in fact, but that appeared to be because action_derivedsourcesallinone.py was not executed since I did not try a clean build.)

[Kentaro Hara]

Tony: Even if I change WebCore.gyp as follows and try clean build on my local Chromium/Windows, it still fails.

    {
      'target_name': 'webcore_bindings_sources',
      'type': 'none',
      'hard_dependency': 1,
      'dependencies': [
          'generate_supplemental_dependency',
          '<(chromium_src_dir)/build/win/system.gyp:cygwin',
      ],

action_derivedsourcesallinone.py outputs errors like this:

    WARNING: file not found: "/cygdrive/c/cygwin/home/haraken/chrome-build/src/third_party/WebKit/Source/WebCore/websockets/CloseEvent.idl"
    WARNING: file not found: "/cygdrive/c/cygwin/home/haraken/chrome-build/src/third_party/WebKit/Source/WebCore/websockets/WebSocket.idl"
    WARNING: file not found: "/cygdrive/c/cygwin/home/haraken/chrome-build/src/third_party/WebKit/Source/WebCore/workers/AbstractWorker.idl"


>>> [A] Perl behaves as we expected even if it is invoked from Cygwin or DOS-Prompt. Therefore, we can rewrite action_derivedsourcesallinone.py in Perl.
> [A] also sounds OK to me, ...

Maybe the "rewriting action_derivedsourcesallinone.py in Perl" approach would be simpler in this situation, instead of investigating the WebCore.gyp issues.

WDYT?

[Tony Chang]

(In reply to comment #31)
> Tony: Even if I change WebCore.gyp as follows and try clean build on my local Chromium/Windows, it still fails.

Yeah, I was surprised when you said it worked.  My understanding was that there's only one version of python installed on the bots and it's win32 python which doesn't know anything about cygwin style paths.

> Maybe the "rewriting action_derivedsourcesallinone.py in Perl" approach would be simpler in this situation, instead of investigating the WebCore.gyp issues.

Seems like there are simpler (i.e., less new code written) solutions like using webkitpy/common/system/path.py:cygpath in action_derivedsourcesallinone.py or using a regex before outputting the filenames in resolve-supplemental.pl.

I'm not opposed to rewriting in perl, but it seems unfortunate that the other files in Source/WebCore/WebCore.gyp/scripts are all in python and to have the code churn because we couldn't get the right path formatting.

[Kentaro Hara]

Created attachment 118332 [details]
final patch?

[Kentaro Hara]

Adam: I modified action_derivedsourcesallinone.py using webkit.common.system.path.cygpath. I confirmed that action_derivedsourcesallinone.py works well in both Chromium/Windows and Chromium/Linux. Would you please take another look at it? Sorry for taking so long time since the first r+.

[WebKit Review Bot]

Comment on attachment 118332 [details]
final patch?

Clearing flags on attachment: 118332

Committed r102321: <http://trac.webkit.org/changeset/102321>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

The committed patch was rolled out, but it appears the patch was actually innocent. However, re-committing the patch now will confuse the (broken) build situation. I will re-commit it tomorrow morning so that I can watch it.

[WebKit Review Bot]

Comment on attachment 118332 [details]
final patch?

Rejecting attachment 118332 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10781467

[Kentaro Hara]

Created attachment 118484 [details]
patch for commit

[WebKit Review Bot]

Comment on attachment 118484 [details]
patch for commit

Clearing flags on attachment: 118484

Committed r102416: <http://trac.webkit.org/changeset/102416>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Reverted r102416 for reason:

Chromium Win clobber builds are failing


Committed r102472: <http://trac.webkit.org/changeset/102472>

[Tony Chang]

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/17488/steps/compile/logs/stdio


128>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\bindings\v8\V8DOMWrapper.h(40) : fatal error C1083: Cannot open include file: 'V8Event.h': No such file or directory

Where 128 is the webcore_bindings target.

I'll try and fix on my win box and reland if possible.

[Tony Chang]

It looks like we're hitting the code at http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/generate-bindings.pl#L103 and not generating the files listed in $supplementalDependencyFile.

[Kentaro Hara]

(In reply to comment #44)
> It looks like we're hitting the code at http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/generate-bindings.pl#L103 and not generating the files listed in $supplementalDependencyFile.

Sorry for the rolling out. As I'm home and don't have a Win environment, I'll take a look at it on Monday.

[Kentaro Hara]

Created attachment 118721 [details]
fixed Chromium/Win build failure

[Kentaro Hara]

(In reply to comment #44)
> It looks like we're hitting the code at http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/generate-bindings.pl#L103 and not generating the files listed in $supplementalDependencyFile.

The reason for the failure is this if statement: http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/generate-bindings.pl#L96

96            if ($idlFile and $idlFile eq $targetIdlFile) {
97	            $idlFound = 1;

On Chromium/Win, the following can happen:

    $idlFile = /cygdrive/c/cygwin/home/haraken/chrome-build/src/third_party/WebKit/Source/WebCore/html/HTMLMeterElement.idl
    $targetIdlFile = /cygdrive/c/cygwin/home/haraken/chrome-build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/HTMLMeterElement.idl

So I fixed the if statement as follows:

96            if ($idlFile and basename($idlFile) eq basename($targetIdlFile)) {
97	            $idlFound = 1;

[WebKit Review Bot]

Comment on attachment 118721 [details]
fixed Chromium/Win build failure

Clearing flags on attachment: 118721

Committed r102556: <http://trac.webkit.org/changeset/102556>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r102556 for reason:

clobber build failure

Committed r102572: <http://trac.webkit.org/changeset/102572>

[Kentaro Hara]

Created attachment 118900 [details]
Patch

[Kentaro Hara]

(In reply to comment #50)
> Reverted r102556 for reason:
> 
> clobber build failure
> 
> Committed r102572: <http://trac.webkit.org/changeset/102572>

Adam: I rolled it out because the build fix required non-trivial change. Review again, please?

The reason for the failure is that V8Internals.h and V8Internals.cpp are not generated in the clobber build.

...In summary, I found that we need to handle three kinds of IDL files.

[a] .h and .cpp should be generated. The .cpp should be included in V8DerivedSources*.cpp. (e.g. XMLHttpRequest.idl and most IDL files)

[b] .h and .cpp should not be generated because the IDL file is marked as [Supplemental]. (e.g. DOMWindowWebAudio.idl). Obviously .cpp should not be included in V8DerivedSources*.cpp (since the .cpp does not exist).

[c] .h and .cpp should be generated but the .cpp should not be included in V8DerivedSources*.cpp. (e.g. Internals.idl)

We've just supported [a] and [b] and we need to handle [c] somehow. Then the uploaded patch added an --additionalIdlFilesList option to generate-bindings.pl, which specifies the IDL files that are not listed in supplemental-dependency.tmp but should generate .h and .cpp files. Specifically,

- The IDL files listed in supplemental-dependency.tmp are for [a].
- The IDL files listed in the --additionalIdlFilesList option are for [c].

In other words,

- action_derivedsourcesallinone.py outputs V8DerivedSources*.cpp for all the IDL files in supplemental-dependency.tmp.
- generate-bindings.pl generates .h and .cpp for all the IDL files in supplemental-dependency.tmp *and* the --additionalIdlFilesList option.

[Adam Barth]

Comment on attachment 118900 [details]
Patch

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

> Source/WebCore/WebCore.gyp/scripts/action_derivedsourcesallinone.py:52
> +sys.path.append("../../../Tools/Scripts/")
> +from webkitpy.common.system import path

We don't allow code outside of Tool/Scripts to import packages from webkitpy.  How large is this function?  Can we just copy/paste it here?

[Kentaro Hara]

(In reply to comment #53)
> (From update of attachment 118900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118900&action=review
> 
> > Source/WebCore/WebCore.gyp/scripts/action_derivedsourcesallinone.py:52
> > +sys.path.append("../../../Tools/Scripts/")
> > +from webkitpy.common.system import path
> 
> We don't allow code outside of Tool/Scripts to import packages from webkitpy.  How large is this function?  Can we just copy/paste it here?

Actually path.cygpath is large but the essential part is small. I'll copy and paste the essential part.

[Kentaro Hara]

Created attachment 118914 [details]
patch for commit

[Tony Chang]

Comment on attachment 118914 [details]
patch for commit

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

> Source/WebCore/WebCore.gyp/scripts/action_derivedsourcesallinone.py:185
> +def cygpath(fileName):
> +    cmd = ['cygpath', '-wa', fileName]
> +    process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
> +    exitCode = process.wait()

It's a lot slower to launch a new cygpath each time you need this.  It'll be worth it to have a module level global that caches the process.

[Adam Barth]

Comment on attachment 118914 [details]
patch for commit

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

Minor nits that are probably not worth worrying about.

> Source/WebCore/WebCore.gyp/scripts/action_derivedsourcesallinone.py:203
> +        if idlFileName.find("/cygdrive") == 0:
> +            idlFileName = cygpath(idlFileName)

idlFileName.startswith ?

Seems like this would cause problems on Unix systems that happen to have a /cygdrive folder.  ;)

[Kentaro Hara]

(In reply to comment #56)
> (From update of attachment 118914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118914&action=review
> 
> > Source/WebCore/WebCore.gyp/scripts/action_derivedsourcesallinone.py:185
> > +def cygpath(fileName):
> > +    cmd = ['cygpath', '-wa', fileName]
> > +    process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
> > +    exitCode = process.wait()
> 
> It's a lot slower to launch a new cygpath each time you need this.  It'll be worth it to have a module level global that caches the process.

Then we can use "cygpath -f - -wa" option. We can pass multiple paths to cygpath command from stdin. In that case we need to invoke only one cygpath command.

The patch is coming...

[Kentaro Hara]

I just modified action_derivedsourcesallinone.py since the previous patch. I confirmed that it works in Chromium/Linux and Chromium/Windows clobber build. Review?

### I was in trouble with mysterious behaviors of the Python subprocess module:-)

> > Source/WebCore/WebCore.gyp/scripts/action_derivedsourcesallinone.py:203
> > +        if idlFileName.find("/cygdrive") == 0:
> > +            idlFileName = cygpath(idlFileName)
> 
> idlFileName.startswith ?
> 
> Seems like this would cause problems on Unix systems that happen to have a /cygdrive folder.  ;)

Fixed it, but aren't v.startswith(...) and v.find(...) == 0 equivalent?

[Kentaro Hara]

Created attachment 118967 [details]
Patch

[Adam Barth]

> Fixed it, but aren't v.startswith(...) and v.find(...) == 0 equivalent?

startswith is faster.  Find is O(length of string) but startswith is constant time.

[WebKit Review Bot]

Comment on attachment 118967 [details]
Patch

Clearing flags on attachment: 118967

Committed r102663: <http://trac.webkit.org/changeset/102663>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.