Bug 85078 - Make DOM Exceptions Errors
Summary: Make DOM Exceptions Errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL: http://www.w3.org/TR/WebIDL/#es-excep...
Keywords:
Depends on:
Blocks: 85057 85100
  Show dependency treegraph
 
Reported: 2012-04-27 10:23 PDT by Erik Arvidsson
Modified: 2012-05-31 15:02 PDT (History)
17 users (show)

See Also:


Attachments
Patch (49.44 KB, patch)
2012-05-03 14:47 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (56.58 KB, patch)
2012-05-04 14:57 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.11 MB, application/zip)
2012-05-04 16:05 PDT, WebKit Review Bot
no flags Details
Patch (62.10 KB, patch)
2012-05-10 10:15 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (58.51 KB, patch)
2012-05-10 11:53 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (59.00 KB, patch)
2012-05-11 15:30 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (58.94 KB, patch)
2012-05-11 15:51 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (54.63 KB, patch)
2012-05-14 13:56 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (48.54 KB, patch)
2012-05-18 12:28 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (569.15 KB, application/zip)
2012-05-18 13:32 PDT, WebKit Review Bot
no flags Details
Patch (49.14 KB, patch)
2012-05-24 16:03 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (526.09 KB, application/zip)
2012-05-24 17:57 PDT, WebKit Review Bot
no flags Details
Patch (48.11 KB, patch)
2012-05-29 12:51 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (47.51 KB, patch)
2012-05-30 12:33 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (47.96 KB, patch)
2012-05-30 17:17 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-04-27 10:23:23 PDT
There is currently work undergoing to add "stack" to DOM exceptions. Since stack is available on Error object (part of its "interface" in both JSC and V8) it would make sense to make DOM exceptions Error objects (or at least have Error.prototype on its prototype chain).

http://lists.w3.org/Archives/Public/public-script-coord/2012AprJun/0136.html
Comment 1 Erik Arvidsson 2012-04-27 10:30:59 PDT
Actually, the spec requires this. I missed that the first time.

http://www.w3.org/TR/WebIDL/#es-exception-interface-prototype-object

