[DevTools] Generate separate copies of inspector_protocol.

[DevTools] Generate separate copies of inspector_protocol.

Generated protocol in core/inspector uses WTF::String directly.
Protocol and classes in platform/v8_inspector use v8_inspector::String16.

BUG=637032
Committed: https://crrev.com/0ca1f564f9660554511191fbd38bec101ca6f08d
Cr-Commit-Position: refs/heads/master@{#414956}

[dgozman, 2016-08-19 00:53:29]

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

[dgozman, 2016-08-19 00:53:30]

Could you please take a look?

I plan to follow up with:
- v8_inspector/public/StringView used in public;
- InspectorProtocolForward.h;
- removing ProtocolPlatformWTF;
- removing Allocator_h.template.

[dgozman, 2016-08-19 00:53:37]

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

[commit-bot: I haz the power, 2016-08-19 00:54:26]

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-19 00:59:20]

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

[commit-bot: I haz the power, 2016-08-19 00:59:21]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[caseq, 2016-08-19 02:50:36]

would it be possible to split string migration from namespace split?

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp (right):

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp:107: const String&
url = toCoreString(stackTrace->topSourceURL());
this doesn't look good :-)

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:683:
m_v8Session->breakProgram(v8_inspector::protocol::Debugger::API::Paused::ReasonEnum::XHR,
toV8InspectorString(eventData->toJSONString()));
It's seventh time we're invoking breakProgram here and we're becoming
exceedingly efficient at that. Let's extract a helper method for that, in case
we're going to touch this again when/if moving to use string views?

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:357:
m_client->configureOverlay(m_state->booleanProperty(PageAgentState::overlaySuspended,
false), overlayMessage.isEmpty() ? String() : overlayMessage);
remove ternary operator here.

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:527:
std::unique_ptr<protocol::Array<v8_inspector::protocol::Debugger::API::SearchMatch>>
results =
protocol::Array<v8_inspector::protocol::Debugger::API::SearchMatch>::create();
auto results = ....::create()?

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorSession.cpp (right):

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorSession.cpp:106:
sendProtocolResponse(callId, toCoreString(message));
Ouch. So it's an extra copy for everything that is dispatched from v8? Can we
avoid that?

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right):

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:333:
TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console",
toCoreString(title).utf8().data(), this);
This and the two below are kinda time-sensitive, it would be cool to get hold of
utf8() without an extra copy.

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/v8_inspector/String16.cpp (right):

https://codereview.chromium.org/2251343003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/String16.cpp:410: const size_t
kBufferSize = 100;
100 looks like overkill :-)

[dgozman, 2016-08-24 19:19:06]

@caseq: I believe this is ready for review.

[pfeldman, 2016-08-25 00:30:08]

looked at a number of random files, move lgtm.

[alph, 2016-08-25 18:19:27]

alph@chromium.org changed reviewers:
+ alph@chromium.org

[alph, 2016-08-25 18:19:29]

Looked at a random file. Got some nits.
Also gyp is no longer supported. You don't need to update gyp files. They are
going to be deleted soon.

lgtm

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

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:44: if
(equalSignPos < length && content[equalSignPos] != '=')
equalSignPos >= length || ...

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:50: }
look for the '\n' in the else part?

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:62: size_t
newLine = match.find("\n");
Why do you look for a new line in the multiline case?

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:87:
result.append(text[i]);
nit: c

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:107:
result->push_back(text.length());
static_cast

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:124: if
(line.length() && line[line.length() - 1] == '\r')
curious why you trimming \r but keeping \n

