Bug 24812

Summary: Add custom V8 bindings for HTMLDocument
Product: WebKit Reporter: Mike Belshe <mbelshe>
Component: WebCore JavaScriptAssignee: Mike Belshe <mbelshe>
Status: RESOLVED FIXED    
Severity: Normal CC: eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
dglazkov: review-
patch w/ Dimitri's comments
none
more nitties dglazkov: review+

Description Mike Belshe 2009-03-25 13:02:53 PDT
Add custom bindings for HTMLDocument in v8.
Comment 1 Mike Belshe 2009-03-25 13:08:26 PDT
Created attachment 28943 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-03-26 07:28:50 PDT
Comment on attachment 28943 [details]
patch

Looks good, except for:

Add bug URL in ChangeLog. I usually put it two line-breaks after Reviewed by ...

> +CALLBACK_FUNC_DECL(HTMLDocumentOpen) {

Brace on new line.

> +            if (context.IsEmpty()) return v8::Undefined();

return on new line.

> +            if (!function->IsFunction()) {
> +                V8Proxy::ThrowError(V8Proxy::TYPE_ERROR, "open is not a function");
> +                return v8::Undefined();
> +            }

use throwError helper from V8Proxy.h

> +    RefPtr<HTMLCollection> v = WTF::getPtr(imp->all());
> +    return V8Proxy::ToV8Object(V8ClassIndex::HTMLCOLLECTION, WTF::getPtr(v));

v -> collection?
Comment 3 Mike Belshe 2009-03-26 08:48:44 PDT
Created attachment 28970 [details]
patch w/ Dimitri's comments
Comment 4 David Levin 2009-03-26 10:02:39 PDT
Drive by comments.

> changelog
Needs a link to the bug in the changelog.

> V8HTMLDocumentCustom.cpp
Copyright: Use 2007, 2008, 2009 instead of range.

> static String WriteHelperGetString(const v8::Arguments& args) {
Lowercase the first letter of the function (camelCase).

> For WriteHelperGetString, HTMLDocumentWrite, and HTMLDocumentWriteln
The braces need to be moved to the next line.

> str += ToWebCoreString(args[i]);
Use toWebCoreString (in V8Binding.h)


> HTMLDocument* imp = V8Proxy::DOMWrapperToNode<HTMLDocument>(args.Holder());
Use whole words for variables.  imp -> document?


>                 V8Proxy::throwError("open is not a function");
>                return v8::Undefined();
Use throwError (from V8Proxy.h)


> v8::Local<v8::Value>* params = new v8::Local<v8::Value>[args.Length()];
Consider using OwnArrayPtr (from wtf/OwnArrayPtr.h)

> for (int i = 0; i < args.Length(); i++)
Consider ++i (as you did above).  Consistency is nice. 



Comment 5 Dimitri Glazkov (Google) 2009-03-26 13:05:44 PDT
LGTM once David's comments are addressed.
Comment 6 Mike Belshe 2009-03-26 13:38:17 PDT
(In reply to comment #4)
> > V8HTMLDocumentCustom.cpp
> Copyright: Use 2007, 2008, 2009 instead of range.

This kind of stuff isn't worth nitting.  Not changing.
Comment 7 Mike Belshe 2009-03-26 13:39:16 PDT
Created attachment 28978 [details]
more nitties
Comment 8 David Levin 2009-03-26 14:40:53 PDT
> > > V8HTMLDocumentCustom.cpp
> > Copyright: Use 2007, 2008, 2009 instead of range.
> This kind of stuff isn't worth nitting.  Not changing.

I think it took more time to write that comment that it would have to fix the issue :(

This is one of those things that WebKit reviews have commented on a few times.  For example, see https://bugs.webkit.org/show_bug.cgi?id=23797 which has comments from Darin Alder about this very issue.

Also the ChangeLog doesn't have a link to this bug.

Comment 9 Dimitri Glazkov (Google) 2009-03-28 09:19:13 PDT
Still no bug URL in ChangeLog, and for all functions, the opening brace should begin on a new line. But no need to keep nitting this -- I'l fix up and land.
Comment 10 Dimitri Glazkov (Google) 2009-03-28 09:24:28 PDT
Landed as http://trac.webkit.org/changeset/42070.