[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