[caseq, 2016-08-26 19:17:21]

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:346:
m_client->configureOverlay(m_state->booleanProperty(PageAgentState::overlaySuspended,
false), overlayMessage.isEmpty() ? String() : overlayMessage);
remove ternary operator, just use overlayMessage directly.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/ProtocolPlatform.h (right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ProtocolPlatform.h:13: #endif //
!defined(ProtocolPlatform_h)
nit: nuke !defined(), this style is mostly unused outside of inspector.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/String16.cpp (right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/String16.cpp:470: void
String16Builder::append(const char* characters, size_t length)
just copy-paste the one-line implementation above and it will work as expected.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerScript.cpp
(right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerScript.cpp:35: const
size_t hashesSize = V8_INSPECTOR_ARRAY_LENGTH(hashes);
nit: I'd get rid of these in favor of std::end(hashes) - std::begin(hashes) now.

[caseq, 2016-08-26 19:17:30]

lgtm

[dgozman, 2016-08-26 21:31:27]

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

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:62: size_t
newLine = match.find("\n");
On 2016/08/25 18:19:28, alph wrote:
> Why do you look for a new line in the multiline case?

These questions are for Aleksey, I'll ask. I've just moved the code.

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:87:
result.append(text[i]);
On 2016/08/25 18:19:29, alph wrote:
> nit: c

Done.

https://codereview.chromium.org/2251343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/SearchUtil.cpp:107:
result->push_back(text.length());
On 2016/08/25 18:19:28, alph wrote:
> static_cast

Done.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:346:
m_client->configureOverlay(m_state->booleanProperty(PageAgentState::overlaySuspended,
false), overlayMessage.isEmpty() ? String() : overlayMessage);
On 2016/08/26 19:17:20, caseq wrote:
> remove ternary operator, just use overlayMessage directly.

Empty and null is not the same thing, but it should work here, as we don't
override value if the key was missing.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/ProtocolPlatform.h (right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ProtocolPlatform.h:13: #endif //
!defined(ProtocolPlatform_h)
On 2016/08/26 19:17:20, caseq wrote:
> nit: nuke !defined(), this style is mostly unused outside of inspector.

Done.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/String16.cpp (right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/String16.cpp:470: void
String16Builder::append(const char* characters, size_t length)
On 2016/08/26 19:17:20, caseq wrote:
> just copy-paste the one-line implementation above and it will work as
expected.

Done.

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerScript.cpp
(right):

https://codereview.chromium.org/2251343003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerScript.cpp:35: const
size_t hashesSize = V8_INSPECTOR_ARRAY_LENGTH(hashes);
On 2016/08/26 19:17:20, caseq wrote:
> nit: I'd get rid of these in favor of std::end(hashes) - std::begin(hashes)
now.

We can do that in a follow up.

[dgozman, 2016-08-27 00:15:41]

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

[commit-bot: I haz the power, 2016-08-27 00:16:10]

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-27 00:32:44]

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

[commit-bot: I haz the power, 2016-08-27 00:32:45]

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

[dgozman, 2016-08-27 01:49:33]

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

[commit-bot: I haz the power, 2016-08-27 01:49:55]

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-27 02:03:49]

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

[commit-bot: I haz the power, 2016-08-27 02:03:51]

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

[dgozman, 2016-08-27 23:01:16]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-08-27 23:01:17]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org,
pfeldman@chromium.org, caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2251343003/#ps120001
(title: "win compile")

[commit-bot: I haz the power, 2016-08-27 23:01:27]

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-28 01:37:58]

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

[commit-bot: I haz the power, 2016-08-28 01:38:00]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2016-08-28 04:43:18]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-08-28 04:43:25]

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-28 05:56:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-28 05:58:06]

Description was changed from

==========
[DevTools] Generate separate copies of inspector_protocol.

Generated protocol in core/inspector uses WTF::String directly.
Protocol and classes in platform/v8_inspector use v8_inspector::String16.

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

to

==========
[DevTools] Generate separate copies of inspector_protocol.

Generated protocol in core/inspector uses WTF::String directly.
Protocol and classes in platform/v8_inspector use v8_inspector::String16.

BUG=637032

Committed: https://crrev.com/0ca1f564f9660554511191fbd38bec101ca6f08d
Cr-Commit-Position: refs/heads/master@{#414956}
==========

[commit-bot: I haz the power, 2016-08-28 05:58:08]

Patchset 7 (id:??) landed as
https://crrev.com/0ca1f564f9660554511191fbd38bec101ca6f08d
Cr-Commit-Position: refs/heads/master@{#414956}