[webkit-reviews] review denied: [Bug 16829] NPAPI: NPN_SetException() sets exception on the Global ExecState instead of local : [Attachment 26726] NPN_SetException() implementation.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 16829: NPAPI: NPN_SetException() sets exception on the Global ExecState
instead of local
https://bugs.webkit.org/show_bug.cgi?id=16829

Attachment 26726: NPN_SetException() implementation.
https://bugs.webkit.org/attachment.cgi?id=26726&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I like the approach.  However, there are a few things wrong here.

1.  s_exceptionEnvironment seems never to be used, and can be removed.

2.  There are a few code style violations (spacing around *, and { on the next
line after a function declaration).
http://webkit.org/coding/coding-style.html

3.  I've never seen getExceptionString() style functions return a mutable
reference.  I've seen setExecptionString(string) used, and execptionString()
style which could return a global poitner.  renaming to sharedExceptionString()
or globalExceptionString() might be sufficient to feel "webkitty".

Parts of the NPAPI code are a sewer (from long neglect).  But we still should
try and update the parts we touch to modern style.  An example of a part which
could use update:
+static bool testHasMethod(NPObject *obj, NPIdentifier name);
+static bool testInvoke(NPObject *obj, NPIdentifier name, const NPVariant
*args, uint32_t argCount, NPVariant *result);

WK Style suggests not using named arguments in declarations, unless the name
helps undestand what the argument is used for.	For example, "name" is probably
helps undestanding, but "obj" does not, and can be removed.

r- for s_exceptionEnvironment and the various style violations.