Summary: | [V8] Remove return values of macros in V8BindingMacros.h | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||
Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||
Status: | RESOLVED WONTFIX | ||||||
Severity: | Normal | CC: | abarth, andersca, dglazkov, japhet, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Kentaro Hara
2012-11-21 20:14:10 PST
Created attachment 175574 [details]
Patch
Comment on attachment 175574 [details] Patch Attachment 175574 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14950331 New failing tests: inspector/console/alert-toString-exception.html fast/workers/worker-constructor.html fast/workers/storage/execute-sql-args-sync.html fast/workers/storage/open-database-inputs-sync.html fast/workers/storage/execute-sql-args-worker.html storage/websql/execute-sql-args.html Comment on attachment 175574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175574&action=review > Source/WebCore/bindings/v8/V8BindingMacros.h:-59 > - return v8::Undefined(); Oh, removing 'v8::Undefined()' is fine but removing 'return' is not fine here. Either way it looks like I need to think about a whole plan of EXCEPTION_BLOCK()s refactoring before making ad-hoc improvements. My goal would be implementing the following methods: double v8ValueToNumber(Handle<Value> value) { if (value->IsNumber()) return value->NumberValue(); v8::TryCatch block; double d = value->NumberValue(); if (block.HasCaught()) block.ReThrow(); return d; } double v8ValueToInt32(Handle<Value> value) { ...; } > My goal would be implementing the following methods: > > double v8ValueToNumber(Handle<Value> value) { > if (value->IsNumber()) > return value->NumberValue(); > v8::TryCatch block; > double d = value->NumberValue(); > if (block.HasCaught()) > block.ReThrow(); What's the point of catching the exception just to re-throw it? > return d; How does the caller know that there was an exception? If there was an exception, the caller should return early (e.g., and not process further arguments). Is there a reason to have a TryCatch block per argument? I wonder if we can have just one for all the arguments. We could declare it up front and each argument would do something like v8::TryCatch block; String foo = v8ValueToString(args[0]); if (block.HasCaught()) { block.ReThrow(); return v8::Undefined(args.GetIsolate()); } double foo = v8ValueToNumber(args[1]); if (block.HasCaught()) { block.ReThrow(); return v8::Undefined(args.GetIsolate()); } We might need to get more fancy and create the block lazily (to avoid the overhead of creating one---we should perf test before doing this): OwnPtr<v8::TryCatch> block; String foo = v8ValueToString(args[0], block); if (block && block->HasCaught()) { block->ReThrow(); return v8::Undefined(args.GetIsolate(), block); } double foo = v8ValueToNumber(args[1]); if (block && block->HasCaught()) { block->ReThrow(); return v8::Undefined(args.GetIsolate()); } Even better might be a way to ask the isolate or the context (whichever is appropriate) if there is a pending exception without needing to create a TryCatch (given that we're just going to re-throw the exception anyway). > double foo = v8ValueToNumber(args[1]);
This should have been
double foo = v8ValueToNumber(args[1], block);
in the last example.
V8 is gone from WebKit. |