Yet more scrubbins. Navigator, Clipboard, Document, and Node.

Yet more scrubbins. Navigator, Clipboard, Document, and Node.

R=eseidel

[eseidel, 2009-02-26 18:59:57]

Yay scrubbing!

it's not you man.. it's the code which makes me stabby. ;)

Nice as always!  LGTM.

http://codereview.chromium.org/27217/diff/1/3
File third_party/WebKit/WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
(right):

http://codereview.chromium.org/27217/diff/1/3#newcode50
Line 50: Clipboard* clipboard =
V8Proxy::ToNativeObject<Clipboard>(V8ClassIndex::CLIPBOARD, info.Holder());
continued sighs of disgust.

http://codereview.chromium.org/27217/diff/1/3#newcode70
Line 70: if (args.Length() == 0) {
no isEmpty()?

http://codereview.chromium.org/27217/diff/1/3#newcode76
Line 76: clipboard->clearData(toWebCoreString(args[0]));
clear the data and set it to a string??

http://codereview.chromium.org/27217/diff/1/3#newcode80
Line 80: return throwError("clearData: Invalid number of arguments",
V8Proxy::SYNTAX_ERROR);
I think if I were writing this I would have put this error first and inverted
the if.

http://codereview.chromium.org/27217/diff/1/3#newcode88
Line 88: if (args.Length() != 1)
Similar to how this one was done!

http://codereview.chromium.org/27217/diff/1/3#newcode110
Line 110: return result ? v8::True() : v8::False();
We should have a v8Boolean(result) for this sort of thing.

http://codereview.chromium.org/27217/diff/1/3#newcode135
Line 135: return throwError("setDragImageFromElement: Invalid first argument",
V8Proxy::SYNTAX_ERROR);
Um1?  Why is one of these a SYNTAX error and the other not?  Seems the ifs could
be combined into one, and they could both not be.

http://codereview.chromium.org/27217/diff/1/4
File third_party/WebKit/WebCore/bindings/v8/custom/V8DocumentCustom.cpp (right):

http://codereview.chromium.org/27217/diff/1/4#newcode59
Line 59: if (V8Node::HasInstance(args[1]))
What, pray tell, does "has instance" mean?

http://codereview.chromium.org/27217/diff/1/4#newcode66
Line 66: resolver = new JSXPathNSResolver(args[2]->ToObject());
JS?  I thought that was only for JSC...  And why is this not leaked?  Does
document->evaluate() ref it?  In which case, why can't we hold it in a RefPtr
here?  Also, we don't really use "new" much anymore in WebCore, create() is our
preferred way for RefCounted stuff.  If it's not refcounted it should be in an
OwnPtr.

http://codereview.chromium.org/27217/diff/1/4#newcode75
Line 75: v8::TryCatch exceptionCatcher;
Does this need to be a block { }?

http://codereview.chromium.org/27217/diff/1/4#newcode80
Line 80: if (!ec)
if (ec)!  EEEK!  Seems we should have a test for this... I kinda think
document.evaluate() may be broken in other ways. ;)

http://codereview.chromium.org/27217/diff/1/2
File third_party/WebKit/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp
(right):

http://codereview.chromium.org/27217/diff/1/2#newcode43
Line 43: v8::Handle<v8::Object> holder = info.Holder();
Stab!  Die "holder" die!

http://codereview.chromium.org/27217/diff/1/2#newcode44
Line 44: Navigator* imp =
V8Proxy::ToNativeObject<Navigator>(V8ClassIndex::NAVIGATOR, holder);
stab!  die imp die!

[dglazkov, 2009-02-26 22:15:42]

Thanks for reviewing -- here are the fixups.

http://codereview.chromium.org/27217/diff/1/3
File third_party/WebKit/WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
(right):

