New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 683447 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

generated code in blink::protocol expects a StringBuilder class with camelCased methods

Project Member Reported by lukasza@chromium.org, Jan 21 2017

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|.
 
The solution is probably to preland a blink::protocol::StringBuilder class that forwards to WTF::StringBuilder?  Yay... :-/
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

Comment 3 by danakj@chromium.org, 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?
> 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?

Comment 5 by danakj@chromium.org, Jan 23 2017

No to change the generator to expect Length/Append/ToString, but I feel like I'm misunderstanding something..
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.
Owner: lukasza@chromium.org
Status: Started (was: Available)
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)
Cc: dgozman@chromium.org
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 ?
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
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.
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);

Oh - and the StringUtil::length might not be needed if we blacklist renaming of |length| in https://crrev.com/2657773003
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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)
Status: Started (was: Fixed)
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).
Status: Fixed (was: Started)
Fixed by 
https://crrev.com/2668393004 (chromium) and https://crrev.com/2675763003 (v8)

Sign in to add a comment