Include the interface name when an uncloneable DOM wrapper is encountered.

Include the interface name when an uncloneable DOM wrapper is encountered.

This has been useful in my debugging, and could be useful to authors as well.

BUG=148757
Committed: https://crrev.com/6adb56143dcc5891898054d0935310f23caa0804
Cr-Commit-Position: refs/heads/master@{#419948}

[jbroman, 2016-09-20 23:17:50]

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

[commit-bot: I haz the power, 2016-09-20 23:18:38]

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

[jbroman, 2016-09-20 23:19:26]

jbroman@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[jbroman, 2016-09-20 23:19:27]

It's easy to be more specific here, so why not?

[haraken, 2016-09-20 23:48:50]

LGTM

[jbroman, 2016-09-20 23:48:54]

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

https://codereview.chromium.org/2354873006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:133:
String interface = wrappable->wrapperTypeInfo()->interfaceName;
Before esprehn points it out, this does one unnecessary copy into a StringImpl
before then concatenating into another StringImpl. There isn't a very convenient
way to efficiently concatenate two const char*, unless we want to call it
explicitly, like:

WTF::StringAppend(wrappable->wrapperTypeInfo()->interfaceName, " could not be
cloned.");

But this isn't a big deal because we concatenate it to more things later on, and
there is much more expensive work in the exception throwing path anyhow.

We could (and perhaps should) at least add StringConcatenate overloads for
StringView. I've sketched it locally; I may upload that as a separate patch.

[haraken, 2016-09-20 23:50:08]

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

https://codereview.chromium.org/2354873006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:133:
String interface = wrappable->wrapperTypeInfo()->interfaceName;
On 2016/09/20 23:48:54, jbroman wrote:
> Before esprehn points it out, this does one unnecessary copy into a StringImpl
> before then concatenating into another StringImpl. There isn't a very
convenient
> way to efficiently concatenate two const char*, unless we want to call it
> explicitly, like:
> 
> WTF::StringAppend(wrappable->wrapperTypeInfo()->interfaceName, " could not be
> cloned.");
> 
> But this isn't a big deal because we concatenate it to more things later on,
and
> there is much more expensive work in the exception throwing path anyhow.
> 
> We could (and perhaps should) at least add StringConcatenate overloads for
> StringView. I've sketched it locally; I may upload that as a separate patch.

Can we use StringBuilder?

[jbroman, 2016-09-20 23:53:14]

On 2016/09/20 at 23:50:08, haraken wrote:
>
https://codereview.chromium.org/2354873006/diff/1/third_party/WebKit/Source/b...
> File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):
> 
>
https://codereview.chromium.org/2354873006/diff/1/third_party/WebKit/Source/b...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:133:
String interface = wrappable->wrapperTypeInfo()->interfaceName;
> On 2016/09/20 23:48:54, jbroman wrote:
> > Before esprehn points it out, this does one unnecessary copy into a
StringImpl
> > before then concatenating into another StringImpl. There isn't a very
convenient
> > way to efficiently concatenate two const char*, unless we want to call it
> > explicitly, like:
> > 
> > WTF::StringAppend(wrappable->wrapperTypeInfo()->interfaceName, " could not
be
> > cloned.");
> > 
> > But this isn't a big deal because we concatenate it to more things later on,
and
> > there is much more expensive work in the exception throwing path anyhow.
> > 
> > We could (and perhaps should) at least add StringConcatenate overloads for
> > StringView. I've sketched it locally; I may upload that as a separate patch.
> 
> Can we use StringBuilder?

StringBuilder also does an extra copy.

[jbroman, 2016-09-20 23:53:43]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-09-20 23:53:44]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:54:04]

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

[esprehn, 2016-09-21 00:07:20]

Yeah I was trying to get to StringView for StringConcatenate, I started towards
it here:
https://codereview.chromium.org/2315853002

but yutak@ wants me to break that up (which is legit) and I got busy this week.
If you want to land that in parts, or just deal with StringView that seems
great! We also need to fix StringUTF8Adaptor as you found out. :)

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

https://codereview.chromium.org/2354873006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:134:
exceptionState.throwDOMException(DataCloneError, interface + " object could not
be cloned.");
String() + interface + " object could not be cloned."

avoids the copies. :)

[commit-bot: I haz the power, 2016-09-21 02:10:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-21 02:11:53]

Description was changed from

==========
Include the interface name when an uncloneable DOM wrapper is encountered.

This has been useful in my debugging, and could be useful to authors as well.

BUG=148757
==========

to

==========
Include the interface name when an uncloneable DOM wrapper is encountered.

This has been useful in my debugging, and could be useful to authors as well.

BUG=148757

Committed: https://crrev.com/6adb56143dcc5891898054d0935310f23caa0804
Cr-Commit-Position: refs/heads/master@{#419948}
==========

[commit-bot: I haz the power, 2016-09-21 02:11:54]

Patchset 1 (id:??) landed as
https://crrev.com/6adb56143dcc5891898054d0935310f23caa0804
Cr-Commit-Position: refs/heads/master@{#419948}