[webkit-reviews] review granted: [Bug 33608] [Android] bindings/v8/NPV8Object.cpp does not compile on Android : [Attachment 47109] Fix bindings/v8/NPV8Object.cpp to compile on Android
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 21 09:49:34 PST 2010
David Levin <levin at chromium.org> has granted Andrei Popescu
<andreip at google.com>'s request for review:
Bug 33608: [Android] bindings/v8/NPV8Object.cpp does not compile on Android
https://bugs.webkit.org/show_bug.cgi?id=33608
Attachment 47109: Fix bindings/v8/NPV8Object.cpp to compile on Android
https://bugs.webkit.org/attachment.cgi?id=47109&action=review
------- Additional Comments from David Levin <levin at chromium.org>
Just a few nits to address below and submit away.
> Index: WebCore/bindings/v8/NPV8Object.h
> +#if PLATFORM(CHROMIUM)
> +// Chromium uses a different npruntime.h, which is in
> +// the Chromium source repository under third_party/npapi/bindings.
> +// The Google-specific changes in that file should probably be
Should this say "FIXME: " right before it.
> +// moved into bridge/npruntime.h, guarded by an #if PlATFORM(CHROMIUM).
> #include "bindings/npruntime.h"
> +#else
> +#include "bridge/npruntime.h" // use WebCore version for Android and other
ports.
Please capitalize the u (s/use/Use/) and only have a single space before end of
line comments.
> Index: WebCore/platform/android/PlatformBridge.h
Add 2010 to the copyright line?
> +// 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
Only use single spaces after periods in comments for WebKit code.
> +// (although not all) pointers. Therefore, you should use arraysize
Single space after period.
> +// ARRAYSIZE_UNSAFE catches a few type errors. If you see a compiler error
Single space after period.
> +// the array) and sizeof(*(arr)) (the # of bytes in one array
> +// element). If the former is divisible by the latter, perhaps arr is
Single space after period.
> +// indeed an array, in which case the division result is the # of
> +// elements in the array. Otherwise, arr cannot possibly be an array,
Single space after period.
> +// and we generate a compiler error to prevent the code from
> +// compiling.
> +// pointers, namely where the pointer size is divisible by the pointee
> +// size. Since all our code has to go through a 32-bit compiler,
Single space after period.
More information about the webkit-reviews
mailing list