Implement better JS exception handling by abstracting KJS::ExecState into an ...

Implement better JS exception handling by abstracting KJS::ExecState into an ExceptionContext, implement custom v8 bindings for TreeWalker, NodeFilter, NodeIterator, and .querySelector that were affected by this, make the world a better place by ticking 98/100 on acid3 test.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2648

[dglazkov, 2008-09-25 23:33:32]

This one is chunky. Please take a look.

[macdome_gmail.com, 2008-09-26 14:31:40]

Looks great!  I look forward to seeing the upstream bug go by...

I talked with mjs and olliej in #webkit about this this morning.  They were
less-than-enthused, but saw the need for v8.  Obj-C doesn't need this they say,
because obj-c tries to avoid exceptions anyway.  Other language bindings (if we
ever landed weinigs lua DOM patch, etc) would need something like this of
course.

http://codereview.chromium.org/4097/diff/71/103
File webkit/pending/ExceptionContext.h (right):

http://codereview.chromium.org/4097/diff/71/103#newcode41
Line 41: #include "v8_proxy.h"
Probably should be ScriptController.h

http://codereview.chromium.org/4097/diff/71/90
File webkit/port/bindings/v8/JSNSResolver.cpp (right):

http://codereview.chromium.org/4097/diff/71/90#newcode67
Line 67: Frame* frame = V8Proxy::retrieveActiveFrame();
I think there is a ScriptController version of this, or there should be... Maybe
I haven't written it yet.  Ideally we will use only ScriptController and
evenetually v8proxy will fade into SC.

http://codereview.chromium.org/4097/diff/71/90#newcode92
Line 92: return ToWebCoreString(retval);
We can add upstream String constructors eventually to take v8 types.

http://codereview.chromium.org/4097/diff/71/94
File webkit/port/bindings/v8/JSNSResolver.h (right):

http://codereview.chromium.org/4097/diff/71/94#newcode33
Line 33: #ifndef JSNSRESOLVER_H__
These are old-style Google names.  I think the new style doesn't have a second
__ and WebKit uses JSNSResolver_h anyway.

http://codereview.chromium.org/4097/diff/71/94#newcode50
Line 50: virtual String lookupNamespaceURI(ExceptionContext* exec,
WebKit style would not include the argument name for ExceptionContext*

http://codereview.chromium.org/4097/diff/71/94#newcode51
Line 51: const String& prefix);
This will likely fit on the same line w/ the arg name gone from the previous
arg.

http://codereview.chromium.org/4097/diff/71/96
File webkit/port/bindings/v8/v8_custom.cpp (right):

http://codereview.chromium.org/4097/diff/71/96#newcode2356
Line 2356: NSResolver* resolver = NULL;
WebKit style uses 0 for NULL in c++ code.  NULL is only used in C code, and nil
in Obj-C code.

http://codereview.chromium.org/4097/diff/71/96#newcode2361
Line 2361: v8::Handle<v8::Object> obj = args[2]->ToObject();
Why does this local need to be used?

