Make blink::protocol::StringBuilder a distinct type from WTF::StringBuilder.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/31c53d7095041a1fdd3e17a0c84d23f411f7933a

[Łukasz Anforowicz, 2017-01-24 22:17:40]

Description was changed from

==========
Make blink::protocol::StringBuilder a distinct type from WTF::StringBuilder.

This CL is needed to make sure that code generated by
//third_party/inspector_protocol keeps working after the Great Blink
Rename.  The generate code requires presence of
blink::protocol::StringBuilder type that exposes |append(...)| and
|toString(...)| methods - these methods would be spelled differently in
the WTF namespace after the Great Blink Rename.

BUG=683447
==========

to

==========
Make blink::protocol::StringBuilder a distinct type from WTF::StringBuilder.

This CL is needed to make sure that code generated by
//third_party/inspector_protocol keeps working after the Great Blink
Rename.  The generate code requires presence of
blink::protocol::StringBuilder type that exposes |append(...)| and and
|reserveCapacity(size_t)| and |toString()| methods - these methods would
be spelled differently in the WTF namespace after the Great Blink
Rename.

BUG=683447
==========

[Łukasz Anforowicz, 2017-01-24 22:17:48]

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

[Łukasz Anforowicz, 2017-01-24 22:17:48]

dgozman@, can you PTAL?  This CL is a Good Thing (tm), regardless of the Great
Blink Rename, right?

[Łukasz Anforowicz, 2017-01-24 22:18:03]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 22:18:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 23:44:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 23:44:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-01-25 00:12:10]

On 2017/01/24 22:17:48, Łukasz Anforowicz wrote:
> dgozman@, can you PTAL?  This CL is a Good Thing (tm), regardless of the Great
> Blink Rename, right?

Hmmm... tryjobs are red - I'll have to take a closer look...  Sorry about that -
I thought that if a change like this builds, then it should pass :-)

[dgozman, 2017-01-25 20:18:24]

https://codereview.chromium.org/2654923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/V8InspectorString.h (right):

https://codereview.chromium.org/2654923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorString.h:69: static void
builderReserve(StringBuilder& builder, unsigned capacity) {
Similarly to other change, this StringUtil class is intended to be an adapter.
It's ugly but small and flexible and serves the purpose (we have a couple of
string adapters throughout codebase). Let's just add more methods here.

[Łukasz Anforowicz, 2017-01-26 19:38:21]

Description was changed from

==========
Make blink::protocol::StringBuilder a distinct type from WTF::StringBuilder.

This CL is needed to make sure that code generated by
//third_party/inspector_protocol keeps working after the Great Blink
Rename.  The generate code requires presence of
blink::protocol::StringBuilder type that exposes |append(...)| and and
|reserveCapacity(size_t)| and |toString()| methods - these methods would
be spelled differently in the WTF namespace after the Great Blink
Rename.

BUG=683447
==========

to

==========
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).

BUG=683447
==========

[Łukasz Anforowicz, 2017-01-26 19:40:16]

Description was changed from

==========
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).

BUG=683447
==========

to

==========
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).

The new StringUtil methods introduced by this CL are temporarily unused,
until we land corresponding changes to upstream of
third_party/inspector_protocol.  Also note that similar changes will
need to be made to v8/src/inspector/string-util.h.

BUG=683447
==========

[Łukasz Anforowicz, 2017-01-26 19:50:03]

dgozman@, can you please take another look?


Can you please help me figure out how I should land related changes to
third_party/inspector_protocol?

1. High-level question: which upstream repo I should work with?  I see
https://chromium.googlesource.com/deps/inspector_protocol, but I am not sure if
this is the repo to work with - should I maybe land something in (the real /
non-blink) WebKit?

