RESOLVED FIXED 24812
Add custom V8 bindings for HTMLDocument
https://bugs.webkit.org/show_bug.cgi?id=24812
Summary Add custom V8 bindings for HTMLDocument
Mike Belshe
Reported 2009-03-25 13:02:53 PDT
Add custom bindings for HTMLDocument in v8.
Attachments
patch (8.09 KB, patch)
2009-03-25 13:08 PDT, Mike Belshe
dglazkov: review-
patch w/ Dimitri's comments (8.10 KB, patch)
2009-03-26 08:48 PDT, Mike Belshe
no flags
more nitties (8.22 KB, patch)
2009-03-26 13:39 PDT, Mike Belshe
dglazkov: review+
Mike Belshe
Comment 1 2009-03-25 13:08:26 PDT
Dimitri Glazkov (Google)
Comment 2 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?
Mike Belshe
Comment 3 2009-03-26 08:48:44 PDT
Created attachment 28970 [details] patch w/ Dimitri's comments
David Levin
Comment 4 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.
Dimitri Glazkov (Google)
Comment 5 2009-03-26 13:05:44 PDT
LGTM once David's comments are addressed.
Mike Belshe
Comment 6 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.
Mike Belshe
Comment 7 2009-03-26 13:39:16 PDT
Created attachment 28978 [details] more nitties
David Levin
Comment 8 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.
Dimitri Glazkov (Google)
Comment 9 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.
Dimitri Glazkov (Google)
Comment 10 2009-03-28 09:24:28 PDT
Note You need to log in before you can comment on or make changes to this bug.