[webkit-reviews] review denied: [Bug 32452] Port of v8 to work with Gtk webkit : [Attachment 109771] Using V8 inside WebKit/Gtk

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Rémi Duraffort
<remi.duraffort at st.com>'s request for review:
Bug 32452: Port of v8 to work with Gtk webkit
https://bugs.webkit.org/show_bug.cgi?id=32452

Attachment 109771: Using V8 inside WebKit/Gtk
https://bugs.webkit.org/attachment.cgi?id=109771&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109771&action=review


This patch is basically fine.  I'll be happy to review it if you break it into
smaller pieces and use a separate bug (each one blocking this one) for each
patch.	There are a number of mistakes in this patch, but nothing that we can't
clear up during review.

> ChangeLog:10
> +	   This work is also copyrighted to Nayan Kumar Konaje
<nayankk at motorola.com>

Does Nayan Kumar Konaje agree to the contributor license as well?

> Source/JavaScriptCore/GNUmakefile.v8.am:13
> +javascriptcore_sources += \

Can this be shared with another GNUmakefile ?  Every time we add a giant list
of all files, we add a maintenance burden on the project.

> Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:31
>  

This blank line should be removed.

> Source/WebCore/dom/CustomEvent.idl:32
> -	   CustomConstructFunction,
> -	   V8CustomConstructor
> +	   CustomConstructFunction

I don't understand this change.

> Source/WebCore/platform/gtk/PlatformBridge.h:27
> +#ifndef PlatformBridge_h
> +#define PlatformBridge_h

There is no such thing as a PlatformBridge anymore.  It's called
PlatformSupport instead.

> Source/WebCore/platform/gtk/PlatformBridge.h:72
> +// V8 bindings use the ARRAYSIZE_UNSAFE macro. This macro was copied
> +// from http://src.chromium.org/viewvc/chrome/trunk/src/base/basictypes.h
> +//
> +// ARRAYSIZE_UNSAFE performs essentially the same calculation as arraysize,
> +// but can be used on anonymous types or types defined inside
> +// functions. It's less safe than arraysize as it accepts some
> +// (although not all) pointers. Therefore, you should use arraysize
> +// whenever possible.
> +//
> +// The expression ARRAYSIZE_UNSAFE(a) is a compile-time constant of type
> +// size_t.
> +//
> +// ARRAYSIZE_UNSAFE catches a few type errors. If you see a compiler error
> +//
> +//	"warning: division by zero in ..."
> +//
> +// when using ARRAYSIZE_UNSAFE, you are (wrongfully) giving it a pointer.
> +// You should only use ARRAYSIZE_UNSAFE on statically allocated arrays.
> +//
> +// The following comments are on the implementation details, and can
> +// be ignored by the users.
> +//
> +// ARRAYSIZE_UNSAFE(arr) works by inspecting sizeof(arr) (the # of bytes in
> +// the array) and sizeof(*(arr)) (the # of bytes in one array
> +// element). If the former is divisible by the latter, perhaps arr is
> +// indeed an array, in which case the division result is the # of
> +// elements in the array. Otherwise, arr cannot possibly be an array,
> +// and we generate a compiler error to prevent the code from
> +// compiling.
> +//
> +// Since the size of bool is implementation-defined, we need to cast
> +// !(sizeof(a) & sizeof(*(a))) to size_t in order to ensure the final
> +// result has type size_t.
> +//
> +// This macro is not perfect as it wrongfully accepts certain
> +// pointers, namely where the pointer size is divisible by the pointee
> +// size. Since all our code has to go through a 32-bit compiler,
> +// where a pointer is 4 bytes, this means all pointers to a type whose
> +// size is 3 or greater than 4 will be (righteously) rejected.

Is this massive comment copy/pasted from somewhere?

> Source/WebCore/platform/gtk/PlatformSupport.h:33
> +typedef PlatformBridge PlatformSupport;

???  Why not just define it as PlatformSupport in the first place?