"Otherwise, the exception is not declared to inherit from another exception. The value of the internal [[Prototype]] property is the Error prototype object ([ECMA-262], section 15.11.3.1)."
Comment 2 Kentaro Hara 2012-04-27 10:31:58 PDT
(In reply to comment #1)
> Actually, the spec requires this. I missed that the first time.
> 
> http://www.w3.org/TR/WebIDL/#es-exception-interface-prototype-object
> 
> "Otherwise, the exception is not declared to inherit from another exception. The value of the internal [[Prototype]] property is the Error prototype object ([ECMA-262], section 15.11.3.1)."

Sounds great!
Comment 3 Oliver Hunt 2012-04-27 10:54:53 PDT
In JSC it should just magically acquire the stack property.
Comment 4 Erik Arvidsson 2012-05-03 14:47:38 PDT
Created attachment 140097 [details]
Patch

Work in progress
Comment 5 Erik Arvidsson 2012-05-04 14:57:17 PDT
Created attachment 140333 [details]
Patch
Comment 6 WebKit Review Bot 2012-05-04 16:05:51 PDT
Comment on attachment 140333 [details]
Patch

Attachment 140333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12629306

New failing tests:
svg/filters/feBlend-invalid-mode.xhtml
Comment 7 WebKit Review Bot 2012-05-04 16:05:58 PDT
Created attachment 140350 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Erik Arvidsson 2012-05-10 10:15:22 PDT
Created attachment 141192 [details]
Patch
Comment 9 Erik Arvidsson 2012-05-10 10:18:57 PDT
Kentaro/Adam, can you look at the V8 bindings?

Olliver/Gavin, can you look at the JSC bindings?
Comment 10 Adam Barth 2012-05-10 11:07:15 PDT
Comment on attachment 141192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141192&action=review

V8 changes LGTM

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:-1830
> -    if (exec->argumentCount() < 2)
> -        return throwVMError(exec, createNotEnoughArgumentsError(exec));

What's the reason for removing this argumentCount check?
Comment 11 Erik Arvidsson 2012-05-10 11:17:14 PDT
(In reply to comment #10)
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:-1830
> > -    if (exec->argumentCount() < 2)
> > -        return throwVMError(exec, createNotEnoughArgumentsError(exec));
> 
> What's the reason for removing this argumentCount check?

Sigh... This is just a rebaseline of run-binding-tests. Let me fix that.
Comment 12 Erik Arvidsson 2012-05-10 11:53:33 PDT
Created attachment 141215 [details]
Patch
Comment 13 Kentaro Hara 2012-05-10 16:50:26 PDT
Comment on attachment 141215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141215&action=review

V8 change looks good.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:123
> +        if (!prototypeValue.IsEmpty() && prototypeValue->IsObject())

Nit: Maybe "!prototypeValue.IsEmpty() &&" is not necessary.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:127
> +        if (!prototypeValue.IsEmpty() && prototypeValue->IsObject())

Nit: Ditto.
Comment 14 Erik Arvidsson 2012-05-11 15:30:53 PDT
Created attachment 141508 [details]
Patch
Comment 15 Erik Arvidsson 2012-05-11 15:51:12 PDT
Created attachment 141516 [details]
Patch
Comment 16 Erik Arvidsson 2012-05-11 15:54:37 PDT
I expanded the test a bit and incorporated Haraken's feedback.

Oliver/Gavin?
Comment 17 Gavin Barraclough 2012-05-12 15:36:23 PDT
Comment on attachment 141516 [details]
Patch

JSC changes look fine to me, I'm hoping Oliver will chime in since he may know more than me about arrays & the DOM.  But feel free to go ahead.  I'll leave for someone else to  r+, since more of the patch is v8 related.
Comment 18 Oliver Hunt 2012-05-13 11:55:16 PDT
Can we separate out the ArrayClass additions from this patch - i don't see any compelling reason to make both changes in the same patch.
Comment 19 Erik Arvidsson 2012-05-14 13:56:01 PDT
Created attachment 141784 [details]
Patch
Comment 20 Erik Arvidsson 2012-05-14 14:37:16 PDT
(In reply to comment #18)
> Can we separate out the ArrayClass additions from this patch - i don't see any compelling reason to make both changes in the same patch.

Done. PTAL.
Comment 21 Kentaro Hara 2012-05-14 14:54:41 PDT
Comment on attachment 141784 [details]
Patch

r+ for the V8 part
Comment 22 Darin Adler 2012-05-14 16:28:19 PDT
Comment on attachment 141784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141784&action=review

> LayoutTests/platform/chromium/fast/dom/insertAdjacentHTML-DocumentFragment-crash-expected.txt:1
> +CONSOLE MESSAGE: line 9: Uncaught NO_MODIFICATION_ALLOWED_ERR: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7

These are really strange error messages, repeating the name of the error twice. Are you sure this is the format we want?
Comment 23 Erik Arvidsson 2012-05-14 16:45:37 PDT
(In reply to comment #22)
> (From update of attachment 141784 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141784&action=review
> 
> > LayoutTests/platform/chromium/fast/dom/insertAdjacentHTML-DocumentFragment-crash-expected.txt:1
> > +CONSOLE MESSAGE: line 9: Uncaught NO_MODIFICATION_ALLOWED_ERR: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7
> 
> These are really strange error messages, repeating the name of the error twice. Are you sure this is the format we want?

I started investigating this last week but I could not figure it out. The error that is printed in the console is

"Error: HIERARCHY_REQUEST_ERR: DOM Exception 3"

but the error that is printed by DumpRenderTree is

"HIERARCHY_REQUEST_ERR: HIERARCHY_REQUEST_ERR: DOM Exception 3"

I'll post a new patch once/if I figure this out.
Comment 24 Erik Arvidsson 2012-05-16 15:27:43 PDT
This is blocked by http://code.google.com/p/v8/issues/detail?id=2138
Comment 25 Erik Arvidsson 2012-05-18 12:28:17 PDT
Created attachment 142762 [details]
Patch
Comment 26 Erik Arvidsson 2012-05-18 12:30:36 PDT
Please take another look.

Darin: The V8 bug has been fixed so the *-expected.txt do not print the name twice.

Oliver: ArrayClass has been removed from this patch.
Comment 27 WebKit Review Bot 2012-05-18 13:32:26 PDT
Comment on attachment 142762 [details]
Patch

Attachment 142762 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12718963

New failing tests:
fast/dynamic/015.html
fast/dom/timer-clear-interval-in-handler-and-generate-error.html
fast/inspector-support/uncaught-dom8-exception.html
fast/inspector-support/uncaught-dom3-exception.html
fast/inspector-support/uncaught-dom1-exception.html
fast/dom/NamedNodeMap-setNamedItem-crash.html
fast/events/remove-target-with-shadow-in-drag.html
fast/dom/insertAdjacentHTML-DocumentFragment-crash.html
Comment 28 WebKit Review Bot 2012-05-18 13:32:32 PDT
Created attachment 142778 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 29 Erik Arvidsson 2012-05-18 13:54:28 PDT
Strange, I thought the V8 roll had happened. I'll check back next week
Comment 30 Kentaro Hara 2012-05-18 13:57:09 PDT
(In reply to comment #29)
> Strange, I thought the V8 roll had happened. I'll check back next week

Maybe you need to ask the sheriff-bot to roll out DEPS of chromium-ews?
Comment 31 Erik Arvidsson 2012-05-24 16:03:17 PDT
Created attachment 143914 [details]
Patch
Comment 32 Eric Seidel (no email) 2012-05-24 16:50:09 PDT
Comment on attachment 143914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143914&action=review

> Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.h:27
> +#include <runtime/ObjectPrototype.h>

I'm confused why this is needed.

> Source/WebCore/bindings/scripts/test/JS/JSTestNode.h:28
> +#include <runtime/ObjectPrototype.h>

Why is this needed?  Is this auto-generated code?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> +#define V8_STORE_PRIMORDIAL(name, Name) \

Macro sadness. :(

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:-39
> -    V(objectPrototype) \

?
Comment 33 Erik Arvidsson 2012-05-24 17:20:44 PDT
Comment on attachment 143914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143914&action=review

>> Source/WebCore/bindings/scripts/test/JS/JSTestNode.h:28
>> +#include <runtime/ObjectPrototype.h>
> 
> Why is this needed?  Is this auto-generated code?

It is auto generated but I can change the generated code to not add this here

>> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
>> +#define V8_STORE_PRIMORDIAL(name, Name) \
> 
> Macro sadness. :(

V8 API is pretty verbose. Without a macro this code has to be duplicated (and one more time for ArrayClass if we ever dare try that again).

>> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:-39
>> -    V(objectPrototype) \
> 
> ?

Not used. I guess someone forgot to remove it earlier (probably me).
Comment 34 WebKit Review Bot 2012-05-24 17:57:11 PDT
Comment on attachment 143914 [details]
Patch

Attachment 143914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12793327

New failing tests:
svg/filters/feBlend-invalid-mode.xhtml
Comment 35 WebKit Review Bot 2012-05-24 17:57:17 PDT
Created attachment 143938 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 36 Erik Arvidsson 2012-05-29 12:51:25 PDT
Created attachment 144602 [details]
Patch
Comment 37 Eric Seidel (no email) 2012-05-29 13:30:24 PDT
Comment on attachment 144602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2441
> +    my $WrapperTypePrototype = $dataNode->isException ? "WrapperTypeErrorPrototype" : "WrapperTypeObjectPrototype";

I'm slightly confused as to why you have to add this enum.  I guess the info structs are short-cuts for initializing objects?  We must have objects in V8 which inherit from something other than object directly.  How do they get initialized?  Why wouldn't objects which inherit from Error do the same?  Is this enum just to make it so the shortcut can be used for either Object or Error subclasses?
Comment 38 Erik Arvidsson 2012-05-29 14:17:53 PDT
(In reply to comment #37)
> (From update of attachment 144602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2441
> > +    my $WrapperTypePrototype = $dataNode->isException ? "WrapperTypeErrorPrototype" : "WrapperTypeObjectPrototype";
> 
> I'm slightly confused as to why you have to add this enum.  I guess the info structs are short-cuts for initializing objects?  We must have objects in V8 which inherit from something other than object directly.  How do they get initialized?  Why wouldn't objects which inherit from Error do the same?  Is this enum just to make it so the shortcut can be used for either Object or Error subclasses?

DOM wrappers are created from FunctionTemplates. A FunctionTemplate can inherit from another FunctionTemplate and that causes the prototype chain to be set up correctly. However, FunctionTemplate cannot inherit from an object. To achieve that we therefore set the prototype when we create the constructor function (this happens once per context since the constructor objects are cached).
Comment 39 Eric Seidel (no email) 2012-05-29 16:09:30 PDT
Comment on attachment 144602 [details]
Patch

The code looks fine, I'd like someone with V8 knowledge to take a look.
Comment 40 Erik Arvidsson 2012-05-29 17:16:25 PDT
Anton, Haraken: Any comments?
Comment 41 Kentaro Hara 2012-05-29 17:34:21 PDT
Comment on attachment 144602 [details]
Patch

LGTM
Comment 42 Erik Arvidsson 2012-05-30 12:33:59 PDT
Created attachment 144896 [details]
Patch
Comment 43 Eric Seidel (no email) 2012-05-30 14:49:31 PDT
Comment on attachment 144896 [details]
Patch

LGTM.
Comment 44 Erik Arvidsson 2012-05-30 17:01:02 PDT
Oliver: Any final comments?
Comment 45 Oliver Hunt 2012-05-30 17:04:07 PDT
Comment on attachment 144896 [details]
Patch

If you're adding a new reference to the ErrorPrototype you need to mark it - look for how m_regExpPrototype, etc are marked
Comment 46 Erik Arvidsson 2012-05-30 17:16:20 PDT
(In reply to comment #45)
> (From update of attachment 144896 [details])
> If you're adding a new reference to the ErrorPrototype you need to mark it - look for how m_regExpPrototype, etc are marked

I assume you mean like this?

diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
index 645fa04..fb523ec 100644
--- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
@@ -374,6 +374,7 @@ void JSGlobalObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
     visitIfNeeded(visitor, &thisObject->m_numberPrototype);
     visitIfNeeded(visitor, &thisObject->m_datePrototype);
     visitIfNeeded(visitor, &thisObject->m_regExpPrototype);
+    visitIfNeeded(visitor, &thisObject->m_errorPrototype);
 
     visitIfNeeded(visitor, &thisObject->m_argumentsStructure);
     visitIfNeeded(visitor, &thisObject->m_arrayStructure);
Comment 47 Erik Arvidsson 2012-05-30 17:17:14 PDT
Created attachment 144952 [details]
Patch
Comment 48 Oliver Hunt 2012-05-30 17:18:52 PDT
Comment on attachment 144952 [details]
Patch

r=me, assuming no changes to the V8 stuff
Comment 49 WebKit Review Bot 2012-05-31 11:00:24 PDT
Comment on attachment 144952 [details]
Patch

Clearing flags on attachment: 144952

Committed r119124: <http://trac.webkit.org/changeset/119124>
Comment 50 WebKit Review Bot 2012-05-31 11:00:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 51 anton muhin 2012-05-31 11:03:39 PDT
Comment on attachment 144602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review

Sorry for late response, I was on vacation.

Where do you initlialize m_errorPrototype in V8 bindings?

Overall, cannot we move logic to setup prototype in function template generating?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> +#define V8_STORE_PRIMORDIAL(name, Name) \

up to you, but I would rather put most of the logic into a function and keep macro minimal to ease debugging/stack trace analysis.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> +    if (symbol.IsEmpty()) \

do you really need this check?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:65
> +    v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(m_context->Global()->Get(symbol)); \

.As<v8::Object>() is a better option imho

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:77
> +    if (prototypeString.IsEmpty())

ditto

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:119
> +        v8::Local<v8::Value> prototypeValue = function->Get(v8::String::NewSymbol("prototype"));

do you need a designated local for prototypeValue?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:120
> +        if (prototypeValue->IsObject())

when this check fails?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> +            v8::Local<v8::Object>::Cast(prototypeValue)->SetPrototype(m_errorPrototype.get());

again, .As<...>
Comment 52 Erik Arvidsson 2012-05-31 11:20:53 PDT
(In reply to comment #51)
> (From update of attachment 144602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review
> 
> Sorry for late response, I was on vacation.
> 
> Where do you initlialize m_errorPrototype in V8 bindings?

It is done in V8BindingPerContextData::init()

> Overall, cannot we move logic to setup prototype in function template generating?

No. FunctionTemplate can only inherit another FunctionTemplate, not a JS object. Yeah, this is backwards, maybe we should update the V8 API here?

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> > +#define V8_STORE_PRIMORDIAL(name, Name) \
> 
> up to you, but I would rather put most of the logic into a function and keep macro minimal to ease debugging/stack trace analysis.
> 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> > +    if (symbol.IsEmpty()) \
> 
> do you really need this check?

OOM check. All V8 functions can return an empty object. Maybe this is too defensive?

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:65
> > +    v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(m_context->Global()->Get(symbol)); \
> 
> .As<v8::Object>() is a better option imho

I go back and forth on this. I think that C++ programmers are more used to Cast but As is slight;y shorter.
 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:77
> > +    if (prototypeString.IsEmpty())
> 
> ditto
> 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:119
> > +        v8::Local<v8::Value> prototypeValue = function->Get(v8::String::NewSymbol("prototype"));
> 
> do you need a designated local for prototypeValue?

Same here... something might go wrong.

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:120
> > +        if (prototypeValue->IsObject())
> 
> when this check fails?
> 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> > +            v8::Local<v8::Object>::Cast(prototypeValue)->SetPrototype(m_errorPrototype.get());
> 
> again, .As<...>

This code is only run once so the extra checks are not really a perf issue but I agree with you that this code is maybe too defensive and if any of these checks fail we are pretty much screwed any way.

I can do a follow up patch if you want?
Comment 53 anton muhin 2012-05-31 11:32:08 PDT
(In reply to comment #52)
> (In reply to comment #51)
> > (From update of attachment 144602 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review
> > 
> > Sorry for late response, I was on vacation.
> > 
> > Where do you initlialize m_errorPrototype in V8 bindings?
> 
> It is done in V8BindingPerContextData::init()

Thanks, missed that behind the macros.

> 
> > Overall, cannot we move logic to setup prototype in function template generating?
> 
> No. FunctionTemplate can only inherit another FunctionTemplate, not a JS object. Yeah, this is backwards, maybe we should update the V8 API here?

Alas---templates are not context-sensitive while objects are, hence it's that way.  I was just hoping we may construct this Error from bindings code, but it's apparently not the case.

I am just really concerned with WrapperTypeInfo growing bigger and bigger.

> 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> > > +#define V8_STORE_PRIMORDIAL(name, Name) \
> > 
> > up to you, but I would rather put most of the logic into a function and keep macro minimal to ease debugging/stack trace analysis.
> > 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> > > +    if (symbol.IsEmpty()) \
> > 
> > do you really need this check?
> 
> OOM check. All V8 functions can return an empty object. Maybe this is too defensive?

No, v8 API never returns empty handle due to OOM: it'll try to GC heap and if even after GC the object cannot be allocated, VM will just kill the process.  Typical reason for V8 API to return an empty handle is an exception thrown.

> 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:65
> > > +    v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(m_context->Global()->Get(symbol)); \
> > 
> > .As<v8::Object>() is a better option imho
> 
> I go back and forth on this. I think that C++ programmers are more used to Cast but As is slight;y shorter.

At least when conceived, As<...> were planned as replacement for Cast :)  But it's stylistic.

> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:77
> > > +    if (prototypeString.IsEmpty())
> > 
> > ditto
> > 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:119
> > > +        v8::Local<v8::Value> prototypeValue = function->Get(v8::String::NewSymbol("prototype"));
> > 
> > do you need a designated local for prototypeValue?
> 
> Same here... something might go wrong.
> 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:120
> > > +        if (prototypeValue->IsObject())
> > 
> > when this check fails?
> > 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> > > +            v8::Local<v8::Object>::Cast(prototypeValue)->SetPrototype(m_errorPrototype.get());
> > 
> > again, .As<...>
> 
> This code is only run once so the extra checks are not really a perf issue but I agree with you that this code is maybe too defensive and if any of these checks fail we are pretty much screwed any way.

I am not concerned with performance, but rather with making code harder to understand: no one should go paranoid what's the edge case is.

> I can do a follow up patch if you want?

That's completely up to you.
Comment 54 Tim Horton 2012-05-31 14:35:05 PDT
Why did you add new chromium platform-specific layout test results instead of updating the shared results?
Comment 55 Erik Arvidsson 2012-05-31 14:46:05 PDT
(In reply to comment #54)
> Why did you add new chromium platform-specific layout test results instead of updating the shared results?

Not sure what you mean here. I consolidated chromium-win/chromium-mac to chromium
Comment 56 Tim Horton 2012-05-31 15:02:27 PDT
(In reply to comment #55)
> (In reply to comment #54)
> > Why did you add new chromium platform-specific layout test results instead of updating the shared results?
> 
> Not sure what you mean here. I consolidated chromium-win/chromium-mac to chromium

Ah. Still:  http://trac.webkit.org/changeset/119137