Bug 26258 – Finish upstreaming V8Custom

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

See summary.

[Nate Chapin]

Created attachment 31063 [details]
patch

[David Levin]

Comment on attachment 31063 [details]
patch

This is a really great start.  There are a few things to fix before it will be ready to land that I've noted below so r- for now.


> Index: WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (C) 2007-2009 Google Inc. All rights reserved.

Use comma separated years.


> Index: WebCore/bindings/v8/custom/V8CustomBinding.h
> ===================================================================
> @@ -31,19 +31,552 @@
>  #ifndef V8CustomBinding_h
>  #define V8CustomBinding_h
>  
> -// FIXME: This is a temporary forwarding header until all bindings have migrated
> -// over and v8_custom actually becomes V8CustomBinding.
> -#include "v8_custom.h"
> +#include <v8.h>
> +#include "V8Index.h"

Move <v8.h> to be after "V8Index.h"

>  
> +struct NPObject;
> +
> +#define CALLBACK_FUNC_DECL(NAME)                \
The trailing slashes seem inconsistent through this patch (sometimes lots of spaces, other times 0, 1, or 2).  It would be nice to make them all just one space after the end of the code.

> +v8::Handle<v8::Value> V8Custom::v8##NAME##Callback(const v8::Arguments& args)

Other places that have define continuations seem to indent the continued line (or you could just expand these to be on one line).

>  namespace WebCore {
>  
> -    class HTMLFrameElementBase;
> -    class Element;
> -    class String;
> +class HTMLFrameElementBase;
> +class Element;
> +class Frame;
> +class V8Proxy;
> +class String;
> +class HTMLCollection;
> +class DOMWindow;

Sort these. 
Also inside a header file items in a namespace should be indented.


> +#define DECLARE_PROPERTY_ACCESSOR_GETTER(NAME) \
> +static v8::Handle<v8::Value> v8##NAME##AccessorGetter(\
> +    v8::Local<v8::String> name, const v8::AccessorInfo& info);
> +
> +#define DECLARE_PROPERTY_ACCESSOR_SETTER(NAME)  \
> +static void v8##NAME##AccessorSetter(v8::Local<v8::String> name, \
> +                                     v8::Local<v8::Value> value, \
> +                                     const v8::AccessorInfo& info);
> +
> +#define DECLARE_PROPERTY_ACCESSOR(NAME) \
> +  DECLARE_PROPERTY_ACCESSOR_GETTER(NAME) \
> +  DECLARE_PROPERTY_ACCESSOR_SETTER(NAME)
> +
> +
> +#define DECLARE_NAMED_PROPERTY_GETTER(NAME)  \
> +static v8::Handle<v8::Value> v8##NAME##NamedPropertyGetter(\
> +    v8::Local<v8::String> name, const v8::AccessorInfo& info);
> +
> +#define DECLARE_NAMED_PROPERTY_SETTER(NAME)  \
> +static v8::Handle<v8::Value> v8##NAME##NamedPropertySetter(\
> +    v8::Local<v8::String> name, \
> +    v8::Local<v8::Value> value, \
> +    const v8::AccessorInfo& info);
> +
> +#define DECLARE_NAMED_PROPERTY_DELETER(NAME)  \
> +static v8::Handle<v8::Boolean> v8##NAME##NamedPropertyDeleter(\
> +    v8::Local<v8::String> name, const v8::AccessorInfo& info);
> +

Would be nice to remove the ; from the #define so that the user of the define would have to add it.



> +DECLARE_PROPERTY_ACCESSOR(CanvasRenderingContext2DStrokeStyle)

These should be indented (like the method declarations that they are).


> +    static NPObject* GetHTMLPlugInElementNPObject(v8::Handle<v8::Object> object);

Since the parameter name "object" doesn't add any information, it should be removed.


> +    static V8ClassIndex::V8WrapperType DowncastSVGPathSeg(void* path_seg);

path_seg has incorrect casing.


> +    static v8::Handle<v8::Value> WindowSetTimeoutImpl(const v8::Arguments& args,
> +                                                    bool single_shot);
single_shot incorrect casing (and the indentation is off).

> +    static void ClearTimeoutImpl(const v8::Arguments& args);

Remove the param name "args".



> Index: WebCore/bindings/v8/custom/V8CustomBinding.cpp
> ===================================================================


> +// DOMImplementation is a singleton in WebCore.  If we use our normal
> +// mapping from DOM objects to V8 wrappers, the same wrapper will be
> +// shared for all frames in the same process.  This is a major
> +// security problem.  Therefore, we generate a DOMImplementation
> +// wrapper per document and store it in an internal field of the
> +// document.  Since the DOMImplementation object is a singleton, we do
> +// not have to do anything to keep the DOMImplementation object alive
> +// for the lifetime of the wrapper.

WebKit uses a single space after .

> +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.
Does this comment clarify anything?

> +    Document* doc = V8Proxy::DOMWrapperToNative<Document>(info.Holder());

Avoid abbreviations s/doc/document/




> +// --------------- Security Checks -------------------------
> +INDEXED_ACCESS_CHECK(History)
> +{
> +    // Only allow same origin access
Add "."

> +    History* imp = V8Proxy::ToNativeObject<History>(V8ClassIndex::HISTORY, host);

"imp" should be replaced with a better name (several instance).

> +
> +// static
> +Frame* V8Custom::GetTargetFrame(v8::Local<v8::Object> host, v8::Local<v8::Value> data)
> +{
>
> +        DOMWindow* target_win = V8Proxy::ToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, window);
target_win bad casing.


> +
> +#if ENABLE(SVG)
> +V8ClassIndex::V8WrapperType V8Custom::DowncastSVGPathSeg(void* path_seg) {
path_seg bad casing.
incorrect { placement.


> +    WebCore::SVGPathSeg *real_path_seg = reinterpret_cast<WebCore::SVGPathSeg*>(path_seg);

* is in the wrong location.
real_path_seg  bad casing.

> +
> +    switch (real_path_seg->pathSegType()) {
> +#define MAKE_CASE(svg_val, v8_val) case WebCore::SVGPathSeg::svg_val: return V8ClassIndex::v8_val;

svg_value,v8_vl bad casing (and abbreviated name).
Avoid ending with ;


MAKE_CASE when used should be indented.

> +#endif  // ENABLE(SVG)
Since space before //

[Nate Chapin]

Created attachment 31101 [details]
patch2

[Nate Chapin]

(In reply to comment #3)
> Created an attachment (id=31101) [review]
> patch2
> 

New patch up.  Not sure I got the indentations exactly right, but it should at least be close this time...

[Adam Barth]

If no one snags this before tonight, I'll review it.

[David Levin]

Comment on attachment 31101 [details]
patch2

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.

[Nate Chapin]

Created attachment 31135 [details]
patch3

Ok, I think I've got everything standardized now.  We'll see...

[David Levin]

Comment on attachment 31135 [details]
patch3

r+

" // Custom implementation is Navigator properties."
As agreed, we'll remove this line.

[David Levin]

Assigned to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/44579