generated code in blink::protocol expects a StringBuilder class with camelCased methods |
||||||
Issue description
First - note that protocol generator generates code not only for blink::protocol, but also elsewhere (e.g. ui::devtools::protocol).
Second - the problem is that code generator assumes that foo::protocol namespace will have a StringBuilder class that exposes methods like |append| and |toString|. Such class is provided today for blink::protocol namespace by the following using declaration:
third_party/WebKit/Source/core/inspector/V8InspectorString.h:
using StringBuilder = WTF::StringBuilder;
The problem is that after the rename WTF::StringBuilder will have |Append| and |ToString| methods, not |Append| and |ToString|.
,
Jan 21 2017
FWIW, right now I only see 3 problematic methods when trying to build obj/third_party/WebKit/Source/core/inspector/generated/Protocol.o (i.e. gen/blink/core/inspector/protocol/Protocol.cpp) after the big rename: - blink::protocol::StringBuilder::append - blink::protocol::StringBuilder::toString - blink::protocol::String::length
,
Jan 23 2017
> The solution is probably to preland a blink::protocol::StringBuilder class that forwards to WTF::StringBuilder? Yay... :-/ Hm, or include a patch to the generator in the rewrite process?
,
Jan 23 2017
> Hm, or include a patch to the generator in the rewrite process? To avoid renaming WTF::String::length (to WTF::String::Length and/or GetLength) and to similarily avoid renaming WTF::StringBuilder::append and WTF::StringBuilder::toString?
,
Jan 23 2017
No to change the generator to expect Length/Append/ToString, but I feel like I'm misunderstanding something..
,
Jan 23 2017
Oh, sorry. I should have given a link to https://crbug.com/677223#c6 - the problem is that changing the generator (i.e. third_party/inspector_protocol/lib/*.template) will impact not only blink::protocol namespace, but also ui::devtools::protocol, content::protocol, etc.
,
Jan 24 2017
I am trying to see how far I can get with #c1: 1. removing using blink::protocol::String = WTF::String 2. introducing a blink::protocol::String class derived from WTF::String - with explicitly implemented |length()| that forwards to WTF::String::length - with forwarding single-arg constructors to WTF::String - with default constructor - with '+' operator 3. changing String to blink::protocol::String in some places (e.g. in method overrides like InspectorDOMAgent::resolveNode)
,
Jan 24 2017
During the Great Blink Rename we probably do want to rename WTF::StringBuilder::toString and reserveCapacity methods to PascalCase - we have to figure out something that makes the new names interoperate fine with blink::protocol namespace (where something is maybe https://crrev.com/2654923002 that I've send out for CR to dgozman@). OTOH, maybe we don't want to rename WTF::String::length to Length. One argument against the renaming would be consistency with the naming used by std::string (and therefore ability to use templated code that assumes |length()| spelling). What do people think about this? Is blocking WTF::String::length preferrable over the perfect-forwarding and operator magic in https://crrev.com/2655783003/patch/1/10027 ?
,
Jan 26 2017
As suggested by dgozman@ in the CR referenced from #c8, the plan now is to provide the necessary adaptation via the existing mechanism - the ...::protocol::StringUtil class. This will require staging the changes as follows:
1. Add the following methods
- static unsigned length(const String&)
- static String toString(const StringBuilder&)
- required overloads of void append(StringBuilder&, ...)
to StringUtil clas in
- Chromium repo:
- content::protocol (content/browser/devtools/protocol_string.*)
- ui::devtools::protocol (components/ui_devtools/string_util.*)
- blink::protocol (third_party/WebKit/Source/core/inspector/V8InspectorString.*)
- V8 repo:
- v8_inspector::protocol (v8/src/inspector/string-util.*)
2. Roll v8
3. Modify third_party/inspector_protocol to use the new StringUtil methods, rather than calling String's and StringBuilder's methods directly.
4. Roll third_party/inspector_protocol
,
Jan 26 2017
Hmmm... actually I have no idea how to make changes to third_party/inspector_protocol. It seems to be part of the chromium repo (I don't see any '.git' directory underneath + it is not mentioned in the top-level DEPS file). So maybe step4 is not needed and non-v8 parts from step1 can be done together with step3.
,
Jan 26 2017
And a small correction on the methods required: - I forgot about |reserveCapacity|: static void reserveCapacity(StringBuilder&, size_t); - And the 3 needed overloads of |append| are: static void append(StringBuilder&, const String& s); static void append(StringBuilder&, char c); static void append(StringBuilder&, const char* buf, size_t size);
,
Jan 26 2017
Oh - and the StringUtil::length might not be needed if we blacklist renaming of |length| in https://crrev.com/2657773003
,
Jan 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31c53d7095041a1fdd3e17a0c84d23f411f7933a commit 31c53d7095041a1fdd3e17a0c84d23f411f7933a Author: lukasza <lukasza@chromium.org> Date: Fri Jan 27 21:19:06 2017 Adapt StringBuilder's append and toString methods via StringUtil helper. This is needed to insulate generated code from blink::protocol namespace from naming changes that we plan to do in the Great Blink Rename (which in particular will change wtf::StringBuilder::toString into ToString, and similarily will rename reserveCapacity and append methods). This CL also includes roll of inspector_protocol which starts to generate code that uses the new methods of StringUtil adapter: rolling third_party/inspector to 1a131872167f0f7653629326891aa3ec94417f27. BUG= 683447 Review-Url: https://codereview.chromium.org/2654923002 Cr-Commit-Position: refs/heads/master@{#446773} [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/components/ui_devtools/string_util.h [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/content/browser/devtools/protocol_string.h [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/third_party/WebKit/Source/core/inspector/V8InspectorString.h [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/third_party/inspector_protocol/README.chromium [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/third_party/inspector_protocol/lib/ErrorSupport_cpp.template [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/third_party/inspector_protocol/lib/Parser_cpp.template [modify] https://crrev.com/31c53d7095041a1fdd3e17a0c84d23f411f7933a/third_party/inspector_protocol/lib/Values_cpp.template
,
Jan 28 2017
This should now be fixed by: - StringBuilder adaptation from #c13 (together with its equivalent for v8 at https://crrev.com/2660503002; and together with https://crrev.com/2655203003 for third_party/inspector_protocol upstream) - Blacklisting renaming of |length| method (from https://crrev.com/2657773003)
,
Feb 2 2017
Reactivating - I missed one usage of String::find that also will need to go through an adapter. Maybe this is an opportunity to reevaluate also adapting String::length, but for now I think the answer is no. I think that 1) we should adapt as little as possible, 2) |length| cannot be renamed because this will break template code elsewhere (template code that assumes |length| is spelled the same inside and outside of blink).
,
Feb 7 2017
Fixed by https://crrev.com/2668393004 (chromium) and https://crrev.com/2675763003 (v8) |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by lukasza@chromium.org
, Jan 21 2017