bindings: Introduces type-check for the internal pointers.

bindings: Introduces type-check for the internal pointers.

Makes the type of the internal pointers ScriptWrappableBase* so we can check
pointer conversions between the internal pointers and the native objects.
ScriptWrappableBase* is much safer than void* here because we can avoid
unintentional pointer conversions.

This CL also includes fixes of misconversions from/to the internal pointers.

Note that the definition of the internal pointers does not change by this CL.
We'll change the actual definition at the coming http://crrev.com/420233002 .

BUG=235436
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179915

[Yuki, 2014-08-08 08:56:36]

Could you review this CL?

void* is ... dangerous!  It's really dangerous...

[Jens Widell, 2014-08-08 09:37:07]

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/DOMWrapperMap.h:145: return
reinterpret_cast<KeyType*>(toInternalPointer(data.GetValue()));
Why use reinterpret_cast here? If KeyType is a ScriptWrapperBase sub-class,
static_cast would be the appropriate cast to use.

[haraken, 2014-08-08 09:37:24]

This is a great CL.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8ArrayBufferCustom.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8ArrayBufferCustom.h:65: return
reinterpret_cast<ScriptWrappableBase*>(impl);

Probably I'm wrong, but I guess this could be static_cast instead of
reinterpret_cast. I guess you can replace all reinterpret_casts in this CL with
static_casts, since all DOM objects now inherit from ScriptWrappableBase.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:35: return
fromInternalPointer(blink::toInternalPointer(object));

Do we need blink:: ? The same comment for other parts where you added blink::.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:39: static void
derefObject(ScriptWrappableBase* internalPointer);

Omit |internalPointer|. Blink normally doesn't add variable names in method
declarations. The same comment for other parts in this CL.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:148: return
reinterpret_cast<ScriptWrappableBase*>(static_cast<void*>(impl));

Do we need static_cast<void*> ? The same comment for other parts where you added
static_cast<void*>.

[haraken, 2014-08-08 09:38:03]

Nit: Wrap the CL description to 80 characters or so.

[Jens Widell, 2014-08-08 09:50:13]

On 2014/08/08 09:38:03, haraken wrote:
> Nit: Wrap the CL description to 80 characters or so.

Also spelling: "poitner converions" (end of second paragraph.)

[Yuki, 2014-08-08 13:01:25]

Thanks for the review.

By the way, I think that recently Rietveld's behavior changed so not wrap long
descriptions.  I think it's a bug of Rietveld.  I never wrapped lines in
descriptions before, and it worked.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/DOMWrapperMap.h:145: return
reinterpret_cast<KeyType*>(toInternalPointer(data.GetValue()));
On 2014/08/08 09:37:06, Jens Lindström wrote:
> Why use reinterpret_cast here? If KeyType is a ScriptWrapperBase sub-class,
> static_cast would be the appropriate cast to use.

We're using DOMWrapperMap with KeyType=NPObject, which is defined outside of
blink and does not inherit from ScriptWrappableBase.  So we need
reinterpret_cast here.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8ArrayBufferCustom.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8ArrayBufferCustom.h:65: return
reinterpret_cast<ScriptWrappableBase*>(impl);
On 2014/08/08 09:37:23, haraken wrote:
> 
> Probably I'm wrong, but I guess this could be static_cast instead of
> reinterpret_cast. I guess you can replace all reinterpret_casts in this CL
with
> static_casts, since all DOM objects now inherit from ScriptWrappableBase.

Unfortunately that's not true that all DOM objects inherit from
ScriptWrappableBase.  We still have some exceptions.

At least WTF::ArrayBuffer doesn't inherit from ScriptWrappableBase, because wtf
shouldn't depend on bindings.  And this code is for ArrayBuffer.  We cannot
static_cast from ArrayBuffer to ScriptWrappableBase.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:35: return
fromInternalPointer(blink::toInternalPointer(object));
On 2014/08/08 09:37:23, haraken wrote:
> 
> Do we need blink:: ? The same comment for other parts where you added blink::.

Yes, we need blink:: to solve ambiguity between blink::V8T::toInternalPointer(T*
impl) and blink::toInternalPointer(v8::Handle<v8::Object>).

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:39: static void
derefObject(ScriptWrappableBase* internalPointer);
On 2014/08/08 09:37:23, haraken wrote:
> 
> Omit |internalPointer|. Blink normally doesn't add variable names in method
> declarations. The same comment for other parts in this CL.

