ABANDONED CL: Script for getting a list of C++ methods implementing an IDL interface.

ABANDONED CL - We won't need this CL anymore, since the Great Blink Rename has already landed - yay!

================

Script for getting a list of C++ methods implementing an IDL interface.

BUG=673039

[Łukasz Anforowicz, 2017-01-12 00:46:58]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-01-12 00:46:59]

dcheng@, could you do a first review pass?

[Łukasz Anforowicz, 2017-01-19 00:03:14]

lukasza@chromium.org changed reviewers:
+ bashi@chromium.org

[Łukasz Anforowicz, 2017-01-19 00:03:14]

bashi@, could you PTAL?

Some high-level notes:

1. I don't have a strong opinion whether the new script should be under
third_party/WebKit/Source/bindings/scripts VS under
tools/clang/rewrite_to_chrome_style.

2. I think that getting a 100%-accurate tool is not possible.  The main problem
that I don't know how to solve is figuring out if an operation/attribute from
Foo.idl is implemented by blink::Foo class VS by blink::FooBase class.  There
are ~250 cases of such methods that for now I've hardcoded into the tool.

2.1. Given 2. above, I am not sure if it is worth spending more time
investigating and fixing things remaining in |print_known_issues| function.

2.2. Given 2. above, I am not sure if the extracting script will be useful in
the longer term, outside of the Great Blink Rename project.

3. dcheng@ suggested offline moving the hardcoded lists to a separate, data-only
file - I'll take care of this after we know that the rest of this CL looks okay.

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py
(right):

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py:466:
FormData:::get:::2
I've tried using IdlReader.read_idl_definitions (instead of
IdlReader.read_idl_file), but this didn't help - when entering an ad-hoc REPL in
ExtractingVisitor.visit_operation, I would still unexpectedly see
operation.idl_type.is_dictionary=False.

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py:480:
Navigator:::cookieEnabled:::0
Given the fact that the extracting script works sufficiently well (*) I think
I'd like to land this CL despite having to hardcode the entries above.

