Bug 73394

Summary: Enable the [Supplemental] IDL in Chromium
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: New BugsAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, dominicc, japhet, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 73652, 73824, 74072    
Bug Blocks: 72138    
Attachments:
Description Flags
Patch
none
rebased patch for commit
none
patch for commit
none
patch for commit
none
final patch?
none
patch for commit
none
fixed Chromium/Win build failure
none
Patch
none
patch for commit
none
Patch none

Description Kentaro Hara 2011-11-29 21:26:44 PST
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.
Comment 1 Kentaro Hara 2011-11-29 21:40:18 PST
Created attachment 117118 [details]
Patch
Comment 2 Adam Barth 2011-11-30 00:45:32 PST
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 ?
Comment 3 Kentaro Hara 2011-11-30 01:26:35 PST
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()));                                     
}
Comment 4 Adam Barth 2011-11-30 01:34:19 PST
> >> 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.
Comment 5 Kentaro Hara 2011-11-30 01:53:12 PST
Created attachment 117144 [details]
rebased patch for commit
Comment 6 Kentaro Hara 2011-11-30 01:56:19 PST
(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.
Comment 7 WebKit Review Bot 2011-11-30 07:39:53 PST
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
Comment 8 Kentaro Hara 2011-12-01 01:49:10 PST
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.
Comment 9 WebKit Review Bot 2011-12-01 08:07:31 PST
Comment on attachment 117144 [details]
rebased patch for commit

Clearing flags on attachment: 117144

Committed r101669: <http://trac.webkit.org/changeset/101669>
Comment 10 Kentaro Hara 2011-12-01 08:29:03 PST
Reverted r101669 for reason:

Win build and Mac build are failing

Committed r101672: <http://trac.webkit.org/changeset/101672>
Comment 11 Kentaro Hara 2011-12-01 16:20:36 PST
Created attachment 117513 [details]
patch for commit
Comment 12 WebKit Review Bot 2011-12-01 20:12:04 PST
Comment on attachment 117513 [details]
patch for commit

Clearing flags on attachment: 117513

Committed r101737: <http://trac.webkit.org/changeset/101737>
Comment 13 Darin Adler 2011-12-01 20:14:00 PST
The title of the bug in the patch says “for Chromium”. Is this Chromium-specific? What effect does this have on other platforms?
Comment 14 Kentaro Hara 2011-12-01 20:35:06 PST
(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.
Comment 15 Kentaro Hara 2011-12-01 20:51:25 PST
Reverted r101737 for reason:

Chromium/Mac and Chromium/Win build are broken

Committed r101740: <http://trac.webkit.org/changeset/101740>
Comment 16 Kentaro Hara 2011-12-01 21:06:13 PST
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.
Comment 17 Adam Barth 2011-12-01 23:09:51 PST
> 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.
Comment 18 Kentaro Hara 2011-12-01 23:17:59 PST
(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.
Comment 19 Kentaro Hara 2011-12-01 23:30:45 PST
Created attachment 117577 [details]
patch for commit
Comment 20 Kentaro Hara 2011-12-01 23:55:16 PST
(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.
Comment 21 WebKit Review Bot 2011-12-02 04:14:12 PST
Comment on attachment 117577 [details]
patch for commit

Clearing flags on attachment: 117577

Committed r101783: <http://trac.webkit.org/changeset/101783>
Comment 22 Kentaro Hara 2011-12-02 06:14:58 PST
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.
Comment 23 Kentaro Hara 2011-12-04 21:57:51 PST
(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?
Comment 24 Adam Barth 2011-12-04 22:20:06 PST
[A] sounds fine to me.  Tony might have some thoughts on the matter though.
Comment 25 Tony Chang 2011-12-05 10:03:14 PST
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:\...
Comment 26 Kentaro Hara 2011-12-05 15:48:21 PST
(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
Comment 27 Tony Chang 2011-12-05 15:55:59 PST
> 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.
Comment 28 Kentaro Hara 2011-12-05 18:41:17 PST
(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.
Comment 29 Tony Chang 2011-12-06 09:40:13 PST
(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.
Comment 30 Kentaro Hara 2011-12-06 17:47:50 PST
(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.)
Comment 31 Kentaro Hara 2011-12-07 16:02:37 PST
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?
Comment 32 Tony Chang 2011-12-07 16:25:24 PST
(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.
Comment 33 Kentaro Hara 2011-12-07 22:37:48 PST
Created attachment 118332 [details]
final patch?
Comment 34 Kentaro Hara 2011-12-07 22:46:10 PST
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+.
Comment 35 WebKit Review Bot 2011-12-08 00:20:42 PST
Comment on attachment 118332 [details]
final patch?

Clearing flags on attachment: 118332

Committed r102321: <http://trac.webkit.org/changeset/102321>
Comment 36 WebKit Review Bot 2011-12-08 00:20:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Kentaro Hara 2011-12-08 05:48:47 PST
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.
Comment 38 WebKit Review Bot 2011-12-08 16:04:51 PST
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
Comment 39 Kentaro Hara 2011-12-08 16:11:05 PST
Created attachment 118484 [details]
patch for commit
Comment 40 WebKit Review Bot 2011-12-08 18:49:46 PST
Comment on attachment 118484 [details]
patch for commit

Clearing flags on attachment: 118484

Committed r102416: <http://trac.webkit.org/changeset/102416>
Comment 41 WebKit Review Bot 2011-12-08 18:49:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Tony Chang 2011-12-09 13:42:08 PST
Reverted r102416 for reason:

Chromium Win clobber builds are failing


Committed r102472: <http://trac.webkit.org/changeset/102472>
Comment 43 Tony Chang 2011-12-09 13:43:26 PST
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.
Comment 44 Tony Chang 2011-12-09 14:11:21 PST
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.
Comment 45 Kentaro Hara 2011-12-09 16:50:09 PST
(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.
Comment 46 Kentaro Hara 2011-12-11 18:49:08 PST
Created attachment 118721 [details]
fixed Chromium/Win build failure
Comment 47 Kentaro Hara 2011-12-11 18:58:29 PST
(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;
Comment 48 WebKit Review Bot 2011-12-11 19:55:23 PST
Comment on attachment 118721 [details]
fixed Chromium/Win build failure

Clearing flags on attachment: 118721

Committed r102556: <http://trac.webkit.org/changeset/102556>
Comment 49 WebKit Review Bot 2011-12-11 19:55:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 50 Kentaro Hara 2011-12-12 05:41:37 PST
Reverted r102556 for reason:

clobber build failure

Committed r102572: <http://trac.webkit.org/changeset/102572>
Comment 51 Kentaro Hara 2011-12-12 16:17:48 PST
Created attachment 118900 [details]
Patch
Comment 52 Kentaro Hara 2011-12-12 16:18:47 PST
(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.
Comment 53 Adam Barth 2011-12-12 16:22:23 PST
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?
Comment 54 Kentaro Hara 2011-12-12 16:25:33 PST
(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.
Comment 55 Kentaro Hara 2011-12-12 17:09:12 PST
Created attachment 118914 [details]
patch for commit
Comment 56 Tony Chang 2011-12-12 17:11:53 PST
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.
Comment 57 Adam Barth 2011-12-12 17:12:28 PST
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.  ;)
Comment 58 Kentaro Hara 2011-12-12 17:22:00 PST
(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...
Comment 59 Kentaro Hara 2011-12-13 00:26:13 PST
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?
Comment 60 Kentaro Hara 2011-12-13 00:26:53 PST
Created attachment 118967 [details]
Patch
Comment 61 Adam Barth 2011-12-13 00:40:16 PST
> 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.
Comment 62 WebKit Review Bot 2011-12-13 02:06:23 PST
Comment on attachment 118967 [details]
Patch

Clearing flags on attachment: 118967

Committed r102663: <http://trac.webkit.org/changeset/102663>
Comment 63 WebKit Review Bot 2011-12-13 02:06:31 PST
All reviewed patches have been landed.  Closing bug.