[Webkit-unassigned] [Bug 26258] Finish upstreaming V8Custom
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 9 17:22:03 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=26258
levin at chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #31101|review? |review-
Flag| |
------- Comment #6 from levin at chromium.org 2009-06-09 17:22 PDT -------
(From update of attachment 31101)
This is a lot better. Just a few last changes and we'll both be done with this
one :)
> Index: WebCore/bindings/v8/custom/V8CustomBinding.cpp
> +ACCESSOR_GETTER(DocumentImplementation)
> +{
> + ASSERT(info.Holder()->InternalFieldCount() >= kDocumentMinimumInternalFieldCount);
> + // Check if the internal field already contains a wrapper.
> + v8::Local<v8::Value> implementation = info.Holder()->GetInternalField(kDocumentImplementationIndex);
> + if (!implementation->IsUndefined())
> + return implementation;
> + // Generate a wrapper.
> + Document* document = V8Proxy::DOMWrapperToNative<Document>(info.Holder());
> + v8::Handle<v8::Value> wrapper = V8Proxy::DOMImplementationToV8Object(document->implementation());
> + // Store the wrapper in the internal field.
> + info.Holder()->SetInternalField(kDocumentImplementationIndex, wrapper);
> +
This code feels very dense. It would look better with returns before each
comment line.
Right before NAMED_ACCESS_CHECK(History) there is an extra blank line.
> +// static
I would just delete this comment.
> +Frame* V8Custom::GetTargetFrame(v8::Local<v8::Object> host, v8::Local<v8::Value> data)
> +{
> + Frame* target = 0;
> + switch (V8ClassIndex::FromInt(data->Int32Value())) {
> + case V8ClassIndex::DOMWINDOW: {
> + v8::Handle<v8::Value> window = V8Proxy::LookupDOMWrapper(V8ClassIndex::DOMWINDOW, host);
> + if (window.IsEmpty())
> + return target;
> +
> + DOMWindow* targetWin = V8Proxy::ToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, window);
s/targetWin/targetWindow/ (avoid abbreviations).
> +#if ENABLE(SVG)
> +V8ClassIndex::V8WrapperType V8Custom::DowncastSVGPathSeg(void* path_seg)
path_seg bad casing -- use camelCase.
> Index: WebCore/bindings/v8/custom/V8CustomBinding.h
> ===================================================================
> +
> +#define ACCESSOR_GETTER(NAME) \
> +v8::Handle<v8::Value> V8Custom::v8##NAME##AccessorGetter( \
> + v8::Local<v8::String> name, const v8::AccessorInfo& info)
These defines that aren't indented are confusing to read.
Here's two alternatives:
#define ACCESSOR_GETTER(NAME) \
v8::Handle<v8::Value> V8Custom::v8##NAME##AccessorGetter( \
v8::Local<v8::String> name, const v8::AccessorInfo& info)
#define ACCESSOR_GETTER(NAME) \
v8::Handle<v8::Value>
V8Custom::v8##NAME##AccessorGetter(v8::Local<v8::String> name, const
v8::AccessorInfo& info)
> + class V8Custom {
> + public:
> +
Typically this is no blank line after public:, private:, etc.
> + // Constants.
> + static const int kMessagePortInternalFieldCount = kDefaultWrapperInternalFieldCount + 2;
> +
> + #if ENABLE(WORKERS)
Don't indent the preprocessor directives.
> + static const int kStyleSheetOwnerNodeIndex = kDefaultWrapperInternalFieldCount + 0;
> + static const int kStyleSheetInternalFieldCount = kDefaultWrapperInternalFieldCount + 1;
> +
> + #define DECLARE_PROPERTY_ACCESSOR_GETTER(NAME) \
Don't indent the preprocessor directives.
> +
> + #define DECLARE_PROPERTY_ACCESSOR(NAME) DECLARE_PROPERTY_ACCESSOR_GETTER(NAME); DECLARE_PROPERTY_ACCESSOR_SETTER(NAME);
I'd omit the trailing ";"
> + DECLARE_PROPERTY_ACCESSOR_GETTER(DOMWindowCrypto);
> + // Customized getter&setter of DOMWindow.location
s/&/and/ This comment doesn't event seem correct as it is just a setter.
These comments appear to be inconsistently done and don't seem to provide any
value. (They don't answer Why? just What? and the code already answer What?
just fine.)
A blank line to separate the different objects seems sufficient (and delete the
comments).
> + DECLARE_CALLBACK(NodeAppendChild);
> +
> + // Custom implementation is Navigator properties.
The sentence doesn't seem grammatically correct.
> + // We actually only need this because WebKit has
> + // navigator.appVersion as custom. Our version just
> + // passes through.
I like this comment as it answers: Why?
> + DECLARE_PROPERTY_ACCESSOR(NavigatorAppVersion);
> + #if ENABLE(DATABASE)
Don't indent preprocessor directives.
> +
> + private:
> + static v8::Handle<v8::Value> WindowSetTimeoutImpl(const v8::Arguments& args, bool singleShot);
Remove the param name "args" since it doesn't add any information.
> Index: WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp
> ===================================================================
> --- WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp (revision 0)
> +++ WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp (revision 0)
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (C) 2007,2008,2009 Google Inc. All rights reserved.
Spaces after commas.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list