BTW: note that some changes such as [ImplementedAs=getFoo] have already landed in
- r377546 (no weirdness AFAICT)
- r377812 (no weirdness AFAICT)
- r378055 (no weirdness AFAICT)
- r378813 (no weirdness AFAICT)
- r380225 - weirdness in gen/blink/core/events/DragEventInit.h - e.g. setGetDataTransfer
- r435958 (no weirdness AFAICT)

+esprehn@ (author of the suggestion to use the "Get" prefix -  https://crbug.com/582312#c17 )

+IDL bindings generator experts

This sounds like a problem with the dictionary code generator? Assigning to haraken@ to route it. We might need different annotations. We could also reconsider the naming conversions. :/

Using ImplementedAs to add "get" prefix doesn't make sense to me. There will be getter/setter functions for a dictionary member, so if you annotate a member with ImplementedAs=prefixFoo, it's natural for code generator to create prefixFoo() (getter) and setPrefixFoo() (setter).

A workable fix would be always add "get" prefix for getter. I can do that but I'm not sure this is what we want to do. Assigning to esprehen@.

I think we probably want to add a GetterPrefix and SetterPrefix properties. We could just prefix things with type/name conflicts as well, but that's a bit magic. Always adding Get/Set means updating code all over which is unfortunate.

OTOH almost everything is getting rewritten anyways..

I'd prefer a short term pain of renaming to a long term pain of weird hacks in future.  Just my two cents.

Yeah, either way we will rewrite everything. This seems a policy decision and I'm not sure I'm an appropriate owner of this issue. I'd prefer to have Get prefix for all dictioanry member getters rather than having annotation, because "getting something from a dictionary" sounds reasonable. Just my two cents.

I can work on this after we decide what we should do.

The policy is as described in the rename rule. I don't know what you mean by rewrite everything, we're not renaming every getter just ones where types conflict. See the recent rename patches as an example.

I thought we'll capitalize getters (and we will rewrite all call sites) but we won't? I don't follow renaming CLs...

The problem is changing getter => Getter might result in a naming conflict.

For example, for things like document(), renaming it to Document() will cause a conflict with the Document type, so we rename document() to GetDocument() instead. If we do that, we need to manually say ImplementedAs=GetDocument in the IDL.

However, in that case, the setter name looks a bit odd, because the current convention in Blink bindings is just to prepend set to the ImplementedAs name.

What about if the name starts with [gG]et[A-Z] then we don't keep the get?

re: #c17, Thanks for the explanation.  Now I understand the situation.

re: #c18, The idea sounds okay, but I'd slightly prefer adding a new extended attribute (e.g. [PrefixGet]) to an invisible magic.

bashi@, do you plan on implementing this in the next day or two? Otherwise, I think we will just use ImplementedAs=GetFoo for now. It's easy enough to fix after the rename, and there should be fairly few instances outside of non-generated code.

Sorry for not being responsive but I'm not sure what I should implement and why using Get prefix for dictionaries is a bad idea. ImplementedAs doesn't seem a reasonable workarond to me, but I don't want to block your works so please use it for now.

My understanding from comment #19 is we would add a new IDL attribute called [PrefixGet]. If this IDL attribute is present, the IDL bindings would try to call the C++ accessor as getFoo() instead of foo().

I'm okay with any short-term approach to make that renaming happen as scheduled, assuming you polish up the solution after the event.

Uploaded a CL.
https://codereview.chromium.org/2587383002/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18c57323a13690ca5efb3844e6a9531e8e1917a6

commit 18c57323a13690ca5efb3844e6a9531e8e1917a6
Author: bashi <bashi@chromium.org>
Date: Tue Dec 20 12:49:13 2016

Add [PrefixGet] extended attribute

When this extended attribute is specified on a dictionary member
called 'foo', impl class will have getFoo() method instead of
foo().

BUG= 673039 

Review-Url: https://codereview.chromium.org/2587383002
Cr-Commit-Position: refs/heads/master@{#439778}

[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/LayoutTests/fast/dom/idl-dictionary-unittest-expected.txt
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/LayoutTests/fast/dom/idl-dictionary-unittest.html
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/scripts/v8_dictionary.py
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/templates/dictionary_impl.h.tmpl
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/templates/dictionary_v8.cpp.tmpl
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.cpp
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/core/testing/DictionaryTest.cpp
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/core/testing/DictionaryTest.h
[modify] https://crrev.com/18c57323a13690ca5efb3844e6a9531e8e1917a6/third_party/WebKit/Source/core/testing/InternalDictionary.idl

bashi@, yukishiino@, can we make the new extended attribute work for attributes as well? This is useful for attributes called name, length, text, etc.

Hmm. [PrefixGet] is a short-term work-around and I'm not really happy about extending the usage.

Can we rethink about using [ImplementedAs=]? We're already defining [ImplementedAs=XXXForBindings] in a number of places.

I don't feel particularly strongly about how we make this work in the IDL bindings. If we use ImplementedAs, that means the IDL bindings generator would have to understand when "Get" can be dropped as a prefix, right? Otherwise, we end up having to implement things like setGetName() in C++, which also looks kind of weird.

From my perspective, the main goal here is two things:
- avoid non-generated code having to write setGetFoo() when setting a dictionary member that needs a 'get' prefix.
- avoid IDL code depending on things named like setGetName, e.g. we shouldn't have to define DOMWindow::setGetName().

My suggestion is to use [ImplementedAs=NameForBindings]. Then the IDL compiler will generate getNameForBindings and setNameForBindings, which doesn't look so weird to me. We're already using xxxForBindings in a number of places.

What do you think?

What an interesting discussion.

It seems to be me that one benefit of the current Blink style is that it aligned closely with the style that the Web Platform APIs use and thus allowed us to reduce the complexity of the bindings generation. This comes with an additional, intangible (and thus hard to measure) benefit of the current Blink style: it's is closer to the style that the Web Platform's primary customers -- the web developers -- use.

It appears that the switch to the Chromium style is going to negate those benefits and impose additional complexity costs.

Should I feel nervous about whether we conducted the proper cost/benefit analysis here? I can't remember now -- it's been a while since we started down this path. I can't help but feel anxious about the already-sunk costs of senior engineers trying to shove this square peg into the decidedly round hole. Do we need a timeout to re-evaluate the next steps?

It's true that we lose a bit of predictability: if something is named readyState, you'll need to notice the IDL uses [PrefixGet] or [ImplementedAs] and look for XMLHttpRequest::GetReadyState().

Let's see how many IDL attributes this impacts. If it's significant and would cause a lot of unpredictability (my sense is it does impact a fair number of things, given that fairly common names like 'name' collide), we could consider always requiring Get on things like attributes.

In the end, I think we should still try to converge styles: having two distinct code styles in the same codebase makes it harder to move code around and refactor things into the right shape. With Onion Soup, we've been moving things like the task scheduler and Mojo implementations around. If everything is named consistently, that makes these sorts of efforts much easier (in fact, the task scheduler code currently uses Google naming conventions even though it lives in Blink).

So far, we've found ~135 instances (29 unique identifiers) in the IDL that require ImplementedAs=Get...

To resolve the unpredictability aspect, I propose that the IDL bindings just always assume the presence of a 'get' prefix. We're already doing something similar for Settings.in, and CSS has something similar (although simpler, and currently only tries to dedupe collisions): https://codereview.chromium.org/2609133002/

Does that sound reasonable?

I'm fine with the plan, but given the concern like #33, I think this should be discussed in platform-architecture-dev@.

After discussion, we've arrived at consensus on an updated approach. For things exposed to via WebIDL, methods and properties will still use lowerCamelCase.

This means:
- We'll revert the changes to use PrefixGet and ImplementedAs=Get*
- We'll build tooling so we can build a list of things of class-method names that are exposed via WebIDL.
- rewrite_to_chrome_style will need to accept a list of these things and skip renaming these

In the long-term (but out of scope of the Blink rename itself), we can consider cleaning up things like the ForBindings suffix.

The next steps are:
1. Add a command line parameter for rewrite_to_chrome_style clang tool for passing a path to a file with a blacklist of IDL-related methods that should not be renamed.  Note that run_tool.py already supports --tool-args parameter.
2. Automate generating the file with a blacklist of IDL-related methods that should not be renamed.
3. (after the #1 and #2 are done and we are confident they are the right way forward) Remove ImplementedAs=get... (from IDL files) and PrefixGet (from IDL files and from the bindings compiler/generator). 

I'll get started with #1 above.  Some discussion about #2 happened (and is happening) in https://groups.google.com/a/chromium.org/d/topic/blink-reviews-bindings/wkJJRj38Ezk/discussion.

Status update:

- Step1 from #c38 is in CR at https://crrev.com/2627003003

- Step2 from #c38 is a WIP at https://crrev.com/2624413002


Interesting cases I've encounted so far:

- IDL constructor, iterable, postMessage, events map to hardcoded C++ methods (create, iterator, postMessage, add/removeEventListener).  We've decided to continue capitalizing these C++ methods in the clang tool.

- CachedAttribute with a method name spelled out inside the IDL.  The clang tool will no longer renamed methods spelled here.

- Bindings generator is oblivious to the fact that some methods are declared in a base class.
    - Example 1: getElementsByTagName method from Document.idl is really implemented in blink::ContainerNode base class, not in blink::Document.
    - Example 2: close method from SharedWorkerGlobalScope.idl is really implemented in blink::WorkerGlobalScope
    - I currently hope that there is only a limited set of such cases, and they can be hardcoded/manually accounted for in the blacklist generator from step 1 in #c38.

- The clang tool can easily skip IDL-originating methods when dealing with fully resolved CXXMethodDecl-s, but in some cases one cannot tell what type is being dealt with (i.e. if the type comes from a template parameter) - in such cases the tool falls back to capitalizing the method name.  There is currently one known case where this is a problem - nextSibling / firstChild / hasChildren / etc. in third_party/WebKit/Source/core/dom/NodeTraversal.h in templates that abstract away differenes between Node and ContainerNode.  Since this is a single case, we can deal with this manually.

- I have no idea what C++ method corresponds to URL (uppercase) attribute from Document.idl

- core/inspector/InspectorInstrumentation.idl is not a real Web IDL file

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2ee38a98ac32d112cb9092bb66ac0164adbb92c

commit f2ee38a98ac32d112cb9092bb66ac0164adbb92c
Author: lukasza <lukasza@chromium.org>
Date: Wed Jan 18 00:36:21 2017

Adding --method-blocklist <filepath> parameter to skip renaming of IDL methods.

BUG= 673039 

Review-Url: https://codereview.chromium.org/2627003003
Cr-Commit-Position: refs/heads/master@{#444205}

[modify] https://crrev.com/f2ee38a98ac32d112cb9092bb66ac0164adbb92c/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[add] https://crrev.com/f2ee38a98ac32d112cb9092bb66ac0164adbb92c/tools/clang/rewrite_to_chrome_style/tests/blocked_methods.txt
[modify] https://crrev.com/f2ee38a98ac32d112cb9092bb66ac0164adbb92c/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
[modify] https://crrev.com/f2ee38a98ac32d112cb9092bb66ac0164adbb92c/tools/clang/rewrite_to_chrome_style/tests/methods-original.cc
[add] https://crrev.com/f2ee38a98ac32d112cb9092bb66ac0164adbb92c/tools/clang/rewrite_to_chrome_style/tests/run_tool.args
[modify] https://crrev.com/f2ee38a98ac32d112cb9092bb66ac0164adbb92c/tools/clang/scripts/test_tool.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/694194cd5bd8e7ced0c59eac109f0033aa7e1f30

commit 694194cd5bd8e7ced0c59eac109f0033aa7e1f30
Author: lukasza <lukasza@chromium.org>
Date: Thu Jan 19 01:58:35 2017

--method-blocklist should also cover free functions.

One example where this is needed is blink::DOMWindowTimers which:
1. implements WindowTimers.idl and
2. is a namespace containing free functions
   (not a class containing static methods).

BUG= 673039 

Review-Url: https://codereview.chromium.org/2639813002
Cr-Commit-Position: refs/heads/master@{#444599}

[modify] https://crrev.com/694194cd5bd8e7ced0c59eac109f0033aa7e1f30/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[modify] https://crrev.com/694194cd5bd8e7ced0c59eac109f0033aa7e1f30/tools/clang/rewrite_to_chrome_style/tests/blocked_methods.txt
[modify] https://crrev.com/694194cd5bd8e7ced0c59eac109f0033aa7e1f30/tools/clang/rewrite_to_chrome_style/tests/functions-expected.cc
[modify] https://crrev.com/694194cd5bd8e7ced0c59eac109f0033aa7e1f30/tools/clang/rewrite_to_chrome_style/tests/functions-original.cc
[modify] https://crrev.com/694194cd5bd8e7ced0c59eac109f0033aa7e1f30/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
[modify] https://crrev.com/694194cd5bd8e7ced0c59eac109f0033aa7e1f30/tools/clang/rewrite_to_chrome_style/tests/methods-original.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba5cc8e55eacf6fc90a8504c34aff210ae82f7f6

commit ba5cc8e55eacf6fc90a8504c34aff210ae82f7f6
Author: dcheng <dcheng@chromium.org>
Date: Tue Mar 28 16:39:47 2017

Stop considering parameter count for methods blocked via --method-blocklist.

BUG= 673039 

Review-Url: https://codereview.chromium.org/2781483004
Cr-Commit-Position: refs/heads/master@{#460125}

[modify] https://crrev.com/ba5cc8e55eacf6fc90a8504c34aff210ae82f7f6/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[modify] https://crrev.com/ba5cc8e55eacf6fc90a8504c34aff210ae82f7f6/tools/clang/rewrite_to_chrome_style/tests/blocked_methods.txt
[modify] https://crrev.com/ba5cc8e55eacf6fc90a8504c34aff210ae82f7f6/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
[modify] https://crrev.com/ba5cc8e55eacf6fc90a8504c34aff210ae82f7f6/tools/clang/rewrite_to_chrome_style/tests/methods-original.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ce26c76894d2f88915d750303ca26c92dc63403

commit 0ce26c76894d2f88915d750303ca26c92dc63403
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Fri Sep 08 09:47:44 2017

bindings: Remove [PrefixGet]

This extended attribute was introduced for the Blink rename but wasn't
used. Let's remove it.

Bug:  673039 
Change-Id: Idd19e1de40cf1130ded056440c47999c0270c3c0
Reviewed-on: https://chromium-review.googlesource.com/654023
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500551}
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest-expected.txt
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest.html
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/bindings/IDLExtendedAttributes.md
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/bindings/scripts/v8_dictionary.py
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.cpp
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/core/testing/DictionaryTest.cpp
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/core/testing/DictionaryTest.h
[modify] https://crrev.com/0ce26c76894d2f88915d750303ca26c92dc63403/third_party/WebKit/Source/core/testing/InternalDictionary.idl