[webkit-reviews] review denied: [Bug 58280] SerializedScriptValue shouldn't always throw exceptions : [Attachment 89113] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 11 16:16:15 PDT 2011
Oliver Hunt <oliver at apple.com> has denied Stephanie Lewis <slewis at apple.com>'s
request for review:
Bug 58280: SerializedScriptValue shouldn't always throw exceptions
https://bugs.webkit.org/show_bug.cgi?id=58280
Attachment 89113: patch
https://bugs.webkit.org/attachment.cgi?id=89113&action=review
------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=89113&action=review
Rather than a bool i'd prefer
enum SerializationErrorMode { Throwing, NonThrowing }
as that makes things more explicit.
The use of Global will also fail on ToT as geoff has renamed everything, i
believe you'll want heap/Strong.h and Strong<..>
r-ing due to the style errors, and it failing to build on tot :-(
> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1435
> + switch (code) {
> + case SuccessfullyCompleted:
> + break;
> + case StackOverflowError:
> + throwError(exec, createStackOverflowError(exec));
> + break;
> + case InterruptedExecutionError:
> + throwError(exec,
createInterruptedExecutionException(&exec->globalData()));
> + break;
> + case ValidationError:
> + throwError(exec, createTypeError(exec, "Unable to deserialize data."));
> + break;
> + case ExistingExceptionError:
> + case UnspecifiedError:
> + break;
> + }
I don't like this code, i'd prefer:
if (code == SuccessfullyCompleted)
return;
switch (code) {
case ...:
throwError(exec, ...)
return;
default:
return;
Also UnspecifiedError and ExistingExceptionError should both create a generic
exception message (as we apparently don't know what has gone wrong).
More information about the webkit-reviews
mailing list