(*) After blacklisting renaming of IDL-related C++ methods, the following ninja
command succeeds:
$ ninja -C out/gn -k 29999 `cat ~/scratch/blink-targets.txt | grep
Source/binding
(blink-targets.txt contains a list of all *.o files under out/gn, after building
blink_tests before the rename).

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/scripts/test_extract_list_of_idl_methods.py
(right):

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/test_extract_list_of_idl_methods.py:85:
unexpected_entries = [
Emitting (and blacklisting renaming of) extra C++ methods is not really a
problem, but we should try to be accurate here.  If a C++ method or a method
overload is not really required by IDL files, then it should be renamed to
PascalCase.  Therefore, the extracting script 1) tries to avoid emitting methods
that are not really present/required and 2) further restricts which overloads
are considered by listing the expected number of parameters that the C++ method
should have (this is slightly complicated by presence of optional parameters -
both in IDL and in C++ side).

[haraken, 2017-01-19 00:06:34]

This may be a very stupid question but assuming that this script is used only
before the Big Blink Rename, do we need to land the script on trunk?

[Łukasz Anforowicz, 2017-01-19 00:14:42]

On 2017/01/19 00:06:34, haraken wrote:
> This may be a very stupid question but assuming that this script is used only
> before the Big Blink Rename, do we need to land the script on trunk?

This is a good question.  I think we do want to put the script under some kind
of versioning/revision-control system.

Landing in the Chromium repo / on trunk is one option - this is what we've done
with the rewrite_to_chrome_style clang tool under
tools/clang/rewrite_to_chrome_style/ directory.  OTOH, maybe your question is an
argument for putting the IDL scraping script under
tools/clang/rewrite_to_chrome_style/ directory - together with the clang tool /
away from the DOM bindings generators.

I guess an alternative would be to set up a separate git repo (or something like
this).  I don't have a strong opinion on how desirable and difficult this is.

[bashi, 2017-01-19 01:03:08]

out/Release/gen/blink/bindings/{core,modules}/*.pickle files have useful
information which this script may want to use so following would be an option.

1. Putting this script under third_party/WebKit/Source/bindings/script
2. Adding a build target which depends on "interfaces_info_individual_modules"
target and runs this script. The output will be in
out/Release/gen/blink/bindings/somewhere.

After the rename, we can remove the script and the target. I think tentative
submission is fine.

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py
(right):

https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py:466:
FormData:::get:::2
On 2017/01/19 00:03:14, Łukasz Anforowicz wrote:
> I've tried using IdlReader.read_idl_definitions (instead of
> IdlReader.read_idl_file), but this didn't help - when entering an ad-hoc REPL
in
> ExtractingVisitor.visit_operation, I would still unexpectedly see
> operation.idl_type.is_dictionary=False.

Unfortunately IdlType has many dependencies to work properly. You need some
pickle files generated by scripts in Source/bindings/scripts. See
https://docs.google.com/document/d/13yQkyEX0_y3n5Mgma6R1yEsKqPESddtzhjsvbArrE...
for details.

To work |is_dictionary| correctly you need to read
out/Release/gen/blink/bindings/modules/InterfacesInfoOverall.pickle then call
IdlType.set_dictionaries() like CodeGeneratorBase.set_global_type_info().

[haraken, 2017-01-19 01:07:58]

If you plan to remove the scraping script in a reasonable timeline, I'm fine
with putting it under bindings/scripts/.

[Łukasz Anforowicz, 2017-01-24 00:19:28]

I think I owe everyone a status update on this CL - I've been slow to respond,
because

1. I had trouble figuring out how to hook up reading of the picked information
to BUILD.gn and my script.  One of the difficulties was with deciding whether I
need core-vs-modules pickled file.

2. I am considering (https://crrev.com/2655543002) blocking method renaming
based on just classname+methodname (i.e. ignoring number of parameters).
  - This seems to work well enough to unblock trying to discover and fix
remaining compilation blockers after the big rename.
  - This avoids much of the complexity of IDL scraping.

On 2017/01/19 01:03:08, bashi wrote:
> out/Release/gen/blink/bindings/{core,modules}/*.pickle files have useful
> information which this script may want to use so following would be an option.
> 
> 1. Putting this script under third_party/WebKit/Source/bindings/script
> 2. Adding a build target which depends on "interfaces_info_individual_modules"
> target and runs this script. The output will be in
> out/Release/gen/blink/bindings/somewhere.
> 
> After the rename, we can remove the script and the target. I think tentative
> submission is fine.
> 
>
https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py
> (right):
> 
>
https://codereview.chromium.org/2624413002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/scripts/extract_list_of_idl_methods.py:466:
> FormData:::get:::2
> On 2017/01/19 00:03:14, Łukasz Anforowicz wrote:
> > I've tried using IdlReader.read_idl_definitions (instead of
> > IdlReader.read_idl_file), but this didn't help - when entering an ad-hoc
REPL
> in
> > ExtractingVisitor.visit_operation, I would still unexpectedly see
> > operation.idl_type.is_dictionary=False.
> 
> Unfortunately IdlType has many dependencies to work properly. You need some
> pickle files generated by scripts in Source/bindings/scripts. See
>
https://docs.google.com/document/d/13yQkyEX0_y3n5Mgma6R1yEsKqPESddtzhjsvbArrE...
> for details.
> 
> To work |is_dictionary| correctly you need to read
> out/Release/gen/blink/bindings/modules/InterfacesInfoOverall.pickle then call
> IdlType.set_dictionaries() like CodeGeneratorBase.set_global_type_info().

[Łukasz Anforowicz, 2017-04-14 16:31:39]

Description was changed from

==========
Script for getting a list of C++ methods implementing an IDL interface.

BUG=673039
==========

to

==========
ABANDONED CL - We won't need this CL anymore, since the Great Blink Rename has
already landed - yay!

================

Script for getting a list of C++ methods implementing an IDL interface.

BUG=673039
==========