Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24053003:
Support partial interface for test support idls (Closed)
|
Created:
7 years, 3 months ago by kihong Modified:
7 years, 2 months ago CC:
blink-reviews, kojih, jsbell+bindings_chromium.org, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSupport partial interface for test support idls
Add implementation to support partial interface for test support idls.
BUG=287635, 261467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159635
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use one InterfaceDependencies.txt file #
Total comments: 12
Patch Set 3 : Remove hardcoding #
Total comments: 29
Patch Set 4 : patch #
Total comments: 14
Patch Set 5 : Patch #Patch Set 6 : Patch #
Total comments: 32
Patch Set 7 : Change namings etc. #Patch Set 8 : Patch #Patch Set 9 : patch #Patch Set 10 : patch #Messages
Total messages: 53 (0 generated)
https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... Source/bindings/derived_sources.gyp:279: '--testSupportInterfaceDependenciesFile', It is not clear to me yet why the test support IDLs need to be passed separately now. This leads to some duplication in the scripts. https://codereview.chromium.org/24053003/diff/1/Source/bindings/scripts/compu... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/1/Source/bindings/scripts/compu... Source/bindings/scripts/compute_dependencies.py:334: if global_object in test_support_interfaces: elif?
https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... Source/bindings/derived_sources.gyp:186: '--test-support-idl-files-list', Nils or haraken likely know better at this point with all the recent changes. Would it make sense to call compute_dependencies.py twice? once of regular IDLs and once for test_support ones? This way we could reuse the same script arguments and avoid duplication in the scripts.
Thanks for the CL Kihong! 1. The general approach of adding a new dependencies file: TestSupportInterfaceDependencies.txt parallel to the existing one: InterfaceDependencies.txt ...is acceptable to me. (It's understandable and straightforward, if a bit bulky.) 2. Per Christophe's excellent suggestion, these should be generated separately, via two calls to compute_dependencies.py, rather than by modifying compute_dependencies.py (to generate Yet More files), due to the duplication. 3. A cleaner solution would be to generate the dependencies once, but generate a separate list of non-test files for creating derived sources. This is a bit more involved, so I won't insist on it, but it's what I plan to do longer-term. The reason we aren't generating dependencies for test files right now is that the dependencies file is used for 2 purposes: to compute dependencies, and as a list of generated files used for derived sources in action_derivedsourcesallinone.py (That's in fact the main goal of derived_sources.gyp.) Each line in the dependencies file corresponds to a generated file, with the first field being the path. So if we put generated test files in the dependencies file, these get put into derived sources, which we don't want. So you could generate a separate "generated files" list of just the paths of the generated non-test files (in compute_dependencies.py), and then modify action_derivedsourcesallinone.py to read that file. (This might be easier or might be more involved; it's ultimately cleaner.) The specific code is: action_derivedsourcesallinone.py:204 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... 4. I've some concerns that we not change the code we actually use in Blink (shouldn't have test support in what we ship), per below comment; could we check this? https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... Source/bindings/derived_sources.gyp:186: '--test-support-idl-files-list', On 2013/09/09 07:42:09, Christophe Dumez wrote: > Nils or haraken likely know better at this point with all the recent changes. > Would it make sense to call compute_dependencies.py twice? once of regular IDLs > and once for test_support ones? > > This way we could reuse the same script arguments and avoid duplication in the > scripts. Agreed, that's much cleaner (to call twice)! (Second time don't generate the global constructors IDL files.) https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... Source/bindings/derived_sources.gyp:279: '--testSupportInterfaceDependenciesFile', On 2013/09/09 07:27:10, Christophe Dumez wrote: > It is not clear to me yet why the test support IDLs need to be passed separately > now. This leads to some duplication in the scripts. That's a good question -- it puzzled me at first too. The issue is that the dependencies file is also used by action_derivedsourcesallinone.py to generate derived sources, so we can't put the test support files in there. (This could be changed, which is a different and cleaner approach, but a bit more involved.) https://codereview.chromium.org/24053003/diff/1/Source/bindings/scripts/compu... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/1/Source/bindings/scripts/compu... Source/bindings/scripts/compute_dependencies.py:334: if global_object in test_support_interfaces: On 2013/09/09 07:27:10, Christophe Dumez wrote: > elif? I'm actually confused about whether we want to be generated the global constructor IDLs for test support interfaces... From the POV of building Blink, we don't want any test interface IDLs in there, right? Do we need the global constructor IDLs for testing? If not, we could omit it here.
> 2. Per Christophe's excellent suggestion, these should be generated separately, > via two calls to compute_dependencies.py, rather than by modifying > compute_dependencies.py (to generate Yet More files), due to the duplication. > I can try this, but we need at least one more parameter for compute_dependencies.py also because of distinguish between core idls and testsupport idls. > 3. A cleaner solution would be to generate the dependencies once, but > generate a separate list of non-test files for creating derived sources. > This is a bit more involved, so I won't insist on it, but it's what I plan > to do longer-term. > I agree. > The reason we aren't generating dependencies for test files right now is that > the dependencies file is used for 2 purposes: to compute dependencies, > and as a list of generated files used for derived sources in > action_derivedsourcesallinone.py > (That's in fact the main goal of derived_sources.gyp.) > Each line in the dependencies file corresponds to a generated file, with the > first field > being the path. > > So if we put generated test files in the dependencies file, these get put into > derived sources, which we don't want. > > So you could generate a separate "generated files" list of just the paths of the > generated non-test files (in compute_dependencies.py), and then modify > action_derivedsourcesallinone.py to read that file. Nils Did you mean add whole test supported files to the InterfaceDependencies.txt and don't append idlFiles in the action_derivedsourcesallinone.py? Am I understand rightly? > > 4. I've some concerns that we not change the code we actually use in Blink > (shouldn't have test support in what we ship), per below comment; could we check > this? > > https://codereview.chromium.org/24053003/diff/1/Source/bindings/derived_sourc... > Source/bindings/derived_sources.gyp:279: > '--testSupportInterfaceDependenciesFile', > On 2013/09/09 07:27:10, Christophe Dumez wrote: > > It is not clear to me yet why the test support IDLs need to be passed > separately > > now. This leads to some duplication in the scripts. > > That's a good question -- it puzzled me at first too. > The issue is that the dependencies file is also used by > action_derivedsourcesallinone.py > to generate derived sources, so we can't put the test support files in there. > > (This could be changed, which is a different and cleaner approach, but a bit > more involved.) I agree. > > https://codereview.chromium.org/24053003/diff/1/Source/bindings/scripts/compu... > Source/bindings/scripts/compute_dependencies.py:334: if global_object in > test_support_interfaces: > On 2013/09/09 07:27:10, Christophe Dumez wrote: > > elif? > > I'm actually confused about whether we want to be generated the global > constructor IDLs for test support interfaces... > From the POV of building Blink, we don't want any test interface IDLs in there, > right? > Do we need the global constructor IDLs for testing? > If not, we could omit it here. I will check this again but I think we can omit this. Thanks for comments chris and Nils. :)
On 2013/09/10 11:56:30, kihong wrote: > > 2. Per Christophe's excellent suggestion, these should be generated > separately, > > via two calls to compute_dependencies.py, rather than by modifying > > compute_dependencies.py (to generate Yet More files), due to the duplication. > > > I can try this, but we need at least one more parameter for > compute_dependencies.py also > because of distinguish between core idls and testsupport idls. Why do we need to distinguish in the scripts? I understand that we would need to make some arguments optional because they are not needed for testsupport idls, however, I'm not sure why the script needs to care if IDLs are testsupport or not. For example, the arguments regarding the Constructors partial interfaces to generate can be omitted. In this case, the script would simply not generate those.
Chris > Why do we need to distinguish in the scripts? I understand that we would need to > make some arguments optional because they are not needed for testsupport idls, > however, I'm not sure why the script needs to care if IDLs are testsupport or > not. I agree, we can make some optional arguments but we don't need to think about that in the in the current patch. Please take a look at it. :)
https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... File Source/core/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:205: if line.find('testing') != -1: I am not a fan of hardcoding such things in the scripts. This looks hackish IMHO.
On 2013/09/11 12:54:24, Christophe Dumez wrote: > https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... > File Source/core/scripts/action_derivedsourcesallinone.py (right): > > https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... > Source/core/scripts/action_derivedsourcesallinone.py:205: if > line.find('testing') != -1: > I am not a fan of hardcoding such things in the scripts. This looks hackish > IMHO. Basically I totally agree with your opinion. But there is no way to distinguish between core idls and test support idls. If you have an idea about that please let me know. Thanks chris.
On 2013/09/11 14:58:47, kihong wrote: > On 2013/09/11 12:54:24, Christophe Dumez wrote: > > > https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... > > File Source/core/scripts/action_derivedsourcesallinone.py (right): > > > > > https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... > > Source/core/scripts/action_derivedsourcesallinone.py:205: if > > line.find('testing') != -1: > > I am not a fan of hardcoding such things in the scripts. This looks hackish > > IMHO. > > Basically I totally agree with your opinion. > But there is no way to distinguish between core idls and test support idls. > If you have an idea about that please let me know. > Thanks chris. Well I had an idea, it was to call the compute_dependencies.py twice and make some of the script arguments optional (e.g. the path to the constructors file) and not generate those files if the parameters are omitted. I haven't tried it but it may be enough. This way, the script would not need to know the IDLs are test IDLs, it would just know not to generate some files (e.g. Window constructors file).
On 2013/09/11 15:04:02, Christophe Dumez wrote: > On 2013/09/11 14:58:47, kihong wrote: > > On 2013/09/11 12:54:24, Christophe Dumez wrote: > > > > > > https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... > > > File Source/core/scripts/action_derivedsourcesallinone.py (right): > > > > > > > > > https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... > > > Source/core/scripts/action_derivedsourcesallinone.py:205: if > > > line.find('testing') != -1: > > > I am not a fan of hardcoding such things in the scripts. This looks hackish > > > IMHO. > > > > Basically I totally agree with your opinion. > > But there is no way to distinguish between core idls and test support idls. > > If you have an idea about that please let me know. > > Thanks chris. > > Well I had an idea, it was to call the compute_dependencies.py twice and make > some of the script arguments optional (e.g. the path to the constructors file) > and not generate those files if the parameters are omitted. I haven't tried it > but it may be enough. > This way, the script would not need to know the IDLs are test IDLs, it would > just know not to generate some files (e.g. Window constructors file). Nils, could you tell me how you think about chris's proposal? I think it could be worked.
On 2013/09/11 15:38:55, kihong wrote:
> Nils,
> could you tell me how you think about chris's proposal?
> I think it could be worked.
Hi Kihong, thanks for the updated CL and sorry for the delayed response (was out
yesterday).
This approach with a single dependencies file looks much cleaner!
I agree with Chris's concern about hardcoding 'testing'.
Since you're going this way, with a little tweaking we can actually make it very
clean
(basically what I was planning). This requires 2 changes; also have a few minor
remarks.
1. Output a 'sources file'. E.g., --sources-file
This should just contain the (non-testing) generated IDL file names, which
clearly splits 'dependencies' from 'sources', and means we can add dependencies
without derivedsources getting changed.
parse_idl_files should have a new return value, let's call it 'sources'.
This should just be a list of the 'idl_file_path' values; we don't need a dict
with the dependencies.
(This lets us remove """idlFileName = line.rstrip().split(' ')[0]""" from the
derived sources script.)
All you then need to do is not add the testing files to the sources, but do add
them to the dependencies.
2. Pass two file lists: --idl-files-list (existing) and --testing-idl-files-list
(or --non-sources-idl-files-list) into compute_dependencies.
This means you don't need to hard-code the string 'testing' to determine if
something is a testing file.
Thus the main line in compute_dependencies will look like:
interfaces, sources, dependencies, global_constructors, event_names =
parse_idl_files(idl_files, testing_idl_files, global_constructors_filenames)
...and you'll just need to include the testing files when computing
dependencies, but not when listing sources.
3. Minor issue: cleaner substring.
Instead of:
.find(...) == -1
use 'not in'.
E.g., instead of
if idl_file_name.find('testing') == -1:
use:
if 'testing' not in idl_file_name:
(We can remove the hard-coding in this CL, but for future reference, for clearer
Python!)
http://docs.python.org/2/library/stdtypes.html#typesseq
Hope this is clear! This'll be a big help, cleaning up a mess in the build -
thanks!
(Minor comments.) https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... File Source/bindings/tests/idls/testing/TestSupportTestInterface.idl (right): https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/TestSupportTestInterface.idl:12: * documentation and/or other materials provided with the distribution. Could you also add this?: 3. Neither the name of Samsung Electronics nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission. https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/TestSupportTestInterface.idl:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY Could you replace "APPLE" with "COPYRIGHT HOLDERS" (and remove "ITS"), as in: THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... File Source/bindings/tests/idls/testing/TestSupportTestPartialInterface.idl (right): https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/TestSupportTestPartialInterface.idl:12: * documentation and/or other materials provided with the distribution. Could you modify the copyright block as mentioned in the other file? https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... File Source/core/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:201: inputFile = open(inputFileName, 'r') While you're here, could you remove the 'r', as it's unnecessary? ('r' is the default open mode) Actually, could you use 'with' instead, as that's generally better than explicit closing? So: with open(inputFileName) as inputFile: for line in inputFile: ... if cygdriveNames: ... (Note that the inputFile.close is too low in the existing code; it should go directly after the for loop, not after 'if cygdriveNames'.) Thanks! https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:208: if idlFileName.startswith("/cygdrive"): Also, could you fix these quotes to single quotes? (Cleaning up as we go)
I changed patch which is adopted comments of nils and chris. https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... File Source/bindings/tests/idls/testing/TestSupportTestInterface.idl (right): https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/TestSupportTestInterface.idl:12: * documentation and/or other materials provided with the distribution. On 2013/09/12 03:47:17, Nils Barth wrote: > Could you also add this?: > 3. Neither the name of Samsung Electronics nor the names of its > contributors may be used to endorse or promote products derived from > this software without specific prior written permission. Done. https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/TestSupportTestInterface.idl:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY On 2013/09/12 03:47:17, Nils Barth wrote: > Could you replace "APPLE" with "COPYRIGHT HOLDERS" (and remove "ITS"), as in: > THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS Done. https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... File Source/bindings/tests/idls/testing/TestSupportTestPartialInterface.idl (right): https://codereview.chromium.org/24053003/diff/11001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/TestSupportTestPartialInterface.idl:12: * documentation and/or other materials provided with the distribution. On 2013/09/12 03:47:17, Nils Barth wrote: > Could you modify the copyright block as mentioned in the other file? Done. https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... File Source/core/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:201: inputFile = open(inputFileName, 'r') On 2013/09/12 03:47:17, Nils Barth wrote: > While you're here, could you remove the 'r', as it's unnecessary? > ('r' is the default open mode) > > Actually, could you use 'with' instead, as that's generally better than explicit > closing? > So: > with open(inputFileName) as inputFile: > for line in inputFile: > ... > > if cygdriveNames: > ... > > (Note that the inputFile.close is too low in the existing code; it should go > directly after the for loop, not after 'if cygdriveNames'.) > > Thanks! Done. https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:205: if line.find('testing') != -1: On 2013/09/11 12:54:24, Christophe Dumez wrote: > I am not a fan of hardcoding such things in the scripts. This looks hackish > IMHO. I removed whole hardcoding things. https://codereview.chromium.org/24053003/diff/11001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:208: if idlFileName.startswith("/cygdrive"): On 2013/09/12 03:47:17, Nils Barth wrote: > Also, could you fix these quotes to single quotes? > (Cleaning up as we go) Done.
Thanks Kihong, looks quite good! Main comment is that handling of the two types of IDL files in compute_dependencies.py is pretty hacky (merging then skipping, adding then removing). This could be made cleaner by reorganizing the code to first process the main bindings IDL files (compute dependencies, add to global constructors, adding to Bindings), and then just computing dependencies for the testing IDL files, rather than merging the lists. (I detail below.) With this change, I think the CL will be good to go -- thanks so much for your hard work here! https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:173: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', Could you fix capitalization, and rename to Binding*s*: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt', https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:188: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', Here too: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt', https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:369: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', Here too. (And now the build dependencies are much clearer!) https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:377: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', Ditto. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:51: parser.add_option('--idl-files-list', help='file listing all IDLs') Could you update the comment here and for --testing-idl-files-list? (They're not all IDL files.) Also, since --idl-files-list is no longer *all* IDL files, perhaps it should be renamed to --bindings-idl-files-list or something? https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:209: def parse_idl_files(idl_files, testing_idl_files, global_constructors_filenames): Could you rename 'idl_files' to something like 'bindings_idl_files', since it's no longer all IDL files? https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:273: if idl_file_name not in testing_idl_files: This is kinda hacky: it's clearer to iterate over two lists, rather than merging and then skipping. Firstly, it's cleaner to do: if idl_file_name in testing_idl_files: continue ...so you don't need to nest the second half of this. Secondly, it would be nicer to do something like: for idl_file_name in idl_files: ...do A ...do B for idl_file_name in testing_idl_files: ...do A (only) https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:301: # An IDL file's dependencies are partial interface files that extend it, If you put this loop (and the global constructors etc.) *before* computing dependencies for support files, then you don't need to add any logic to specially skip support files, since they're not included yet. Cleaner, eh? https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:313: # Remove test suport IDLs for derived_sources_all_in_one As above, this is hacky. (Also, "suport" -> "support") Rather than adding the support files and then removing them, could you not add them in the first place? https://codereview.chromium.org/24053003/diff/7001/Tools/Scripts/webkitpy/bin... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/7001/Tools/Scripts/webkitpy/bin... Tools/Scripts/webkitpy/bindings/main.py:245: passed &= all([generate_and_check_output_pl(input_file, input_directory) Could you make this a quick loop? for directory in [input_directory, testing_input_directory]: passed &= ...
Chatted with nbarth. Overall, the approach looks good. I have a couple of renaming suggestions. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--testing-idl-files-list', help='file listing all IDLs') I'd rename: --idl-files-list => --core-idl-files-list --testing-idl-files-list => --noncore-idl-files-list The point is to make it clear that we treat core IDL files specially (i.e. we have to aggregate generated code using action_derivedsourceallinone.py) because there are too many IDL files. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:54: parser.add_option('--binding-derived-source-file', help='output file') --binding-derived-source-file => --core-derived-sources-file Also, I'd be happy if you could rename all "derived sources" in the code base to "generated code". It's more consistent to the IDL compiler terminology.
https://codereview.chromium.org/24053003/diff/7001/Source/bindings/tests/idls... File Source/bindings/tests/idls/testing/TestSupportTestInterface.idl (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/tests/idls... Source/bindings/tests/idls/testing/TestSupportTestInterface.idl:32: ] interface TestSupportTestInterface { TestSupportTestInterface => NonCoreTestInterface
Some followups per haraken's naming suggestions (As he notes, this isn't just for testing, so we want a different name.) https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--testing-idl-files-list', help='file listing all IDLs') On 2013/09/17 03:36:52, haraken wrote: > > I'd rename: > > --idl-files-list => --core-idl-files-list > --testing-idl-files-list => --noncore-idl-files-list > > The point is to make it clear that we treat core IDL files specially (i.e. we > have to aggregate generated code using action_derivedsourceallinone.py) because > there are too many IDL files. Agreed, this naming makes the purposes clear. However, 'core' is potentially confusing with Source/core ('core' instead of 'modules'??) --main-idl-files-list? https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:54: parser.add_option('--binding-derived-source-file', help='output file') On 2013/09/17 03:36:52, haraken wrote: > Also, I'd be happy if you could rename all "derived sources" in the code base to > "generated code". It's more consistent to the IDL compiler terminology. Agreed that "derived sources" is pretty confusing. AFAICT it refers not to the .cpp/.h generated by the IDL compiler, but to the .cpp files which just *include* the generated code. Derived sources is a separate step, and the IDL compiler itself doesn't know or care about it. So we have: Node.idl - IDL file V8Node.h/V8Node.cpp - generated code V8DerivedSources01.cpp - "derived sources" (has #include "bindings/V8Node.cpp" and others) Maybe we can think of a better name to make the difference clearer, but we shouldn't confuse these. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:273: if idl_file_name not in testing_idl_files: BTW, to elaborate: The code: interface_name, _ = os.path.splitext(os.path.basename(idl_file_name)) ... implemented_somewhere |= set(implemented_interfaces) ...can be factored out to a nested function (compute_dependencies?) which takes one argument, idl_file_name, and modifies the other variables in-place (you'll need to change |= to .update). This avoids duplicating the code. This function can then be called first for the (usual) idl_files_list, and then (after doing other things like global constructors), for the testing_idl_files. Does this make sense?
https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--testing-idl-files-list', help='file listing all IDLs') On 2013/09/17 05:17:46, Nils Barth wrote: > On 2013/09/17 03:36:52, haraken wrote: > > > > I'd rename: > > > > --idl-files-list => --core-idl-files-list > > --testing-idl-files-list => --noncore-idl-files-list > > > > The point is to make it clear that we treat core IDL files specially (i.e. we > > have to aggregate generated code using action_derivedsourceallinone.py) > because > > there are too many IDL files. > > Agreed, this naming makes the purposes clear. > However, 'core' is potentially confusing with Source/core ('core' instead of > 'modules'??) > --main-idl-files-list? We don't need to think core indicate Source/core but still core-idl/noncore-idl makes confuse because of noncore-idl. IMHO, noncore-idl looks like including idls which are in the modules. core-idl / test-idl main-idl / assistance-idl If there is no objection, I would like to use core-idl/test-idl. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:54: parser.add_option('--binding-derived-source-file', help='output file') On 2013/09/17 05:17:46, Nils Barth wrote: > On 2013/09/17 03:36:52, haraken wrote: > > Also, I'd be happy if you could rename all "derived sources" in the code base > to > > "generated code". It's more consistent to the IDL compiler terminology. > > Agreed that "derived sources" is pretty confusing. > AFAICT it refers not to the .cpp/.h generated by the IDL compiler, but to the > .cpp files which just *include* the generated code. > Derived sources is a separate step, and the IDL compiler itself doesn't know or > care about it. > > So we have: > Node.idl - IDL file > V8Node.h/V8Node.cpp - generated code > V8DerivedSources01.cpp - "derived sources" (has #include "bindings/V8Node.cpp" > and others) > > Maybe we can think of a better name to make the difference clearer, but we > shouldn't confuse these. "--binding-derived-source-file" indicates files list for bindings_derived_sources Therefore how about --derived-source-list-file ?
https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--testing-idl-files-list', help='file listing all IDLs') On 2013/09/17 06:33:04, kihong wrote: > On 2013/09/17 05:17:46, Nils Barth wrote: > > On 2013/09/17 03:36:52, haraken wrote: > > > > > > I'd rename: > > > > > > --idl-files-list => --core-idl-files-list > > > --testing-idl-files-list => --noncore-idl-files-list > > > > > > The point is to make it clear that we treat core IDL files specially (i.e. > we > > > have to aggregate generated code using action_derivedsourceallinone.py) > > because > > > there are too many IDL files. > > > > Agreed, this naming makes the purposes clear. > > However, 'core' is potentially confusing with Source/core ('core' instead of > > 'modules'??) > > --main-idl-files-list? > > We don't need to think core indicate Source/core but still core-idl/noncore-idl > makes confuse because of noncore-idl. > IMHO, noncore-idl looks like including idls which are in the modules. > > core-idl / test-idl > main-idl / assistance-idl > > If there is no objection, I would like to use core-idl/test-idl. We're planning to use --testing-idl-files not only for testing/ but for EventSender bindings and other bindings in Blink. So, what we really want is core-and-modules-idl-files/other-idl-files. If you don't like core-idl-files/noncore-idl-files (because of the presence of modules/), maybe you can use core-idl-files/assistance-idl-files?
https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--testing-idl-files-list', help='file listing all IDLs') On 2013/09/17 06:42:06, haraken wrote: > If you don't like core-idl-files/noncore-idl-files (because of the presence of > modules/), maybe you can use core-idl-files/assistance-idl-files? Instead of "assistance", maybe "support"? (More idiomatic.)
On 2013/09/17 06:47:14, Nils Barth wrote: > > If you don't like core-idl-files/noncore-idl-files (because of the presence of > > modules/), maybe you can use core-idl-files/assistance-idl-files? > > Instead of "assistance", maybe "support"? > (More idiomatic.) SGTM.
On 2013/09/17 06:49:22, haraken wrote: > On 2013/09/17 06:47:14, Nils Barth wrote: > > > If you don't like core-idl-files/noncore-idl-files (because of the presence > of > > > modules/), maybe you can use core-idl-files/assistance-idl-files? > > > > Instead of "assistance", maybe "support"? > > (More idiomatic.) > > SGTM. Thank you Nils and Kentaro. I will change this like core-idl-files/support-idl-files. :)
https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:173: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', On 2013/09/17 02:23:35, Nils Barth wrote: > Could you fix capitalization, and rename to Binding*s*: > '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt', Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:188: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', On 2013/09/17 02:23:35, Nils Barth wrote: > Here too: > '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt', Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:369: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', On 2013/09/17 02:23:35, Nils Barth wrote: > Here too. > (And now the build dependencies are much clearer!) Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:377: '<(SHARED_INTERMEDIATE_DIR)/blink/BindingDeriveDsources.txt', On 2013/09/17 02:23:35, Nils Barth wrote: > Ditto. Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--testing-idl-files-list', help='file listing all IDLs') On 2013/09/17 06:47:15, Nils Barth wrote: > On 2013/09/17 06:42:06, haraken wrote: > > If you don't like core-idl-files/noncore-idl-files (because of the presence of > > modules/), maybe you can use core-idl-files/assistance-idl-files? > > Instead of "assistance", maybe "support"? > (More idiomatic.) Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:209: def parse_idl_files(idl_files, testing_idl_files, global_constructors_filenames): On 2013/09/17 02:23:35, Nils Barth wrote: > Could you rename 'idl_files' to something like 'bindings_idl_files', since it's > no longer all IDL files? Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/scripts/co... Source/bindings/scripts/compute_dependencies.py:273: if idl_file_name not in testing_idl_files: On 2013/09/17 02:23:35, Nils Barth wrote: > This is kinda hacky: > it's clearer to iterate over two lists, rather than merging and then skipping. > > Firstly, it's cleaner to do: > if idl_file_name in testing_idl_files: > continue > ...so you don't need to nest the second half of this. > > Secondly, it would be nicer to do something like: > for idl_file_name in idl_files: > ...do A > ...do B > > for idl_file_name in testing_idl_files: > ...do A (only) Done. https://codereview.chromium.org/24053003/diff/7001/Source/bindings/tests/idls... File Source/bindings/tests/idls/testing/TestSupportTestInterface.idl (right): https://codereview.chromium.org/24053003/diff/7001/Source/bindings/tests/idls... Source/bindings/tests/idls/testing/TestSupportTestInterface.idl:32: ] interface TestSupportTestInterface { On 2013/09/17 03:44:29, haraken wrote: > > TestSupportTestInterface => NonCoreTestInterface Done. https://codereview.chromium.org/24053003/diff/7001/Tools/Scripts/webkitpy/bin... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/7001/Tools/Scripts/webkitpy/bin... Tools/Scripts/webkitpy/bindings/main.py:245: passed &= all([generate_and_check_output_pl(input_file, input_directory) On 2013/09/17 02:23:35, Nils Barth wrote: > Could you make this a quick loop? > for directory in [input_directory, testing_input_directory]: > passed &= ... Done.
Thanks for revisions! Some style nits, but otherwise LGTM! I'm not an OWNER, so Chris, haraken, could you take a quick look? https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:51: parser.add_option('--main-idl-files-list', help='file listing all IDLs') Not all anymore; how about: 'file listing main (compiled to Blink) IDL files' https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--support-idl-files-list', help='file listing to test support IDLs') 'file listing support IDL files (not compiled to Blink, e.g. testing)' https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:76: parser.error('Must specify the file listing all IDLs using --idl-files-list.') 'Must specify a file listing main IDL files using --main-idl-files-list.' https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:78: parser.error('Must specify the file listing all IDLs using --support-idl-files-list.') 'Must specify a file listing support IDL files using --support-idl-files-list.' https://codereview.chromium.org/24053003/diff/39001/Source/core/scripts/actio... File Source/core/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/39001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:210: inputFile.close() Could you remove inputFile.close() ? (It's not needed with 'with'.) https://codereview.chromium.org/24053003/diff/39001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/39001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:148: idl_paths = [os.path.join(support_input_directory, input_file) Could you call this 'support_idl_paths'? (Prefer single assignment.) https://codereview.chromium.org/24053003/diff/39001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:151: idl_files_list_contents = ''.join(idl_path + '\n' ...and this 'support_idl_files_list_contents'?
Nils, I fixed whole your comments. Thanks :) https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:51: parser.add_option('--main-idl-files-list', help='file listing all IDLs') On 2013/10/08 02:11:18, Nils Barth wrote: > Not all anymore; how about: > 'file listing main (compiled to Blink) IDL files' Done. https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:52: parser.add_option('--support-idl-files-list', help='file listing to test support IDLs') On 2013/10/08 02:11:18, Nils Barth wrote: > 'file listing support IDL files (not compiled to Blink, e.g. testing)' Done. https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:76: parser.error('Must specify the file listing all IDLs using --idl-files-list.') On 2013/10/08 02:11:18, Nils Barth wrote: > 'Must specify a file listing main IDL files using --main-idl-files-list.' Done. https://codereview.chromium.org/24053003/diff/39001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:78: parser.error('Must specify the file listing all IDLs using --support-idl-files-list.') On 2013/10/08 02:11:18, Nils Barth wrote: > 'Must specify a file listing support IDL files using --support-idl-files-list.' Done. https://codereview.chromium.org/24053003/diff/39001/Source/core/scripts/actio... File Source/core/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/39001/Source/core/scripts/actio... Source/core/scripts/action_derivedsourcesallinone.py:210: inputFile.close() On 2013/10/08 02:11:18, Nils Barth wrote: > Could you remove inputFile.close() ? > (It's not needed with 'with'.) Done. https://codereview.chromium.org/24053003/diff/39001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/39001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:148: idl_paths = [os.path.join(support_input_directory, input_file) On 2013/10/08 02:11:18, Nils Barth wrote: > Could you call this 'support_idl_paths'? > (Prefer single assignment.) Done. https://codereview.chromium.org/24053003/diff/39001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:151: idl_files_list_contents = ''.join(idl_path + '\n' On 2013/10/08 02:11:18, Nils Barth wrote: > ...and this 'support_idl_files_list_contents'? Done.
Thanks Kihong, looks great! BTW, one file has moved (in a different CL): Source/core/scripts/action_derivedsourcesallinone.py ...is now: Source/build/scripts/action_derivedsourcesallinone.py ...so I don't see your changes in the latest Patch Set -- could you fix this?
On 2013/10/10 04:42:12, Nils Barth wrote: > Thanks Kihong, looks great! > > BTW, one file has moved (in a different CL): > Source/core/scripts/action_derivedsourcesallinone.py > ...is now: > Source/build/scripts/action_derivedsourcesallinone.py > ...so I don't see your changes in the latest Patch Set -- could you fix this? OK, I will add that file also. Thanks
On 2013/10/10 05:01:37, kihong wrote: > On 2013/10/10 04:42:12, Nils Barth wrote: > > Thanks Kihong, looks great! > > > > BTW, one file has moved (in a different CL): > > Source/core/scripts/action_derivedsourcesallinone.py > > ...is now: > > Source/build/scripts/action_derivedsourcesallinone.py > > ...so I don't see your changes in the latest Patch Set -- could you fix this? > > OK, I will add that file also. Thanks Done.
Ok, looks fine. Added Source/build owners (Adam, Eric): could you PTAL? (Build and bindings changes LGTM.)
lgtm src/build needs OWNERS=*.
On 2013/10/10 05:36:26, eseidel wrote: > src/build needs OWNERS=*. That can be arranged: Add OWNERS=* to Source/build https://codereview.chromium.org/26574010/
https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:40: 'main_idl_files': [ Nit: Actually, "main" and "support" are not that descriptive. However, I have any better idea :( https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:46: '<@(webcore_test_support_idl_files)', webcore_test_support_idl_files => webcore_testing_support_idl_files https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:48: 'test_support_idl_files': [ test_support_idl_files => testing_support_idl_files https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:49: '<@(webcore_test_support_idl_files)', webcore_test_support_idl_files => webcore_testing_support_idl_files https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:50: '<@(generated_webcore_test_support_idl_files)', generated_webcore_test_support_idl_files => generated_webcore_testing_support_idl_files https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:232: def remove_implementation_redundancy(dependencies, interface_name_to_idl_file, implemented_somewhere): remove_implementation_redundancy => remove_interfaces_implemented_somewhere ? https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:270: main IDL filename except test support IDLs list of main IDL file names (except support IDL file names) https://codereview.chromium.org/24053003/diff/51001/Source/bindings/tests/idl... File Source/bindings/tests/idls/testing/NonCoreTestInterface.idl (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/NonCoreTestInterface.idl:32: ] interface NonCoreTestInterface { NonCoreTestInterface => SupportTestInterface https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:56: support_input_directory = os.path.join('bindings', 'tests', 'idls', 'testing') Why do we want to allocate a separate directory for testing/? Could you put the test files to bindings/tests/idls/? https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:166: '--bindings-derived-sources-file', self.derived_sources_list_filename, We might want to rename "derived sources" in the code base to "generated". You can do it in a follow-up CL.
https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:166: '--bindings-derived-sources-file', self.derived_sources_list_filename, On 2013/10/10 05:57:51, haraken wrote: > We might want to rename "derived sources" in the code base to "generated". You > can do it in a follow-up CL. Nono, these are different. The IDL compiler generates .h/.cpp files. The derived sources are files like V8DerivedSources01.cpp that just include the generated ones, hence "derived".
Sorry, my previous lgtm was more about it being OK to change build/ than that I'd looked in depth at the change. I depend on you all to be experts in this code. :) https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:210: interface_name, _ = os.path.splitext(os.path.basename(idl_file_name)) I miss our FileSystem abstraction from webkitpy. :) https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:215: partial_interface_name = get_partial_interface_name_from_idl(idl_file_contents) I take it get_ is chromium python style? https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:374: main_idl_files.append(string.rstrip(line, '\n')) This could be a list comprehension. with open() as list: main_idl_files = [string.rstrip(line, '\n') for line in list https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... File Source/build/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... Source/build/scripts/action_derivedsourcesallinone.py:203: with open(inputFileName) as inputFile: Woh, cxrazy this is no pep8, but I guess chromium style with studlyCaps? https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... Source/build/scripts/action_derivedsourcesallinone.py:206: if idlFileName.startswith('/cygdrive'): Bleh. Filesystem abstractions ftw.
https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:166: '--bindings-derived-sources-file', self.derived_sources_list_filename, On 2013/10/10 06:02:14, Nils Barth wrote: > On 2013/10/10 05:57:51, haraken wrote: > > We might want to rename "derived sources" in the code base to "generated". You > > can do it in a follow-up CL. > > Nono, these are different. > The IDL compiler generates .h/.cpp files. > The derived sources are files like V8DerivedSources01.cpp > that just include the generated ones, hence "derived". I meant, it would be better to rename V8DerivedSources01.cpp to V8GeneratedFiles01.cpp. We're mixing "derived sources" and "generate files/sources/code" in the code base.
https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... File Source/build/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... Source/build/scripts/action_derivedsourcesallinone.py:203: with open(inputFileName) as inputFile: On 2013/10/10 06:08:37, eseidel wrote: > Woh, cxrazy this is no pep8, but I guess chromium style with studlyCaps? Nono, you're right, this should be under_scores: http://www.chromium.org/developers/coding-style http://www.chromium.org/blink/coding-style#TOC-Python This is legacy code and we should fix it up; I'll do that in a separate CL so this CL doesn't bloat.
Yeah, sorry for the nit-spray. :) Just more python cleanup to add to the list. Thanks for working on this!
https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:166: '--bindings-derived-sources-file', self.derived_sources_list_filename, On 2013/10/10 06:13:24, haraken wrote: > I meant, it would be better to rename V8DerivedSources01.cpp to > V8GeneratedFiles01.cpp. We're mixing "derived sources" and "generate > files/sources/code" in the code base. Ok; admittedly "Derived Sources" is a bit vague.
Replies to Eric's comments. Kihong, I've fixed the Python style in action_derivedsourcesallinone.py in a separate CL https://codereview.chromium.org/26802002/ ...so if that lands first you'll need to rebase. Otherwise there's one code change (list comprehension) per Eric, and then haraken's comments (naming and testing/ directory). If you could address these please? Thanks! https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:215: partial_interface_name = get_partial_interface_name_from_idl(idl_file_contents) On 2013/10/10 06:08:37, eseidel wrote: > I take it get_ is chromium python style? No particular style AFAICT, and I don't think it's esp. consistent. In the IDL compiler we originally were using: foo = get_foo(...) ...but we decided instead to use this_foo = foo(...) ...largely because often we don't need to use a variable (can just use function in expression). https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:374: main_idl_files.append(string.rstrip(line, '\n')) On 2013/10/10 06:08:37, eseidel wrote: > This could be a list comprehension. > > with open() as list: > main_idl_files = [string.rstrip(line, '\n') for line in list Good point, also for support_idl_files. https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... File Source/build/scripts/action_derivedsourcesallinone.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/build/scripts/acti... Source/build/scripts/action_derivedsourcesallinone.py:203: with open(inputFileName) as inputFile: Python style (and bug fix!) for this file addressed in: action_derivedsourcesallinone.py: Fix Python style and conditional bug https://codereview.chromium.org/26802002/
On 2013/10/10 08:51:01, Nils Barth wrote: > Replies to Eric's comments. > Kihong, I've fixed the Python style in action_derivedsourcesallinone.py > in a separate CL > https://codereview.chromium.org/26802002/ > ...so if that lands first you'll need to rebase. > > Otherwise there's one code change (list comprehension) per Eric, > and then haraken's comments (naming and testing/ directory). > > If you could address these please? > Thanks! Sure, Thanks Nils. > https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... > Source/bindings/scripts/compute_dependencies.py:374: > main_idl_files.append(string.rstrip(line, '\n')) > On 2013/10/10 06:08:37, eseidel wrote: > > This could be a list comprehension. > > > > with open() as list: > > main_idl_files = [string.rstrip(line, '\n') for line in list > > Good point, also for support_idl_files. I agree, I will change this in this patch.
I will add "--no-find-copies" for landing. :) https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:46: '<@(webcore_test_support_idl_files)', On 2013/10/10 05:57:51, haraken wrote: > > webcore_test_support_idl_files => webcore_testing_support_idl_files Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:48: 'test_support_idl_files': [ On 2013/10/10 05:57:51, haraken wrote: > > test_support_idl_files => testing_support_idl_files Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:49: '<@(webcore_test_support_idl_files)', On 2013/10/10 05:57:51, haraken wrote: > > webcore_test_support_idl_files => webcore_testing_support_idl_files Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:50: '<@(generated_webcore_test_support_idl_files)', On 2013/10/10 05:57:51, haraken wrote: > > generated_webcore_test_support_idl_files => > generated_webcore_testing_support_idl_files Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... File Source/bindings/scripts/compute_dependencies.py (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:232: def remove_implementation_redundancy(dependencies, interface_name_to_idl_file, implemented_somewhere): On 2013/10/10 05:57:51, haraken wrote: > > remove_implementation_redundancy => remove_interfaces_implemented_somewhere ? Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:270: main IDL filename except test support IDLs On 2013/10/10 05:57:51, haraken wrote: > > list of main IDL file names (except support IDL file names) Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/scripts/c... Source/bindings/scripts/compute_dependencies.py:374: main_idl_files.append(string.rstrip(line, '\n')) On 2013/10/10 08:51:02, Nils Barth wrote: > On 2013/10/10 06:08:37, eseidel wrote: > > This could be a list comprehension. > > > > with open() as list: > > main_idl_files = [string.rstrip(line, '\n') for line in list > > Good point, also for support_idl_files. Done. https://codereview.chromium.org/24053003/diff/51001/Source/bindings/tests/idl... File Source/bindings/tests/idls/testing/NonCoreTestInterface.idl (right): https://codereview.chromium.org/24053003/diff/51001/Source/bindings/tests/idl... Source/bindings/tests/idls/testing/NonCoreTestInterface.idl:32: ] interface NonCoreTestInterface { On 2013/10/10 05:57:51, haraken wrote: > > NonCoreTestInterface => SupportTestInterface Done. https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:56: support_input_directory = os.path.join('bindings', 'tests', 'idls', 'testing') On 2013/10/10 05:57:51, haraken wrote: > > Why do we want to allocate a separate directory for testing/? Could you put the > test files to bindings/tests/idls/? If these files are in the bindings/test/idls, they are dealt like main idls. And This is same environment with testing idls. They are separated from main idl.(under core/testing directory) https://codereview.chromium.org/24053003/diff/51001/Tools/Scripts/webkitpy/bi... Tools/Scripts/webkitpy/bindings/main.py:166: '--bindings-derived-sources-file', self.derived_sources_list_filename, On 2013/10/10 06:22:52, Nils Barth wrote: > On 2013/10/10 06:13:24, haraken wrote: > > I meant, it would be better to rename V8DerivedSources01.cpp to > > V8GeneratedFiles01.cpp. We're mixing "derived sources" and "generate > > files/sources/code" in the code base. > > Ok; admittedly "Derived Sources" is a bit vague. I agree, but IMHO, it's better to change this on the other CL. There are so many derived_xxx files and variables which are not related to this patch.
LGTM. Thanks for the iterative work!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/24053003/77001
Failed to apply patch for Source/bindings/derived_sources.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/bindings/derived_sources.gyp
Hunk #2 FAILED at 147.
1 out of 8 hunks FAILED -- saving rejects to file
Source/bindings/derived_sources.gyp.rej
Patch: Source/bindings/derived_sources.gyp
Index: Source/bindings/derived_sources.gyp
diff --git a/Source/bindings/derived_sources.gyp
b/Source/bindings/derived_sources.gyp
index
201f6831d1540e842cfcf21f5d1d9d976f2f2f93..88a1e6213846f67bc073bdff04f9abd111e104a7
100644
--- a/Source/bindings/derived_sources.gyp
+++ b/Source/bindings/derived_sources.gyp
@@ -37,11 +37,18 @@
],
'variables': {
- 'idl_files': [
+ 'main_idl_files': [
'<@(core_idl_files)',
'<@(modules_idl_files)',
'<@(svg_idl_files)',
],
+ 'support_idl_files': [
+ '<@(webcore_testing_support_idl_files)',
+ ],
+ 'testing_support_idl_files': [
+ '<@(webcore_testing_support_idl_files)',
+ '<@(generated_webcore_testing_support_idl_files)',
+ ],
'compiler_module_files': [
'scripts/idl_compiler.py',
'<(DEPTH)/third_party/ply/lex.py',
@@ -140,15 +147,19 @@
'variables': {
# Write sources into a file, so that the action command line won't
# exceed OS limits.
- 'idl_files_list': '<|(idl_files_list.tmp <@(idl_files))',
+ 'main_idl_files_list': '<|(main_idl_files_list.tmp
<@(main_idl_files))',
+ 'support_idl_files_list': '<|(support_idl_files_list.tmp
<@(support_idl_files))',
},
'inputs': [
'scripts/compute_dependencies.py',
- '<(idl_files_list)',
- '<!@(cat <(idl_files_list))',
+ '<(main_idl_files_list)',
+ '<!@(cat <(main_idl_files_list))',
+ '<(support_idl_files_list)',
+ '<!@(cat <(support_idl_files_list))',
],
'outputs': [
'<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
'<@(generated_global_constructors_idl_files)',
'<(SHARED_INTERMEDIATE_DIR)/blink/EventInterfaces.in',
],
@@ -156,10 +167,14 @@
'action': [
'python',
'scripts/compute_dependencies.py',
- '--idl-files-list',
- '<(idl_files_list)',
+ '--main-idl-files-list',
+ '<(main_idl_files_list)',
+ '--support-idl-files-list',
+ '<(support_idl_files_list)',
'--interface-dependencies-file',
'<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '--bindings-derived-sources-file',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
'--window-constructors-file',
'<(SHARED_INTERMEDIATE_DIR)/blink/WindowConstructors.idl',
'--workerglobalscope-constructors-file',
@@ -186,8 +201,8 @@
'../core/core_derived_sources.gyp:generate_test_support_idls',
],
'sources': [
- '<@(idl_files)',
- '<@(webcore_test_support_idl_files)',
+ '<@(main_idl_files)',
+ '<@(testing_support_idl_files)',
],
'rules': [{
'rule_name': 'binding',
@@ -209,7 +224,7 @@
#
# If a new partial interface is added, need to regyp to update these
# dependencies, as these are computed statically at gyp runtime.
- '<!@pymod_do_main(list_idl_files_with_partial_interface
<@(idl_files))',
+ '<!@pymod_do_main(list_idl_files_with_partial_interface
<@(main_idl_files))',
# Generated IDLs are all partial interfaces, hence everything
# potentially depends on them.
'<@(generated_global_constructors_idl_files)',
@@ -249,7 +264,7 @@
'--interfaceDependenciesFile',
'<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
'--additionalIdlFiles',
- '<(webcore_test_support_idl_files)',
+ '<(testing_support_idl_files)',
'<@(preprocessor)',
'<@(write_file_only_if_changed)',
'<(RULE_INPUT_PATH)',
@@ -268,7 +283,7 @@
'action_name': 'derived_sources_all_in_one',
'inputs': [
'../build/scripts/action_derivedsourcesallinone.py',
- '<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
],
'outputs': [
'<@(derived_sources_aggregate_files)',
@@ -276,7 +291,7 @@
'action': [
'python',
'../build/scripts/action_derivedsourcesallinone.py',
- '<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
'--',
'<@(derived_sources_aggregate_files)',
],
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/24053003/77001
Failed to apply patch for Source/bindings/derived_sources.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/bindings/derived_sources.gyp
Hunk #2 FAILED at 147.
1 out of 8 hunks FAILED -- saving rejects to file
Source/bindings/derived_sources.gyp.rej
Patch: Source/bindings/derived_sources.gyp
Index: Source/bindings/derived_sources.gyp
diff --git a/Source/bindings/derived_sources.gyp
b/Source/bindings/derived_sources.gyp
index
201f6831d1540e842cfcf21f5d1d9d976f2f2f93..88a1e6213846f67bc073bdff04f9abd111e104a7
100644
--- a/Source/bindings/derived_sources.gyp
+++ b/Source/bindings/derived_sources.gyp
@@ -37,11 +37,18 @@
],
'variables': {
- 'idl_files': [
+ 'main_idl_files': [
'<@(core_idl_files)',
'<@(modules_idl_files)',
'<@(svg_idl_files)',
],
+ 'support_idl_files': [
+ '<@(webcore_testing_support_idl_files)',
+ ],
+ 'testing_support_idl_files': [
+ '<@(webcore_testing_support_idl_files)',
+ '<@(generated_webcore_testing_support_idl_files)',
+ ],
'compiler_module_files': [
'scripts/idl_compiler.py',
'<(DEPTH)/third_party/ply/lex.py',
@@ -140,15 +147,19 @@
'variables': {
# Write sources into a file, so that the action command line won't
# exceed OS limits.
- 'idl_files_list': '<|(idl_files_list.tmp <@(idl_files))',
+ 'main_idl_files_list': '<|(main_idl_files_list.tmp
<@(main_idl_files))',
+ 'support_idl_files_list': '<|(support_idl_files_list.tmp
<@(support_idl_files))',
},
'inputs': [
'scripts/compute_dependencies.py',
- '<(idl_files_list)',
- '<!@(cat <(idl_files_list))',
+ '<(main_idl_files_list)',
+ '<!@(cat <(main_idl_files_list))',
+ '<(support_idl_files_list)',
+ '<!@(cat <(support_idl_files_list))',
],
'outputs': [
'<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
'<@(generated_global_constructors_idl_files)',
'<(SHARED_INTERMEDIATE_DIR)/blink/EventInterfaces.in',
],
@@ -156,10 +167,14 @@
'action': [
'python',
'scripts/compute_dependencies.py',
- '--idl-files-list',
- '<(idl_files_list)',
+ '--main-idl-files-list',
+ '<(main_idl_files_list)',
+ '--support-idl-files-list',
+ '<(support_idl_files_list)',
'--interface-dependencies-file',
'<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '--bindings-derived-sources-file',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
'--window-constructors-file',
'<(SHARED_INTERMEDIATE_DIR)/blink/WindowConstructors.idl',
'--workerglobalscope-constructors-file',
@@ -186,8 +201,8 @@
'../core/core_derived_sources.gyp:generate_test_support_idls',
],
'sources': [
- '<@(idl_files)',
- '<@(webcore_test_support_idl_files)',
+ '<@(main_idl_files)',
+ '<@(testing_support_idl_files)',
],
'rules': [{
'rule_name': 'binding',
@@ -209,7 +224,7 @@
#
# If a new partial interface is added, need to regyp to update these
# dependencies, as these are computed statically at gyp runtime.
- '<!@pymod_do_main(list_idl_files_with_partial_interface
<@(idl_files))',
+ '<!@pymod_do_main(list_idl_files_with_partial_interface
<@(main_idl_files))',
# Generated IDLs are all partial interfaces, hence everything
# potentially depends on them.
'<@(generated_global_constructors_idl_files)',
@@ -249,7 +264,7 @@
'--interfaceDependenciesFile',
'<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
'--additionalIdlFiles',
- '<(webcore_test_support_idl_files)',
+ '<(testing_support_idl_files)',
'<@(preprocessor)',
'<@(write_file_only_if_changed)',
'<(RULE_INPUT_PATH)',
@@ -268,7 +283,7 @@
'action_name': 'derived_sources_all_in_one',
'inputs': [
'../build/scripts/action_derivedsourcesallinone.py',
- '<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
],
'outputs': [
'<@(derived_sources_aggregate_files)',
@@ -276,7 +291,7 @@
'action': [
'python',
'../build/scripts/action_derivedsourcesallinone.py',
- '<(SHARED_INTERMEDIATE_DIR)/blink/InterfaceDependencies.txt',
+ '<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
'--',
'<@(derived_sources_aggregate_files)',
],
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/24053003/87001
Message was sent while issue was closed.
Change committed as 159635
Message was sent while issue was closed.
It looks like this reverted https://codereview.chromium.org/26980003/ . Was that intentional?
Message was sent while issue was closed.
On 2013/10/17 22:49:31, Nico wrote: > It looks like this reverted https://codereview.chromium.org/26980003/ . Was that > intentional? Nope, unintentional -- just a merge/rebase error on a big CL. (Right Kihong?) I've fixed this in: Reapply/merge derived_sources.gyp useless use of cat (demoggification) https://codereview.chromium.org/27873002/
Message was sent while issue was closed.
On 2013/10/18 00:58:14, Nils Barth wrote: > On 2013/10/17 22:49:31, Nico wrote: > > It looks like this reverted https://codereview.chromium.org/26980003/ . Was > that > > intentional? > > Nope, unintentional -- just a merge/rebase error on a big CL. > (Right Kihong?) > > I've fixed this in: > Reapply/merge derived_sources.gyp useless use of cat (demoggification) > https://codereview.chromium.org/27873002/ I'm sorry for making this mistake. I should have been to take a look at that more detail. And thank you very much Nils.
Message was sent while issue was closed.
On 2013/10/18 01:27:15, kihong wrote: > I'm sorry for making this mistake. > I should have been to take a look at that more detail. > And thank you very much Nils. n/p, nothing serious this time! |
