Bug 24812 – Add custom V8 bindings for HTMLDocument

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mike Belshe]

Add custom bindings for HTMLDocument in v8.

[Mike Belshe]

Created attachment 28943 [details]
patch

[Dimitri Glazkov (Google)]

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?

[Mike Belshe]

Created attachment 28970 [details]
patch w/ Dimitri's comments

[David Levin]

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. 

[Dimitri Glazkov (Google)]

LGTM once David's comments are addressed.

[Mike Belshe]

(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.

[Mike Belshe]

Created attachment 28978 [details]
more nitties

[David Levin]

> > > 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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/42070.