I read the style guide at
http://dev.chromium.org/blink/coding-style
especially at [Names] section's 9, which says "Leave meaningless variable names
out of function declarations. A good rule of thumb is if the parameter type name
contains the parameter name (without trailing numbers or pluralization), then
the parameter name isn't needed. Usually, there should be a parameter name for
bools, strings, and numerical types."

I thought that "ScriptWrappableBase" and "internalPointer" differ and it's
helpful to put the name explicitly so that people understand that we need to
pass "internal pointer" here.

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:148: return
reinterpret_cast<ScriptWrappableBase*>(static_cast<void*>(impl));
On 2014/08/08 09:37:23, haraken wrote:
> 
> Do we need static_cast<void*> ? The same comment for other parts where you
added
> static_cast<void*>.

The other parts really need this trick because there are cases that T doesn't
inherit from ScriptWrappableBase.

This part is a special exception that I would like to keep the existing behavior
as much as possible in this CL.  I'm planning to change the behavior at
http://crrev.com/420233002 .

We have been using pointers to most-base DOM class as the internal pointers, not
poitners to ScriptWrappable or ScriptWrappableBase.  For example, pointer to
EventTarget as the internal pointer of Node.  This cast returns a pointer to
EventTarget as the internal pointer of Node as same as before.

[Yuki, 2014-08-08 13:07:00]

I agree that "reinterpret_cast" is a useful signal to detect that we're doing
something in a bad manner.  I'll revisit these reinterpret_casts later and I'd
like to remove them as much as possible.

[haraken, 2014-08-08 13:14:30]

> I agree that "reinterpret_cast" is a useful signal to detect that we're doing
> something in a bad manner.  I'll revisit these reinterpret_casts later and I'd
> like to remove them as much as possible.

Agreed.

Is the patch set 3 ready for review?

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:39: static void
derefObject(ScriptWrappableBase* internalPointer);
On 2014/08/08 13:01:25, Yuki wrote:
> On 2014/08/08 09:37:23, haraken wrote:
> > 
> > Omit |internalPointer|. Blink normally doesn't add variable names in method
> > declarations. The same comment for other parts in this CL.
> 
> I read the style guide at
> http://dev.chromium.org/blink/coding-style
> especially at [Names] section's 9, which says "Leave meaningless variable
names
> out of function declarations. A good rule of thumb is if the parameter type
name
> contains the parameter name (without trailing numbers or pluralization), then
> the parameter name isn't needed. Usually, there should be a parameter name for
> bools, strings, and numerical types."
> 
> I thought that "ScriptWrappableBase" and "internalPointer" differ and it's
> helpful to put the name explicitly so that people understand that we need to
> pass "internal pointer" here.

I think the issue is we're using a word "internalPointer" although it's actually
a ScriptWrapperBase's pointer. It should be named "wrappableBase" or something
like that. Then we can remove the variable name.

You can do this in a separate CL.

[Yuki, 2014-08-08 13:21:20]

On 2014/08/08 13:14:30, haraken wrote:
> > I agree that "reinterpret_cast" is a useful signal to detect that we're
doing
> > something in a bad manner.  I'll revisit these reinterpret_casts later and
I'd
> > like to remove them as much as possible.
> 
> Agreed.
> 
> Is the patch set 3 ready for review?

Yes, please.


>
https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
> File Source/bindings/templates/interface.h (right):
> 
>
https://codereview.chromium.org/456683002/diff/20001/Source/bindings/template...
> Source/bindings/templates/interface.h:39: static void
> derefObject(ScriptWrappableBase* internalPointer);
> On 2014/08/08 13:01:25, Yuki wrote:
> > On 2014/08/08 09:37:23, haraken wrote:
> > > 
> > > Omit |internalPointer|. Blink normally doesn't add variable names in
method
> > > declarations. The same comment for other parts in this CL.
> > 
> > I read the style guide at
> > http://dev.chromium.org/blink/coding-style
> > especially at [Names] section's 9, which says "Leave meaningless variable
> names
> > out of function declarations. A good rule of thumb is if the parameter type
> name
> > contains the parameter name (without trailing numbers or pluralization),
then
> > the parameter name isn't needed. Usually, there should be a parameter name
for
> > bools, strings, and numerical types."
> > 
> > I thought that "ScriptWrappableBase" and "internalPointer" differ and it's
> > helpful to put the name explicitly so that people understand that we need to
> > pass "internal pointer" here.
> 
> I think the issue is we're using a word "internalPointer" although it's
actually
> a ScriptWrapperBase's pointer. It should be named "wrappableBase" or something
> like that. Then we can remove the variable name.
> 
> You can do this in a separate CL.

