Add custom bindings for HTMLDocument in v8.
Created attachment 28943 [details] patch
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?
Created attachment 28970 [details] patch w/ Dimitri's comments
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.
LGTM once David's comments are addressed.
(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.
Created attachment 28978 [details] more nitties
> > > 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.
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.
Landed as http://trac.webkit.org/changeset/42070.