WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
patch w/ Dimitri's comments
(8.10 KB, patch)
2009-03-26 08:48 PDT
,
Mike Belshe
no flags
Details
Formatted Diff
Diff
more nitties
(8.22 KB, patch)
2009-03-26 13:39 PDT
,
Mike Belshe
dglazkov
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mike Belshe
Comment 1
2009-03-25 13:08:26 PDT
Created
attachment 28943
[details]
patch
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
Landed as
http://trac.webkit.org/changeset/42070
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug