[Webkit-unassigned] [Bug 26258] Finish upstreaming V8Custom

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26258


levin at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31063|review?                     |review-
               Flag|                            |




------- Comment #2 from levin at chromium.org  2009-06-08 16:05 PDT -------
(From update of attachment 31063)
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 //


-- 
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.