Bug 33608 - [Android] bindings/v8/NPV8Object.cpp does not compile on Android
Summary: [Android] bindings/v8/NPV8Object.cpp does not compile on Android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 33917
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-13 10:50 PST by Andrei Popescu
Modified: 2010-01-25 03:34 PST (History)
3 users (show)

See Also:


Attachments
Fix bindings/v8/NPV8Object.cpp to compile on Android (5.32 KB, patch)
2010-01-13 11:22 PST, Andrei Popescu
levin: review-
Details | Formatted Diff | Diff
Fix bindings/v8/NPV8Object.cpp to compile on Android (5.29 KB, patch)
2010-01-21 04:06 PST, Andrei Popescu
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Popescu 2010-01-13 10:50:12 PST
bindings/v8/NPV8Object.cpp because

1. it includes "bindings/npruntime.h". This file is actually from the Chromium source tree in third_party/npapi/. On Android, we can live with the standard npruntime.h header in WebCore/bridge/. Ideally, Chromium would move their changes to bridge/npruntime.h and gave up using the non-standard header.
2. it uses ARRAYSIZE_UNSAFE macro, which is defined in the Chromium repository in base/basictypes.h. On Android, we will copy this macro into platform/android/PlatformBridge.h

Patch coming soon.
Comment 1 Andrei Popescu 2010-01-13 11:22:21 PST
Created attachment 46476 [details]
Fix bindings/v8/NPV8Object.cpp to compile on Android
Comment 2 Eric Seidel (no email) 2010-01-14 17:23:45 PST
Seems the ARRAYSIZE_UNSAFE macro should move into WTF at some point.  Ojan filed a bug about that a while back.  Not specifically about that macro, but about the idea of having a macro or template function to do C-style array size calculations.
Comment 3 David Levin 2010-01-20 08:10:19 PST
Comment on attachment 46476 [details]
Fix bindings/v8/NPV8Object.cpp to compile on Android

Pls get rid of ifdef's as discussed.
Comment 4 Andrei Popescu 2010-01-20 12:57:24 PST
I filed a separate bug (33917) for adding the PlatformBridge.h header. Will upload a new patch here, as well.
Comment 5 Andrei Popescu 2010-01-21 04:06:15 PST
Created attachment 47109 [details]
Fix bindings/v8/NPV8Object.cpp to compile on Android
Comment 6 David Levin 2010-01-21 09:49:34 PST
Comment on attachment 47109 [details]
Fix bindings/v8/NPV8Object.cpp to compile on Android

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.
Comment 7 Andrei Popescu 2010-01-25 03:34:03 PST
Thanks for the review David. I addressed all your comments and Steve Block landed the patch in http://trac.webkit.org/changeset/53634