http://codereview.chromium.org/4097/diff/71/96#newcode2370
Line 2370: if (ec != 0) {
WebKit style would be if (ec)

http://codereview.chromium.org/4097/diff/71/96#newcode2399
Line 2399: if (ec != 0) {
again, if (ec)

http://codereview.chromium.org/4097/diff/71/91
File webkit/port/bindings/v8/v8_nodefilter.cpp (right):

http://codereview.chromium.org/4097/diff/71/91#newcode76
Line 76: if (exception_context->hadException()) {
Sigh.  Eventually this will need to be WebKit style too, if it's going upstream.
 exceptionContext

http://codereview.chromium.org/4097/diff/71/105
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/71/105#newcode55
Line 55: m_currentCatcher->updateContext();
4 spaces in wk style.

[dglazkov, 2008-09-26 17:15:07]

Ready for another look. Also adding Feng, since he did the original TreeWalker
patch a while back.

http://codereview.chromium.org/4097/diff/71/103
File webkit/pending/ExceptionContext.h (right):

http://codereview.chromium.org/4097/diff/71/103#newcode41
Line 41: #include "v8_proxy.h"
On 2008/09/26 14:31:40, Eric Seidel wrote:
> Probably should be ScriptController.h

Even better!

http://codereview.chromium.org/4097/diff/71/94
File webkit/port/bindings/v8/JSNSResolver.h (right):

http://codereview.chromium.org/4097/diff/71/94#newcode33
Line 33: #ifndef JSNSRESOLVER_H__
On 2008/09/26 14:31:40, Eric Seidel wrote:
> These are old-style Google names.  I think the new style doesn't have a second
> __ and WebKit uses JSNSResolver_h anyway.

Done.

http://codereview.chromium.org/4097/diff/71/94#newcode50
Line 50: virtual String lookupNamespaceURI(ExceptionContext* exec,
On 2008/09/26 14:31:40, Eric Seidel wrote:
> WebKit style would not include the argument name for ExceptionContext*

Done. Also cleaned up constructor args.

http://codereview.chromium.org/4097/diff/71/94#newcode51
Line 51: const String& prefix);
On 2008/09/26 14:31:40, Eric Seidel wrote:
> This will likely fit on the same line w/ the arg name gone from the previous
> arg.

Done.

http://codereview.chromium.org/4097/diff/71/96
File webkit/port/bindings/v8/v8_custom.cpp (right):

http://codereview.chromium.org/4097/diff/71/96#newcode2356
Line 2356: NSResolver* resolver = NULL;
On 2008/09/26 14:31:40, Eric Seidel wrote:
> WebKit style uses 0 for NULL in c++ code.  NULL is only used in C code, and
nil
> in Obj-C code.

Apologies. There are cases of both NULL and 0 being used in this file, so I just
picked one :) Done.

http://codereview.chromium.org/4097/diff/71/96#newcode2361
Line 2361: v8::Handle<v8::Object> obj = args[2]->ToObject();
On 2008/09/26 14:31:40, Eric Seidel wrote:
> Why does this local need to be used?

It doesn't. I think this was a remnant of something I picked up from the
generated code.

http://codereview.chromium.org/4097/diff/71/96#newcode2370
Line 2370: if (ec != 0) {
On 2008/09/26 14:31:40, Eric Seidel wrote:
> WebKit style would be if (ec)

I picked the majority here. To avoid further disparity and until we start
readying this file for upstream, I left these alone.

http://codereview.chromium.org/4097/diff/71/96#newcode2399
Line 2399: if (ec != 0) {
On 2008/09/26 14:31:40, Eric Seidel wrote:
> again, if (ec)

Done.

http://codereview.chromium.org/4097/diff/71/91
File webkit/port/bindings/v8/v8_nodefilter.cpp (right):

http://codereview.chromium.org/4097/diff/71/91#newcode76
Line 76: if (exception_context->hadException()) {
On 2008/09/26 14:31:40, Eric Seidel wrote:
> Sigh.  Eventually this will need to be WebKit style too, if it's going
upstream.
>  exceptionContext

Agreed. I wasn't sure which parts were going upstream eventually, so I stuck
with the style of the file. IMHO, it's better to keep the style consistent so
that when ready to convert, you don't have the "dueling style banjos" to deal
with.

http://codereview.chromium.org/4097/diff/71/105
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/71/105#newcode55
Line 55: m_currentCatcher->updateContext();
On 2008/09/26 14:31:40, Eric Seidel wrote:
> 4 spaces in wk style.

Done.

[dglazkov, 2008-09-26 17:17:59]

Added a note about the exception logging.

http://codereview.chromium.org/4097/diff/71/105
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/71/105#newcode80
Line 80: m_catcher.SetVerbose(true);
We don't want these exceptions to be printed to the console -- because we're
re-throwing them later.

[macdome_gmail.com, 2008-09-26 17:47:38]

I think maybe you didn't upload all files?  at least JSNSResolver.h looks old...

http://codereview.chromium.org/4097/diff/71/103
File webkit/pending/ExceptionContext.h (right):

http://codereview.chromium.org/4097/diff/71/103#newcode59
Line 59: class ExceptionContext {
Should be NonCopyable I think.

http://codereview.chromium.org/4097/diff/71/103#newcode67
Line 67: static ExceptionContext* create();
the create() paradigm is only for RefCounted objects.  This can just use a
normal default constructor.

http://codereview.chromium.org/4097/diff/71/90
File webkit/port/bindings/v8/JSNSResolver.cpp (right):

http://codereview.chromium.org/4097/diff/71/90#newcode68
Line 68: log_info(frame, "NSResolver does not have a lookupNamespaceURI
method.",
Hum... I guess v8 has its own logging system?  Funny.  I mean, I guess JSC does
too (in wtf), but WebCore/WebKit share the wtf one.

http://codereview.chromium.org/4097/diff/71/90#newcode88
Line 88: if (exceptionContext->hadException()) {
Since this looks to be in WK style, there should be no { } around a single-line
if.

http://codereview.chromium.org/4097/diff/71/94
File webkit/port/bindings/v8/JSNSResolver.h (right):

http://codereview.chromium.org/4097/diff/71/94#newcode33
Line 33: #ifndef JSNSRESOLVER_H__
On 2008/09/26 17:15:07, Dimitri Glazkov wrote:
> On 2008/09/26 14:31:40, Eric Seidel wrote:
> > These are old-style Google names.  I think the new style doesn't have a
second
> > __ and WebKit uses JSNSResolver_h anyway.
> 
> Done.

Am I looking at the right version?  doesn't look done...

[dglazkov, 2008-09-26 20:51:07]

After a bit more testing, strenghtenin' and straightenin', ready for another
look. We're now failing only two selector API tests, and I am working on that.

http://codereview.chromium.org/4097/diff/71/103
File webkit/pending/ExceptionContext.h (right):

http://codereview.chromium.org/4097/diff/71/103#newcode59
Line 59: class ExceptionContext {
On 2008/09/26 17:47:38, Eric Seidel wrote:
> Should be NonCopyable I think.

Done.

http://codereview.chromium.org/4097/diff/71/103#newcode67
Line 67: static ExceptionContext* create();
On 2008/09/26 17:47:38, Eric Seidel wrote:
> the create() paradigm is only for RefCounted objects.  This can just use a
> normal default constructor.

Done.

http://codereview.chromium.org/4097/diff/71/90
File webkit/port/bindings/v8/JSNSResolver.cpp (right):

http://codereview.chromium.org/4097/diff/71/90#newcode67
Line 67: Frame* frame = V8Proxy::retrieveActiveFrame();
On 2008/09/26 14:31:40, Eric Seidel wrote:
> I think there is a ScriptController version of this, or there should be...
Maybe
> I haven't written it yet.  Ideally we will use only ScriptController and
> evenetually v8proxy will fade into SC.

You did. Done.

http://codereview.chromium.org/4097/diff/71/90#newcode68
Line 68: log_info(frame, "NSResolver does not have a lookupNamespaceURI
method.",
On 2008/09/26 17:47:38, Eric Seidel wrote:
> Hum... I guess v8 has its own logging system?  Funny.  I mean, I guess JSC
does
> too (in wtf), but WebCore/WebKit share the wtf one.

Leaving as-is for now.

http://codereview.chromium.org/4097/diff/71/90#newcode88
Line 88: if (exceptionContext->hadException()) {
On 2008/09/26 17:47:38, Eric Seidel wrote:
> Since this looks to be in WK style, there should be no { } around a
single-line
> if.

Done.

http://codereview.chromium.org/4097/diff/71/90#newcode92
Line 92: return ToWebCoreString(retval);
On 2008/09/26 14:31:40, Eric Seidel wrote:
> We can add upstream String constructors eventually to take v8 types.

That would be neat! :)

http://codereview.chromium.org/4097/diff/71/94
File webkit/port/bindings/v8/JSNSResolver.h (right):

http://codereview.chromium.org/4097/diff/71/94#newcode33
Line 33: #ifndef JSNSRESOLVER_H__
On 2008/09/26 17:47:38, Eric Seidel wrote:
> On 2008/09/26 17:15:07, Dimitri Glazkov wrote:
> > On 2008/09/26 14:31:40, Eric Seidel wrote:
> > > These are old-style Google names.  I think the new style doesn't have a
> second
> > > __ and WebKit uses JSNSResolver_h anyway.
> > 
> > Done.
> 
> Am I looking at the right version?  doesn't look done...

It's done, I promise! :)

[macdome_gmail.com, 2008-09-26 20:59:45]

Looks good.

http://codereview.chromium.org/4097/diff/282/584
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/282/584#newcode60
Line 60: ExceptionContext* ExceptionContext::createFromNode(Node* node)
No need to list the argument name, this will just cause warnings in some
compilers.

http://codereview.chromium.org/4097/diff/282/584#newcode66
Line 66: JSException ExceptionContext::NoException() {
WebKit style.  { on a new line.

http://codereview.chromium.org/4097/diff/282/584#newcode70
Line 70: ExceptionCatcher::ExceptionCatcher(ExceptionContext *exceptionContext)
WebKit style *

http://codereview.chromium.org/4097/diff/282/584#newcode89
Line 89: m_context->setCurrentCatcher(0);
Feels like this should really be just setExecptionCatcher(0) instead of being
called the "current" catcher.  If you ever change the catcher on the context,
does it update the context to no longer point to the catcher?  We should
probably just ASSERT that case, that setExceptionCatcher never changes the
pointer, only 0s it or sets it.

[dglazkov, 2008-09-26 21:43:27]

Ready for yet another look.

I promise it's the last one :)

http://codereview.chromium.org/4097/diff/282/584
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/282/584#newcode60
Line 60: ExceptionContext* ExceptionContext::createFromNode(Node* node)
On 2008/09/26 20:59:45, Eric Seidel wrote:
> No need to list the argument name, this will just cause warnings in some
> compilers.

Done.

http://codereview.chromium.org/4097/diff/282/584#newcode66
Line 66: JSException ExceptionContext::NoException() {
On 2008/09/26 20:59:45, Eric Seidel wrote:
> WebKit style.  { on a new line.

Done.

http://codereview.chromium.org/4097/diff/282/584#newcode70
Line 70: ExceptionCatcher::ExceptionCatcher(ExceptionContext *exceptionContext)
On 2008/09/26 20:59:45, Eric Seidel wrote:
> WebKit style *

Done.

http://codereview.chromium.org/4097/diff/282/584#newcode89
Line 89: m_context->setCurrentCatcher(0);
On 2008/09/26 20:59:45, Eric Seidel wrote:
> Feels like this should really be just setExecptionCatcher(0) instead of being
> called the "current" catcher.  If you ever change the catcher on the context,
> does it update the context to no longer point to the catcher?  We should
> probably just ASSERT that case, that setExceptionCatcher never changes the
> pointer, only 0s it or sets it.

Huh. Made me think about nested/multiple catchers. Because v8::TryCatch chains
up exception handlers, you should be able to have nested ExceptionCatchers
without a problem. Tweaked to allow for that.

[macdome_gmail.com, 2008-09-26 22:00:22]

LGTM.

http://codereview.chromium.org/4097/diff/287/669
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/287/669#newcode49
Line 49: if (m_currentCatcher && exceptionCatcher)
you call the method setExceptionCacther, but the ivar m_currentCatcher.  i would
expect the ivar to match.  m_exceptionCatcher.

http://codereview.chromium.org/4097/diff/287/669#newcode65
Line 65: // V8's ExceptionContext does not use Node or its ScriptingContext
Your comment says "what" instead of "why".  The code already says what. :)  And
I don't know the why...

[dglazkov, 2008-09-26 22:12:38]

Committed! Yay! TGIF!

http://codereview.chromium.org/4097/diff/287/669
File webkit/port/bridge/ExceptionContextV8.cpp (right):

http://codereview.chromium.org/4097/diff/287/669#newcode49
Line 49: if (m_currentCatcher && exceptionCatcher)
On 2008/09/26 22:00:22, Eric Seidel wrote:
> you call the method setExceptionCacther, but the ivar m_currentCatcher.  i
would
> expect the ivar to match.  m_exceptionCatcher.  

Changed.

http://codereview.chromium.org/4097/diff/287/669#newcode65
Line 65: // V8's ExceptionContext does not use Node or its ScriptingContext
On 2008/09/26 22:00:22, Eric Seidel wrote:
> Your comment says "what" instead of "why".  The code already says what. :) 
And
> I don't know the why...

Added the whys.