I see.  Will do in a separate CL.

[haraken, 2014-08-08 13:27:28]

Looks like the patch set 3 doesn't compile on try bots... :)

[Yuki, 2014-08-08 15:15:08]

On 2014/08/08 13:27:28, haraken wrote:
> Looks like the patch set 3 doesn't compile on try bots... :)

I found a few more misconversions to the internal pointers.  I think now it's
all fixed (at least hope so).

[haraken, 2014-08-08 15:50:31]

The code change looks OK. However, I want to add some verification to verify
that internal pointers are used correctly.

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/CustomElementWrapper.cpp (right):

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/CustomElementWrapper.cpp:104: v8::Handle<v8::Object>
wrapper = V8DOMWrapper::createWrapper(creationContext, binding->wrapperType(),
WrapperType::toInternalPointer(element.get()), isolate);

What happens if someone passes element.get() to createWrapper or setNativeInfo
by mistake? I think we should have a way to detect the error. Would it be
possible to add ASSERTs to verify the following facts?

- Pointers stored into an internal field are internal pointers.
- Pointers retrieved from an internal field are internal pointers.

To verify the facts, probably you'll need to add a dummy member to something so
that the internal pointer and the head of the object have different addresses.
Adding such a dummy member is fine in Debug builds.

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1151: return hasInstance(value, isolate)
?
fromInternalPointer(blink::toInternalPointer(v8::Handle<v8::Object>::Cast(value)))
: 0;

fromInternalPointer(toInternalPointer(value)) is a bit confusing.

Probably we can:

- Rename toInternalPointer to toScriptWrappableBase
- Rename fromInternalPointer to toImpl
- Remove toNative
- Rename toNativeWithTypeCheck to toImplWithTypeCheck

You can do the refactoring in a separate CL.

[Yuki, 2014-08-11 05:42:00]

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/CustomElementWrapper.cpp (right):

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/CustomElementWrapper.cpp:104: v8::Handle<v8::Object>
wrapper = V8DOMWrapper::createWrapper(creationContext, binding->wrapperType(),
WrapperType::toInternalPointer(element.get()), isolate);
On 2014/08/08 15:50:31, haraken wrote:
> 
> What happens if someone passes element.get() to createWrapper or setNativeInfo
> by mistake? I think we should have a way to detect the error. Would it be
> possible to add ASSERTs to verify the following facts?
> 
> - Pointers stored into an internal field are internal pointers.
> - Pointers retrieved from an internal field are internal pointers.
> 
> To verify the facts, probably you'll need to add a dummy member to something
so
> that the internal pointer and the head of the object have different addresses.
> Adding such a dummy member is fine in Debug builds.

Your idea sounds very nice and I'll add that check after
http://crrev.com/420233002 will be landed.

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface.cpp:1151: return hasInstance(value, isolate)
?
fromInternalPointer(blink::toInternalPointer(v8::Handle<v8::Object>::Cast(value)))
: 0;
On 2014/08/08 15:50:31, haraken wrote:
> 
> fromInternalPointer(toInternalPointer(value)) is a bit confusing.
> 
> Probably we can:
> 
> - Rename toInternalPointer to toScriptWrappableBase
> - Rename fromInternalPointer to toImpl
> - Remove toNative
> - Rename toNativeWithTypeCheck to toImplWithTypeCheck
> 
> You can do the refactoring in a separate CL.

I see.  Let's choose the best name with an offline discussion, and then, I'll
rename it.

[haraken, 2014-08-11 05:44:24]