2. Low-level question: any pointers to contribution/landing process docs would
be great.  I looked at third_party/inspector_protocol/README.chromium, but found
only generic info there.  In particular, I am not sure how does one roll
chromium DEPS forward:
2.a. which repo (only Chromium vs also something to be done in
https://chromium.googlesource.com/deps/inspector_protocol)
2.b. roll mechanics (e.g. I don't see "inspector" substring in chromium/src/DEPS
- does that mean that I should land directly in chromium repo?)


PS1. I understand that I also need to land corresponding changes in v8.  I think
I understand the process to do this (I hope that git cl upload will just work +
that I'll be able to figure out how to land).  I'll upload a v8 CL once I get
more confidence about the overall process (i.e. an ACK/CR for the current CR +
some understanding of how to land+roll third_party/inspector_protocol).

PS2. I am not covering String::length in this CL - based on limited testing I've
done so far I am optimistic that blacklisting renaming of |length| is the right
path forward (i.e. if blacklisting works, then there is no need to add |length|
to StringUtil).

On 2017/01/25 20:18:24, dgozman wrote:
>
https://codereview.chromium.org/2654923002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/V8InspectorString.h (right):
> 
>
https://codereview.chromium.org/2654923002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/V8InspectorString.h:69: static void
> builderReserve(StringBuilder& builder, unsigned capacity) {
> Similarly to other change, this StringUtil class is intended to be an adapter.
> It's ugly but small and flexible and serves the purpose (we have a couple of
> string adapters throughout codebase). Let's just add more methods here.

Makes sense - this seems cleaner / better for the long term.  Done.

[Łukasz Anforowicz, 2017-01-26 19:50:06]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 19:50:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2017-01-26 19:56:14]

The code looks good. Questions below make me thing we need a "contribution
guide" somewhere :-)

> Can you please help me figure out how I should land related changes to
> third_party/inspector_protocol?
> 
> 1. High-level question: which upstream repo I should work with?  I see
> https://chromium.googlesource.com/deps/inspector_protocol, but I am not sure
if
> this is the repo to work with - should I maybe land something in (the real /
> non-blink) WebKit?

deps/inspector_protocol is the correct one.


> > 2. Low-level question: any pointers to contribution/landing process docs
would
> be great.  I looked at third_party/inspector_protocol/README.chromium, but
found
> only generic info there.  In particular, I am not sure how does one roll
> chromium DEPS forward:
> 2.a. which repo (only Chromium vs also something to be done in
> https://chromium.googlesource.com/deps/inspector_protocol)
> 2.b. roll mechanics (e.g. I don't see "inspector" substring in
chromium/src/DEPS
> - does that mean that I should land directly in chromium repo?)

Once patch lands to deps/inspector_protocol, roll manually by copying files into
third_party/inspector_protocol (see README.chromium for which files to copy).
This should be done separately in V8 vs Chromium, which makes it easier to land
API breaking changes.


> 
> PS1. I understand that I also need to land corresponding changes in v8.  I
think
> I understand the process to do this (I hope that git cl upload will just work
+
> that I'll be able to figure out how to land).  I'll upload a v8 CL once I get
> more confidence about the overall process (i.e. an ACK/CR for the current CR +
> some understanding of how to land+roll third_party/inspector_protocol).

Same thing as above - copy files to third_party/inspector_protocol and send a
patch to me. Look for an example by running git log
third_party/inspector_protocol.

 
> PS2. I am not covering String::length in this CL - based on limited testing
I've
> done so far I am optimistic that blacklisting renaming of |length| is the
right
> path forward (i.e. if blacklisting works, then there is no need to add
|length|
> to StringUtil).

Sounds good.

Thank you for working on this!

[Łukasz Anforowicz, 2017-01-26 20:35:20]

Description was changed from

==========
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).

The new StringUtil methods introduced by this CL are temporarily unused,
until we land corresponding changes to upstream of
third_party/inspector_protocol.  Also note that similar changes will
need to be made to v8/src/inspector/string-util.h.

BUG=683447
==========

to

==========
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).

BUG=683447
==========

[Łukasz Anforowicz, 2017-01-26 20:43:33]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 20:44:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-01-26 20:47:28]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 20:47:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 20:56:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 20:56:11]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2017-01-26 22:07:16]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:07:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 22:20:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 22:20:46]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2017-01-26 22:36:23]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:37:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 22:53:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 22:53:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2017-01-26 23:00:44]

