Issue 30673002: More informative error messages for non-Transferables.

Issue 30673002: More informative error messages for non-Transferables. (Closed)

Created:
7 years, 2 months ago by sof

Modified:
7 years, 1 month ago

Reviewers:
Mike West, Inactive

CC:
blink-reviews, jochen (gone - plz use gerrit), marja

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

More informative error messages for non-Transferables. Elaborate the TypeError exception reported if the various postMessage()s (on window, at the dedicated Worker global scope, or over MessagePorts) are ill-typed transferable arguments, supplying an error message indicating that the transferable argument isn't a sequence type. R= BUG=308730 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160323

Patch Set 1 #

Total comments: 5

Patch Set 2 : More informative error messages for non-Transferables. #

Total comments: 15

Patch Set 3 : More informative error messages for non-Transferables. #

Created: 7 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+86 lines, -33 lines)			Patch
M	LayoutTests/fast/dom/Window/window-postmessage-args.html	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	LayoutTests/fast/dom/Window/window-postmessage-args-expected.txt	View	1 2	1 chunk	+4 lines, -3 lines	0 comments	Download
M	LayoutTests/fast/events/message-port-multi-expected.txt	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/resources/message-port-multi.js	View	1 2	2 chunks	+6 lines, -6 lines	0 comments	Download
M	LayoutTests/fast/workers/worker-context-multi-port-expected.txt	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/workers/worker-multi-port-expected.txt	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
A +	LayoutTests/fast/workers/worker-onerror-09.html	View	1 2	1 chunk	+5 lines, -6 lines	0 comments	Download
A	LayoutTests/fast/workers/worker-onerror-09-expected.txt	View	1 2	1 chunk	+16 lines, -0 lines	0 comments	Download
M	Source/bindings/v8/ExceptionMessages.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/bindings/v8/ExceptionMessages.cpp	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
M	Source/bindings/v8/V8Binding.h	View	1 2	6 chunks	+14 lines, -6 lines	0 comments	Download
M	Source/bindings/v8/V8Utilities.h	View	1	1 chunk	+4 lines, -2 lines	0 comments	Download
M	Source/bindings/v8/V8Utilities.cpp	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/v8/custom/V8DedicatedWorkerGlobalScopeCustom.cpp	View	1 2	2 chunks	+6 lines, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8MessagePortCustom.cpp	View	1 2	2 chunks	+6 lines, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8WindowCustom.cpp	View	1 2	2 chunks	+6 lines, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8WorkerCustom.cpp	View	1 2	2 chunks	+6 lines, -1 line	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

Mike West

Thanks, I like this patch! A few comments, really just bikeshedding about the error messages. ...

7 years, 2 months ago (2013-10-21 06:51:21 UTC) #2

sof

On 2013/10/21 06:51:21, Mike West wrote: > Thanks, I like this patch! A few comments, ...

7 years, 2 months ago (2013-10-21 22:47:07 UTC) #4

On 2013/10/21 06:51:21, Mike West wrote:
> Thanks, I like this patch! A few comments, really just bikeshedding about the
> error messages.
> 

Quality review and comments, many thanks for the feedback & pointing me
in a better direction re: error message handling.

> https://codereview.chromium.org/30673002/diff/1/Source/bindings/v8/V8Binding.h
> File Source/bindings/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/30673002/diff/1/Source/bindings/v8/V8Binding....
> Source/bindings/v8/V8Binding.h:78: v8::Handle<v8::Value>
> toV8Sequence(v8::Handle<v8::Value>, uint32_t& length, v8::Isolate*, const
> String& errorMessage);
> Let's keep the isolate as the last parameter.
>

Makes good sense to keep it so. However, the 2nd patch set ended up with an
optional
argument here instead. See if you agree with that way of working with multiple
toV8Sequence() callers.
 