On 2014/08/11 05:42:00, Yuki wrote:
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/CustomElementWrapper.cpp (right):
> 
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
> Source/bindings/core/v8/CustomElementWrapper.cpp:104: v8::Handle<v8::Object>
> wrapper = V8DOMWrapper::createWrapper(creationContext, binding->wrapperType(),
> WrapperType::toInternalPointer(element.get()), isolate);
> On 2014/08/08 15:50:31, haraken wrote:
> > 
> > What happens if someone passes element.get() to createWrapper or
setNativeInfo
> > by mistake? I think we should have a way to detect the error. Would it be
> > possible to add ASSERTs to verify the following facts?
> > 
> > - Pointers stored into an internal field are internal pointers.
> > - Pointers retrieved from an internal field are internal pointers.
> > 
> > To verify the facts, probably you'll need to add a dummy member to something
> so
> > that the internal pointer and the head of the object have different
addresses.
> > Adding such a dummy member is fine in Debug builds.
> 
> Your idea sounds very nice and I'll add that check after
> http://crrev.com/420233002 will be landed.
> 
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
> File Source/bindings/templates/interface.cpp (right):
> 
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
> Source/bindings/templates/interface.cpp:1151: return hasInstance(value,
isolate)
> ?
>
fromInternalPointer(blink::toInternalPointer(v8::Handle<v8::Object>::Cast(value)))
> : 0;
> On 2014/08/08 15:50:31, haraken wrote:
> > 
> > fromInternalPointer(toInternalPointer(value)) is a bit confusing.
> > 
> > Probably we can:
> > 
> > - Rename toInternalPointer to toScriptWrappableBase
> > - Rename fromInternalPointer to toImpl
> > - Remove toNative
> > - Rename toNativeWithTypeCheck to toImplWithTypeCheck
> > 
> > You can do the refactoring in a separate CL.
> 
> I see.  Let's choose the best name with an offline discussion, and then, I'll
> rename it.

ok, LGTM. So your plan is:

(1) Land this CL.
(2) Land http://crrev.com/420233002
(3) Add the verification about pointer values.
(4) Rename toInternalPointer/fromInternalPointer/toNative.

Am I understanding correctly?

[Yuki, 2014-08-11 05:46:29]

On 2014/08/11 05:44:24, haraken wrote:
> On 2014/08/11 05:42:00, Yuki wrote:
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
> > File Source/bindings/core/v8/CustomElementWrapper.cpp (right):
> > 
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
> > Source/bindings/core/v8/CustomElementWrapper.cpp:104: v8::Handle<v8::Object>
> > wrapper = V8DOMWrapper::createWrapper(creationContext,
binding->wrapperType(),
> > WrapperType::toInternalPointer(element.get()), isolate);
> > On 2014/08/08 15:50:31, haraken wrote:
> > > 
> > > What happens if someone passes element.get() to createWrapper or
> setNativeInfo
> > > by mistake? I think we should have a way to detect the error. Would it be
> > > possible to add ASSERTs to verify the following facts?
> > > 
> > > - Pointers stored into an internal field are internal pointers.
> > > - Pointers retrieved from an internal field are internal pointers.
> > > 
> > > To verify the facts, probably you'll need to add a dummy member to
something
> > so
> > > that the internal pointer and the head of the object have different
> addresses.
> > > Adding such a dummy member is fine in Debug builds.
> > 
> > Your idea sounds very nice and I'll add that check after
> > http://crrev.com/420233002 will be landed.
> > 
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
> > File Source/bindings/templates/interface.cpp (right):
> > 
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
> > Source/bindings/templates/interface.cpp:1151: return hasInstance(value,
> isolate)
> > ?
> >
>
fromInternalPointer(blink::toInternalPointer(v8::Handle<v8::Object>::Cast(value)))
> > : 0;
> > On 2014/08/08 15:50:31, haraken wrote:
> > > 
> > > fromInternalPointer(toInternalPointer(value)) is a bit confusing.
> > > 
> > > Probably we can:
> > > 
> > > - Rename toInternalPointer to toScriptWrappableBase
> > > - Rename fromInternalPointer to toImpl
> > > - Remove toNative
> > > - Rename toNativeWithTypeCheck to toImplWithTypeCheck
> > > 
> > > You can do the refactoring in a separate CL.
> > 
> > I see.  Let's choose the best name with an offline discussion, and then,
I'll
> > rename it.
> 
> ok, LGTM. So your plan is:
> 
> (1) Land this CL.
> (2) Land http://crrev.com/420233002
> (3) Add the verification about pointer values.
> (4) Rename toInternalPointer/fromInternalPointer/toNative.
> 
> Am I understanding correctly?

Yes, you're right.

[Yuki, 2014-08-11 05:50:08]

