Bug 73394

Created attachment 117118 [details] Patch

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 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())); }

> >> 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.

Created attachment 117144 [details] rebased patch for commit

(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 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

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 on attachment 117144 [details] rebased patch for commit Clearing flags on attachment: 117144 Committed r101669: <http://trac.webkit.org/changeset/101669>

Reverted r101669 for reason: Win build and Mac build are failing Committed r101672: <http://trac.webkit.org/changeset/101672>

Created attachment 117513 [details] patch for commit

Comment on attachment 117513 [details] patch for commit Clearing flags on attachment: 117513 Committed r101737: <http://trac.webkit.org/changeset/101737>

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

(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.

Reverted r101737 for reason: Chromium/Mac and Chromium/Win build are broken Committed r101740: <http://trac.webkit.org/changeset/101740>

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.

> 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.

(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.

Created attachment 117577 [details] patch for commit

(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 on attachment 117577 [details] patch for commit Clearing flags on attachment: 117577 Committed r101783: <http://trac.webkit.org/changeset/101783>

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.

(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?

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

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:\...

(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

> 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.

(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.

(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.

(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.)

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?

(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.

Created attachment 118332 [details] final patch?

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 on attachment 118332 [details] final patch? Clearing flags on attachment: 118332 Committed r102321: <http://trac.webkit.org/changeset/102321>

All reviewed patches have been landed. Closing bug.

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 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

Created attachment 118484 [details] patch for commit

Comment on attachment 118484 [details] patch for commit Clearing flags on attachment: 118484 Committed r102416: <http://trac.webkit.org/changeset/102416>

All reviewed patches have been landed. Closing bug.

Reverted r102416 for reason: Chromium Win clobber builds are failing Committed r102472: <http://trac.webkit.org/changeset/102472>

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.

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.

(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.

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

(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 on attachment 118721 [details] fixed Chromium/Win build failure Clearing flags on attachment: 118721 Committed r102556: <http://trac.webkit.org/changeset/102556>

All reviewed patches have been landed. Closing bug.

Reverted r102556 for reason: clobber build failure Committed r102572: <http://trac.webkit.org/changeset/102572>

Created attachment 118900 [details] Patch

(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 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?

(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.

Created attachment 118914 [details] patch for commit

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 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. ;)

(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...

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?

Created attachment 118967 [details] Patch

> 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 on attachment 118967 [details] Patch Clearing flags on attachment: 118967 Committed r102663: <http://trac.webkit.org/changeset/102663>

All reviewed patches have been landed. Closing bug.