[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound helps to avoid copies in important places (mostly when passing large protocol messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
Committed: https://crrev.com/101f402f95f5a67df948347edc4dce050ce482c7
Cr-Commit-Position: refs/heads/master@{#414846}

[dgozman, 2016-08-19 18:39:07]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

This opens possibility to:
- use WTF::String in core/inspector;
- use String16 in v8_inspector;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

BUG=637032
==========

[dgozman, 2016-08-19 18:40:43]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

Using StringView helps to avoid copies in important places (mostly when passing
large protocol messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

[dgozman, 2016-08-19 18:41:44]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org, esprehn@chromium.org, pfeldman@chromium.org

[dgozman, 2016-08-19 18:41:45]

Hi,

Could you please take a look?

Andrey - at everything.
Pavel and Elliott - at v8_inspector/public/StringView and
core/inspector/V8InspectorStringView. StringView is intended to be the final API
before we move v8_inspector to v8.


Follow up dropping String16 from core completely:
https://codereview.chromium.org/2251343003/.

Thanks,
Dmitry

[esprehn, 2016-08-19 18:52:26]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

Using StringView helps to avoid copies in important places (mostly when passing
large protocol messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

Using StringView helps to avoid copies in important places (mostly when passing
large protocol messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

[esprehn, 2016-08-19 18:53:13]

This puts a string copy at for every toCoreString() that didn't used to be
there, that's probably fine as long as we're not doing then when the inspector
is not running.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp (right):

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp:14: return
v8_inspector::StringView();
this is converting empty to null, those are two different things inside blink.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp:29: return
v8_inspector::StringView(reinterpret_cast<const uint8_t*>(string),
strlen(string));
You should either take a WTF::StringView or make this inline. Then the compiler
will figure out the length of the string for you.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp:34: if
(!string.length())
This converts zero length strings to empty strings. That means round tripping a
null string comes out empty not null.

toV8InspectorStringView(String()) => emptyString()

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

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.h:16: CORE_EXPORT
v8_inspector::StringView toV8InspectorStringView(const String&);
just take a WTF::StringView

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.h:18: CORE_EXPORT
v8_inspector::StringView toV8InspectorStringView(const char*);
remove this one

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h
(right):

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h:14: class
V8ContextInfo {
These are only ever on the stack I hope? :)

[dgozman, 2016-08-19 20:50:12]

PTAL, using WTF::StringView in converter now.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp (right):

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp:14: return
v8_inspector::StringView();
On 2016/08/19 18:53:12, esprehn wrote:
> this is converting empty to null, those are two different things inside blink.

Fixed.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp:29: return
v8_inspector::StringView(reinterpret_cast<const uint8_t*>(string),
strlen(string));
On 2016/08/19 18:53:12, esprehn wrote:
> You should either take a WTF::StringView or make this inline. Then the
compiler
> will figure out the length of the string for you.

Done.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.cpp:34: if
(!string.length())
On 2016/08/19 18:53:12, esprehn wrote:
> This converts zero length strings to empty strings. That means round tripping
a
> null string comes out empty not null.
> 
> toV8InspectorStringView(String()) => emptyString()

Fixed.

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

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.h:16: CORE_EXPORT
v8_inspector::StringView toV8InspectorStringView(const String&);
On 2016/08/19 18:53:12, esprehn wrote:
> just take a WTF::StringView

Done.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/V8InspectorStringView.h:18: CORE_EXPORT
v8_inspector::StringView toV8InspectorStringView(const char*);
On 2016/08/19 18:53:12, esprehn wrote:
> remove this one

Done.

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h
(right):

https://codereview.chromium.org/2260233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h:14: class
V8ContextInfo {
On 2016/08/19 18:53:13, esprehn wrote:
> These are only ever on the stack I hope? :)

Correct :-)

[caseq, 2016-08-19 21:57:03]

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:42: static
const char animationObjectGroup[] = "animation";
please move it into function that needs it, or use anonymous namespace if you
need to share it between functions.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:156:
contextInfo.origin = toV8InspectorStringView(originString);
ouch, passing references to a temp object once again.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:157:
contextInfo.auxData = toV8InspectorStringView(auxData);
ditto.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.cpp
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.cpp:204:
*objectGroup = toStringView(group);
This does not look good -- you're returning to the caller a reference to a
memory controlled by a stack-allocated object that is about to die.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.cpp:276:
return toStringView(json);
I'm sure we must have some very different understanding of how StringView works.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8StackTraceImpl.cpp
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8StackTraceImpl.cpp:263: return
toStringView(string);
nope-nope-nope-nope!

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h:29:
StringView humanReadableName;
This is going to be a landmine, unless you can assure this object is only
stack-allocated.

[dgozman, 2016-08-20 01:55:25]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to StringView.

Using StringView helps to avoid copies in important places (mostly when passing
large protocol messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound results
helps to avoid copies in important places (mostly when passing large protocol
messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

[dgozman, 2016-08-20 02:04:51]

Please take another look. I've added StringBuffer to pass results with
ownership.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:42: static
const char animationObjectGroup[] = "animation";
On 2016/08/19 21:57:02, caseq wrote:
> please move it into function that needs it, or use anonymous namespace if you
> need to share it between functions.

Done.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.cpp
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.cpp:204:
*objectGroup = toStringView(group);
On 2016/08/19 21:57:02, caseq wrote:
> This does not look good -- you're returning to the caller a reference to a
> memory controlled by a stack-allocated object that is about to die. 

Right! Added simple StringBuffer for this.

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h
(right):

https://codereview.chromium.org/2260233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/public/V8ContextInfo.h:29:
StringView humanReadableName;
On 2016/08/19 21:57:02, caseq wrote:
> This is going to be a landmine, unless you can assure this object is only
> stack-allocated.

Done.

[dgozman, 2016-08-21 22:00:03]

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

[commit-bot: I haz the power, 2016-08-21 22:00:09]

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, 2016-08-21 22:03:49]

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

[commit-bot: I haz the power, 2016-08-21 22:03:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2016-08-22 00:01:51]

Patchset #5 (id:80001) has been deleted

[dgozman, 2016-08-22 00:01:59]

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

[commit-bot: I haz the power, 2016-08-22 00:02:12]

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

[dgozman, 2016-08-22 00:02:33]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound results
helps to avoid copies in important places (mostly when passing large protocol
messages around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound helps to
avoid copies in important places (mostly when passing large protocol messages
around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

[commit-bot: I haz the power, 2016-08-22 00:32:58]

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

[commit-bot: I haz the power, 2016-08-22 00:32:59]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[caseq, 2016-08-22 17:53:02]

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h (right):

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h:42: static
std::unique_ptr<StringBufferImpl> create(String16&);
s/create/adopt/

This way you've got a pair of methods in base and derived class that have rather
different semantics.

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h:47:
std::unique_ptr<String16> m_owner;
can we just make it a member, i.e. drop unique_ptr<>?

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/public/String.h (right):

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:40: virtual
const StringView& string() = 0;
You're not hiding StringView, right? So this could perhaps be made an inline and
not virtual if you move m_string here perhaps?
Also, this likely needs a virtual destructor.

[dgozman, 2016-08-22 22:35:19]

PTAL

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h (right):

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h:42: static
std::unique_ptr<StringBufferImpl> create(String16&);
On 2016/08/22 17:53:02, caseq wrote:
> s/create/adopt/
> 
> This way you've got a pair of methods in base and derived class that have
rather
> different semantics.

Done.

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h:47:
std::unique_ptr<String16> m_owner;
On 2016/08/22 17:53:02, caseq wrote:
> can we just make it a member, i.e. drop unique_ptr<>?

Yes. First version used to clear it, now we can just make it a member.

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/public/String.h (right):

https://codereview.chromium.org/2260233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:40: virtual
const StringView& string() = 0;
On 2016/08/22 17:53:02, caseq wrote:
> You're not hiding StringView, right? So this could perhaps be made an inline
and
> not virtual if you move m_string here perhaps?
> Also, this likely needs a virtual destructor.

I'd prefer to make this pure virtual.

[caseq, 2016-08-22 23:46:11]

lgtm % pfeldman's concerns over growing complexity. I.e. I think this is correct
now, but I wonder what we get in exchange for things being considerably more
complex.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1170: if
(!m_v8Session->unwrapObject(errorString, toV8InspectorStringView(objectId),
&value, &context, &objectGroup))
Can we make objectGroup parameter in unwrapObject accept nullptr? Most usages
don't seem to care and pass a dummy.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:440:
std::unique_ptr<protocol::DOMDebugger::EventListener>
InspectorDOMDebuggerAgent::buildObjectForEventListener(v8::Local<v8::Context>
context, const V8EventListenerInfo& info, const String& objectGroupId)
Actually, it looks like we never use the fact that object group id is a string
-- we just pass it while converting back and forth. Should we just encapsulate
it in some opaque object?

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:503:
std::unique_ptr<protocol::DictionaryValue> description =
protocol::DictionaryValue::create();
nit: auto description = ...

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:149:
std::unique_ptr<protocol::DictionaryValue> auxDataValue =
protocol::DictionaryValue::create();
nit: auto auxDataValue = ...

[dgozman, 2016-08-23 17:15:34]

@esprehn: would you like to take another look?

[esprehn, 2016-08-23 17:35:39]

Maybe I just missed it, but I don't see where you did:

"make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations."

you're still using String16 inside core/ though, and I'd rather not have
std::basic_string stuff floating around if possible :)

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:377: static
const char animationObjectGroup[] = "animation";
constants are kFoo named, kAnimationObjectGroup

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:658: if
(url.contains(String(breakpoint.first))) {
This is allocating a string in the loop, contains actually takes a
WTF::StringView, so you want to convert to that and avoid the string
allocations.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:672:
m_v8Session->breakProgram(toV8InspectorStringView(protocol::Debugger::API::Paused::ReasonEnum::XHR),
toV8InspectorStringView(json));
protocol::Debugger::API::Paused::ReasonEnum::XHR is crazy named, you might want
do something about the code generator :P

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/V8InspectorString.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/V8InspectorString.cpp:26: if
(!string.characters8())
the string constructor does this for you.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/V8InspectorString.cpp:29: return
emptyString();
ditto

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/V8InspectorString.cpp:33: return
String();
ditto, you don't need to null check or empty check

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp:174: return
v8::String::Empty(isolate);
this turns null strings into Empty(), maybe that's what you want?

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h:38: class
StringBufferImpl : public StringBuffer {
this is very weird to have the String.h define an interface and the StringUtil.h
be an impl, I guess it's not part of public/ ?

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/public/String.h (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:19:
StringView(const uint8_t* characters, unsigned length) : m_is8Bit(true),
m_length(length) { m_characters8 = characters; }
you can just initialize an unnamed union member directly, 

m_characters8(characters)

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:20:
StringView(const uint16_t* characters, unsigned length) : m_is8Bit(false),
m_length(length) { m_characters16 = characters; }
ditto

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:24: const
uint8_t* characters8() const { return m_characters8; }
DCHECK(m_is8Bit)

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:25: const
uint16_t* characters16() const { return m_characters16; }
DCHECK(!m_is8Bit)

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:36: class
PLATFORM_EXPORT StringBuffer {
can we put these in separate headers? StringView.h and StringBuffer.h ?

[dgozman, 2016-08-24 00:45:01]

Elliott, please take another look.

> Maybe I just missed it, but I don't see where you did:
> 
> "make String16 an alias to std::basic_string<uint16_t> to leverage all
existing
> optimizations."
>
> you're still using String16 inside core/ though, and I'd rather not have
> std::basic_string stuff floating around if possible :)

This will happen in next patch, draft here:
https://codereview.chromium.org/2251343003/. I will make sure we have final
shape before committing :)

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:377: static
const char animationObjectGroup[] = "animation";
On 2016/08/23 17:35:38, esprehn wrote:
> constants are kFoo named, kAnimationObjectGroup

Ops. Done.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1170: if
(!m_v8Session->unwrapObject(errorString, toV8InspectorStringView(objectId),
&value, &context, &objectGroup))
On 2016/08/22 23:46:10, caseq wrote:
> Can we make objectGroup parameter in unwrapObject accept nullptr? Most usages
> don't seem to care and pass a dummy.

Done.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:440:
std::unique_ptr<protocol::DOMDebugger::EventListener>
InspectorDOMDebuggerAgent::buildObjectForEventListener(v8::Local<v8::Context>
context, const V8EventListenerInfo& info, const String& objectGroupId)
On 2016/08/22 23:46:10, caseq wrote:
> Actually, it looks like we never use the fact that object group id is a string
> -- we just pass it while converting back and forth. Should we just encapsulate
> it in some opaque object?

Nice catch! Changed to passing StringBuffer around.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:658: if
(url.contains(String(breakpoint.first))) {
On 2016/08/23 17:35:39, esprehn wrote:
> This is allocating a string in the loop, contains actually takes a
> WTF::StringView, so you want to convert to that and avoid the string
> allocations.

breakpoint.first will become WTF::String with the next patch, so I'd rather put
a TODO here to remove extra noop String(...) call.
Right now String(String16) just takes out inner m_impl, so there are no extra
allocations here.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:672:
m_v8Session->breakProgram(toV8InspectorStringView(protocol::Debugger::API::Paused::ReasonEnum::XHR),
toV8InspectorStringView(json));
On 2016/08/23 17:35:39, esprehn wrote:
> protocol::Debugger::API::Paused::ReasonEnum::XHR is crazy named, you might
want
> do something about the code generator :P

Yeah... We can in theory shorten this to
v8_inspector::Debugger::PausedReason::XHR, I'll play with naming in generator.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/V8InspectorString.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/V8InspectorString.cpp:33: return
String();
On 2016/08/23 17:35:39, esprehn wrote:
> ditto, you don't need to null check or empty check

Done.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp:174: return
v8::String::Empty(isolate);
On 2016/08/23 17:35:39, esprehn wrote:
> this turns null strings into Empty(), maybe that's what you want?

Yes, empty v8 string is what we need here.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.h:38: class
StringBufferImpl : public StringBuffer {
On 2016/08/23 17:35:39, esprehn wrote:
> this is very weird to have the String.h define an interface and the
StringUtil.h
> be an impl, I guess it's not part of public/ ?

Yep. StringBufferImpl wants to hide m_owner from public so it has a non-public
implementation. Most non-public files include StringUtil.

It could make sense to split StringBufferImpl out of StringUtil if the
functionality grows, but right now one private header with all string-related
stuff looks ok.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/public/String.h (right):

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:19:
StringView(const uint8_t* characters, unsigned length) : m_is8Bit(true),
m_length(length) { m_characters8 = characters; }
On 2016/08/23 17:35:39, esprehn wrote:
> you can just initialize an unnamed union member directly, 
> 
> m_characters8(characters)

Done.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:24: const
uint8_t* characters8() const { return m_characters8; }
On 2016/08/23 17:35:39, esprehn wrote:
> DCHECK(m_is8Bit)

Unfortunately, I cannot do that right now, since it requires wtf/Assertions.h
include in this public header which is unacceptable. I think we can do it after
moving to v8 though.

https://codereview.chromium.org/2260233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/String.h:36: class
PLATFORM_EXPORT StringBuffer {
On 2016/08/23 17:35:39, esprehn wrote:
> can we put these in separate headers? StringView.h and StringBuffer.h ?

Sure.

[dgozman, 2016-08-24 18:58:38]

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

[commit-bot: I haz the power, 2016-08-24 18:59: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, 2016-08-24 19:39:05]

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

[commit-bot: I haz the power, 2016-08-24 19:39:06]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dgozman, 2016-08-25 01:35:30]

@esprehn: could you please take another look?

[dgozman, 2016-08-25 18:22:38]

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

[commit-bot: I haz the power, 2016-08-25 18:23:15]

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, 2016-08-25 20:14:14]

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

[commit-bot: I haz the power, 2016-08-25 20:14:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[esprehn, 2016-08-26 04:55:49]

lgtm

https://codereview.chromium.org/2260233002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/public/StringView.h
(right):

https://codereview.chromium.org/2260233002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/StringView.h:18:
StringView() : m_is8Bit(true), m_length(0), m_characters8(nullptr) { }
We would normally wrap this to one var per line, see wtf/text/StringView.h

[dgozman, 2016-08-26 18:50:52]

Thanks everyone for review!

https://codereview.chromium.org/2260233002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/public/StringView.h
(right):

https://codereview.chromium.org/2260233002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/StringView.h:18:
StringView() : m_is8Bit(true), m_length(0), m_characters8(nullptr) { }
On 2016/08/26 04:55:48, esprehn wrote:
> We would normally wrap this to one var per line, see wtf/text/StringView.h

Done.

[dgozman, 2016-08-26 21:00:39]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-08-26 21:00:40]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2260233002/#ps200001
(title: "styling")

[commit-bot: I haz the power, 2016-08-26 21:01:33]

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, 2016-08-26 22:55:28]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound helps to
avoid copies in important places (mostly when passing large protocol messages
around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound helps to
avoid copies in important places (mostly when passing large protocol messages
around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

[commit-bot: I haz the power, 2016-08-26 22:55:30]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-08-26 22:58:55]

Description was changed from

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound helps to
avoid copies in important places (mostly when passing large protocol messages
around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing

optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032
==========

to

==========
[DevTools] Migrate v8_inspector/public from String16 to String{View,Buffer}.

Using StringView for inbound parameters and StringBuffer for outbound helps to
avoid copies in important places (mostly when passing large protocol messages
around).

This opens possibility to:
- use WTF::String in core/inspector instead of String16;
- make String16 an alias to std::basic_string<uint16_t> to leverage all existing
optimizations.

One possible optimization is to migrate search utility (in V8StringUtil) to
StringView and avoid any copies of content there.

BUG=637032

Committed: https://crrev.com/101f402f95f5a67df948347edc4dce050ce482c7
Cr-Commit-Position: refs/heads/master@{#414846}
==========

[commit-bot: I haz the power, 2016-08-26 22:58:57]

Patchset 10 (id:??) landed as
https://crrev.com/101f402f95f5a67df948347edc4dce050ce482c7
Cr-Commit-Position: refs/heads/master@{#414846}