[webkit-reviews] review denied: [Bug 58280] SerializedScriptValue shouldn't always throw exceptions : [Attachment 89113] patch

[bugzilla-daemon at webkit.org]

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).