http://codereview.chromium.org/27217/diff/1/3#newcode70
Line 70: if (args.Length() == 0) {
On 2009/02/26 18:59:57, Eric Seidel (Google) wrote:
> no isEmpty()?

Not on v8::Arguments.

http://codereview.chromium.org/27217/diff/1/3#newcode76
Line 76: clipboard->clearData(toWebCoreString(args[0]));
On 2009/02/26 18:59:57, Eric Seidel (Google) wrote:
> clear the data and set it to a string??

No, the argument is a content type to identify which part of clipboard is to be
cleared. I changed it back to a local with a clarifying name.

http://codereview.chromium.org/27217/diff/1/3#newcode80
Line 80: return throwError("clearData: Invalid number of arguments",
V8Proxy::SYNTAX_ERROR);
I pretended as if you were writing this. Fixed.

http://codereview.chromium.org/27217/diff/1/3#newcode110
Line 110: return result ? v8::True() : v8::False();
On 2009/02/26 18:59:57, Eric Seidel (Google) wrote:
> We should have a v8Boolean(result) for this sort of thing.

Your wish is my command. I am rietveld Genie.

http://codereview.chromium.org/27217/diff/1/3#newcode135
Line 135: return throwError("setDragImageFromElement: Invalid first argument",
V8Proxy::SYNTAX_ERROR);
On 2009/02/26 18:59:57, Eric Seidel (Google) wrote:
> Um1?  Why is one of these a SYNTAX error and the other not?  Seems the ifs
could
> be combined into one, and they could both not be.

True. Done. And it's a type error, not syntax error.

http://codereview.chromium.org/27217/diff/1/4
File third_party/WebKit/WebCore/bindings/v8/custom/V8DocumentCustom.cpp (right):

http://codereview.chromium.org/27217/diff/1/4#newcode66
Line 66: resolver = new JSXPathNSResolver(args[2]->ToObject());
Yeah, I was wondering the same thing, too. This is a class I haven't touched
yet. I will figure it out when I get there.

http://codereview.chromium.org/27217/diff/1/4#newcode75
Line 75: v8::TryCatch exceptionCatcher;
On 2009/02/26 18:59:57, Eric Seidel (Google) wrote:
> Does this need to be a block { }?

Nah. result is needed until the end of function scope anyway.

http://codereview.chromium.org/27217/diff/1/4#newcode80
Line 80: if (!ec)
On 2009/02/26 18:59:57, Eric Seidel (Google) wrote:
> if (ec)!  EEEK!  Seems we should have a test for this... I kinda think
> document.evaluate() may be broken in other ways. ;)

Done.

[eseidel, 2009-02-26 22:26:50]

Skimmed again for your enjoyment.

http://codereview.chromium.org/27217/diff/12/1006
File third_party/WebKit/WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
(right):

http://codereview.chromium.org/27217/diff/12/1006#newcode89
Line 89: return throwError("getData: Invalid number of arguments",
V8Proxy::SYNTAX_ERROR);
syntax?  Maybe v8 views JS as a non-dynamic language?  I mean, I guess for
predefined functions it makes sense to call arg count errors "syntax"?  Not
sure.

http://codereview.chromium.org/27217/diff/12/1006#newcode105
Line 105: return throwError("setData: Invalid number of arguments",
V8Proxy::SYNTAX_ERROR);
Looks like this also shouldnt' be a syntax error.  Seems we have a bunch of
syntax error abuse.

[dglazkov, 2009-02-26 22:34:29]

http://codereview.chromium.org/27217/diff/12/1006
File third_party/WebKit/WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
(right):

http://codereview.chromium.org/27217/diff/12/1006#newcode89
Line 89: return throwError("getData: Invalid number of arguments",
V8Proxy::SYNTAX_ERROR);
On 2009/02/26 22:26:50, Eric Seidel (Google) wrote:
> syntax?  Maybe v8 views JS as a non-dynamic language?  I mean, I guess for
> predefined functions it makes sense to call arg count errors "syntax"?  Not
> sure.

So, basically this is how I understand it: number of arguments mismatch is a
syntax error. Incorrect argument type is a type error.

It's the same way in JSC:
http://trac.webkit.org/browser/trunk/WebCore/bindings/js/JSClipboardCustom.cpp