dgozman@, could you PTAL?

https://codereview.chromium.org/2654923002/diff/60001/third_party/inspector_p...
File third_party/inspector_protocol/README.chromium (right):

https://codereview.chromium.org/2654923002/diff/60001/third_party/inspector_p...
third_party/inspector_protocol/README.chromium:5: Revision: DO NOT SUBMIT / TODO
/ FIXME
Before landing *this* CL, I'll update the revision above to point to
https://codereview.chromium.org/2655203003 (once it lands in the other repo - in
https://chromium.googlesource.com/deps/inspector_protocol).

[dgozman, 2017-01-26 23:11:43]

Please change the patch title and description to reflect the contents, and
include the part about "rolling third_party/inspector to <hash>".

lgtm

[Łukasz Anforowicz, 2017-01-27 17:08:39]

Description was changed from

==========
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).

BUG=683447
==========

to

==========
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:
73028acaa3646789fd2a3bfd0d79eb2d91b696b3.

BUG=683447
==========

[Łukasz Anforowicz, 2017-01-27 17:08:44]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 17:09:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-01-27 17:11:45]

Description was changed from

==========
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:
73028acaa3646789fd2a3bfd0d79eb2d91b696b3.

BUG=683447
==========

to

==========
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:
1a131872167f0f7653629326891aa3ec94417f27.

BUG=683447
==========

[Łukasz Anforowicz, 2017-01-27 17:13:24]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 17:13:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-01-27 17:16:24]

On 2017/01/26 23:11:43, dgozman wrote:
> Please change the patch title and description to reflect the contents, and
> include the part about "rolling third_party/inspector to <hash>".

Done - I've added the following paragraph to the CL description:

    This CL also includes roll of inspector_protocol which starts to
    generate code that uses the new methods of StringUtil adapter:
    1a131872167f0f7653629326891aa3ec94417f27.

I've also updated README.chromium to point at the current revision/hash of
inspector_protocol.

> lgtm

I am running a CQ dry-run right now.  I plan to land via CQ later today - please
shout if you'd like me to make some additional tweaks before doing this.

I think now I've got the right commit hash (initially I've used the local one -
oops...).

[Łukasz Anforowicz, 2017-01-27 18:23:01]

Description was changed from

==========
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:
1a131872167f0f7653629326891aa3ec94417f27.

BUG=683447
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-01-27 19:51:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 19:51:10]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-27 20:02:14]

On 2017/01/27 17:16:24, Łukasz Anforowicz wrote:
> On 2017/01/26 23:11:43, dgozman wrote:
> > Please change the patch title and description to reflect the contents, and
> > include the part about "rolling third_party/inspector to <hash>".
> 
> Done - I've added the following paragraph to the CL description:
> 
>     This CL also includes roll of inspector_protocol which starts to
>     generate code that uses the new methods of StringUtil adapter:
>     1a131872167f0f7653629326891aa3ec94417f27.
> 
> I've also updated README.chromium to point at the current revision/hash of
> inspector_protocol.
> 
> > lgtm
> 
> I am running a CQ dry-run right now.  I plan to land via CQ later today -
please
> shout if you'd like me to make some additional tweaks before doing this.
> 
> I think now I've got the right commit hash (initially I've used the local one
-
> oops...).

Sounds good.

[Łukasz Anforowicz, 2017-01-27 21:08:59]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-01-27 21:08:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2654923002/#ps140001
(title: "Corrected the revision number... :-/")

[commit-bot: I haz the power, 2017-01-27 21:09:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 21:19:10]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485551339313410,
"parent_rev": "d34536f5fddb8e11e0e24c12326f5f8523cbf98c", "commit_rev":
"31c53d7095041a1fdd3e17a0c84d23f411f7933a"}

[commit-bot: I haz the power, 2017-01-27 21:19:43]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/31c53d7095041a1fdd3e17a0c84d...
==========

[commit-bot: I haz the power, 2017-01-27 21:19:45]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/31c53d7095041a1fdd3e17a0c84d...