On 2014/08/11 05:46:29, Yuki wrote:
> On 2014/08/11 05:44:24, haraken wrote:
> > On 2014/08/11 05:42:00, Yuki wrote:
> > >
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
> > > File Source/bindings/core/v8/CustomElementWrapper.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/core/v8/...
> > > Source/bindings/core/v8/CustomElementWrapper.cpp:104:
v8::Handle<v8::Object>
> > > wrapper = V8DOMWrapper::createWrapper(creationContext,
> binding->wrapperType(),
> > > WrapperType::toInternalPointer(element.get()), isolate);
> > > On 2014/08/08 15:50:31, haraken wrote:
> > > > 
> > > > What happens if someone passes element.get() to createWrapper or
> > setNativeInfo
> > > > by mistake? I think we should have a way to detect the error. Would it
be
> > > > possible to add ASSERTs to verify the following facts?
> > > > 
> > > > - Pointers stored into an internal field are internal pointers.
> > > > - Pointers retrieved from an internal field are internal pointers.
> > > > 
> > > > To verify the facts, probably you'll need to add a dummy member to
> something
> > > so
> > > > that the internal pointer and the head of the object have different
> > addresses.
> > > > Adding such a dummy member is fine in Debug builds.
> > > 
> > > Your idea sounds very nice and I'll add that check after
> > > http://crrev.com/420233002 will be landed.
> > > 
> > >
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
> > > File Source/bindings/templates/interface.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/456683002/diff/80001/Source/bindings/template...
> > > Source/bindings/templates/interface.cpp:1151: return hasInstance(value,
> > isolate)
> > > ?
> > >
> >
>
fromInternalPointer(blink::toInternalPointer(v8::Handle<v8::Object>::Cast(value)))
> > > : 0;
> > > On 2014/08/08 15:50:31, haraken wrote:
> > > > 
> > > > fromInternalPointer(toInternalPointer(value)) is a bit confusing.
> > > > 
> > > > Probably we can:
> > > > 
> > > > - Rename toInternalPointer to toScriptWrappableBase
> > > > - Rename fromInternalPointer to toImpl
> > > > - Remove toNative
> > > > - Rename toNativeWithTypeCheck to toImplWithTypeCheck
> > > > 
> > > > You can do the refactoring in a separate CL.
> > > 
> > > I see.  Let's choose the best name with an offline discussion, and then,
> I'll
> > > rename it.
> > 
> > ok, LGTM. So your plan is:
> > 
> > (1) Land this CL.
> > (2) Land http://crrev.com/420233002
> > (3) Add the verification about pointer values.
> > (4) Rename toInternalPointer/fromInternalPointer/toNative.
> > 
> > Am I understanding correctly?
> 
> Yes, you're right.

This CL blocks my other work, so I'm going to commit this CL.

Jens (jl@), please feel free to send me your comments if any.  I'll address your
comments in a separate CL.

[Yuki, 2014-08-11 05:50:25]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-08-11 05:51:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/456683002/80001

[commit-bot: I haz the power, 2014-08-11 05:52:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-11 05:52:04]

Failed to apply patch for
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
  Hunk #1 FAILED at 64.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp.rej

Patch:       Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
Index: Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
diff --git a/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
b/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
index
12577dc41c98754c67f10ab118e6293d94f21a1c..1860116c73243dcc06dbac591fd69c6d8d651a03
100644
--- a/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
+++ b/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
@@ -64,9 +64,9 @@ static v8::Local<v8::Object>
createInjectedScriptHostV8Wrapper(InjectedScriptHos
         return v8::Local<v8::Object>();
     }
 #if ENABLE(OILPAN)
-    V8DOMWrapper::setNativeInfoWithPersistentHandle(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo, host, new
Persistent<InjectedScriptHost>(host));
+    V8DOMWrapper::setNativeInfoWithPersistentHandle(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo,
V8InjectedScriptHost::toInternalPointer(host), new
Persistent<InjectedScriptHost>(host));
 #else
-    V8DOMWrapper::setNativeInfo(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo, host);
+    V8DOMWrapper::setNativeInfo(instanceTemplate,
&V8InjectedScriptHost::wrapperTypeInfo,
V8InjectedScriptHost::toInternalPointer(host));
 #endif
     // Create a weak reference to the v8 wrapper of InspectorBackend to deref
     // InspectorBackend when the wrapper is garbage collected.

[Jens Widell, 2014-08-11 05:55:52]

On 2014/08/11 05:50:08, Yuki wrote:
> Jens (jl@), please feel free to send me your comments if any.  I'll address
your
> comments in a separate CL.

My concerns were covered (in greater detail) by haraken's comments (and answered
by you); I have nothing to add.

Getting rid of void* is great! Getting rid of the new reinterpret_cast:s would
be great too, but I'm sure we'll continue improving this.

[Yuki, 2014-08-11 06:22:41]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-08-11 06:23:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/456683002/100001

[commit-bot: I haz the power, 2014-08-11 07:32:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-11 08:00:24]

Change committed as 179915