>
https://codereview.chromium.org/30673002/diff/1/Source/bindings/v8/custom/V8D...
> File Source/bindings/v8/custom/V8DedicatedWorkerGlobalScopeCustom.cpp (right):
> 
>
https://codereview.chromium.org/30673002/diff/1/Source/bindings/v8/custom/V8D...
> Source/bindings/v8/custom/V8DedicatedWorkerGlobalScopeCustom.cpp:50:
> args.GetIsolate(), "Second argument is not a sequence type"))
> Two things:
> 
> 1. Can you bring in ExceptionMessages here? Something like
> "ExceptionMessages::failedToExecute("postMessage", "WorkerGlobalScope", ...)"
> would generate a friendlier error message.
> 
> 2. I'd prefer to use something more clear than "sequence type". Developers
don't
> read IDL (and, honestly, I had to go find the IDL spec to understand what this
> meant. :) ). I'd suggest something like "The second argument ('transfer') is
> neither an array, nor does it support indexed properties."
> 

Done both, but avoided the up-front cost of constructing this exception(al)
error
message string. Instead signalling the type error and only constructing it on
such failed conversion of the transferable argument.

Mike West

Thanks for going another round. Just a few nits at this point, LGTM once you ...

7 years, 2 months ago (2013-10-22 06:50:39 UTC) #5

sof

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/ExceptionMessages.cpp File Source/bindings/v8/ExceptionMessages.cpp (right): https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/ExceptionMessages.cpp#newcode63 Source/bindings/v8/ExceptionMessages.cpp:63: return argument + " argument is not an array ...

7 years, 1 month ago (2013-10-22 16:06:56 UTC) #6

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/Excep...
File Source/bindings/v8/ExceptionMessages.cpp (right):

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/Excep...
Source/bindings/v8/ExceptionMessages.cpp:63: return argument + " argument is not
an array type, nor has indexed properties.";
On 2013/10/22 06:50:40, Mike West wrote:
> "not ... nor" sounds strange. I'd still prefer "... argument is neither an
array
> type, nor does it have indexed properties."

I changed this to "is neither an array, nor does it .."

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:78: v8::Handle<v8::Value>
toV8Sequence(v8::Handle<v8::Value>, uint32_t& length, v8::Isolate*, bool*
notASequence = 0);
On 2013/10/22 06:50:40, Mike West wrote:
> I think I'd prefer to keep the Isolate as the last argument by creating
another
> version of this function. I don't know of anywhere else in the bindings code
> where the Isolate isn't at the end, and I'd like to maintain that unless
there's
> a good reason not to.
> 
> That is, I'd prefer to see:
> 
>  v8::Handle<v8::Value> toV8Sequence(v8::Handle<v8::Value>, uint32_t& length,
> v8::Isolate*);
>  v8::Handle<v8::Value> toV8Sequence(v8::Handle<v8::Value>, uint32_t& length,
> bool* notASequence, v8::Isolate*);
> 
> where the first just wraps the second with a temporary bool. If that overhead
> causes perf regressions, that'd be a good reason to break convention, but I
> don't think it will.

Done, and dropped the "nicety" of having the extra argument be optional.
However, the argument ordering convention of having 'out' arguments appear last
isn't followed, but I assume that doesn't trump Isolate consistency in
this particular case.

(The previous version was patterned on the ordering that toRefPtrNativeArray()
uses.)

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:525: throwTypeError(isolate);
On 2013/10/22 06:50:40, Mike West wrote:
> I'd be happy to review a subsequent patch that added a nicer error message
here,
> and on line 542. :)

Definitely, I can have a look at improving that.

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8DedicatedWorkerGlobalScopeCustom.cpp (right):

https://codereview.chromium.org/30673002/diff/110001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8DedicatedWorkerGlobalScopeCustom.cpp:52: args[1],
ports, arrayBuffers, notASequence, args.GetIsolate())) {
On 2013/10/22 06:50:40, Mike West wrote:
> Nit: The line breaks here are strange. Generally speaking, Blink does not have
a
> line-length limit, so you can just leave this on one line.

Sorry, presubmit checks leading me to assume wrongly about reqd formatting. Top
tip :)

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/30673002/230001

7 years, 1 month ago (2013-10-23 09:01:33 UTC) #8

sof

On 2013/10/23 08:06:10, Mike West wrote: > LGTM. Assuming the bots are happy, please land ...

7 years, 1 month ago (2013-10-23 09:03:39 UTC) #9

Message was sent while issue was closed.

Change committed as 160323

Expand Messages | Collapse Messages