Remove ScriptWrappable::FromNode

Remove ScriptWrappable::FromNode

This CL removes ScriptWrappable::FromNode.

BUG=682322
Review-Url: https://codereview.chromium.org/2808403007
Cr-Commit-Position: refs/heads/master@{#467021}
Committed: https://chromium.googlesource.com/chromium/src/+/87356e8607d5c60651e22ab2ed406cfc99b512b5

[adithyas, 2017-04-13 14:25:18]

adithyas@chromium.org changed reviewers:
+ jbroman@chromium.org

[adithyas, 2017-04-13 14:25:19]

I'm not sure if Node::ToScriptWrappable is even needed anymore, I think we could
just use static_cast<ScriptWrappable*>?

[adithyas, 2017-04-13 14:26:20]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 14:26:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-04-13 14:34:25]

On 2017/04/13 14:25:19, adithyas wrote:
> I'm not sure if Node::ToScriptWrappable is even needed anymore, I think we
could
> just use static_cast<ScriptWrappable*>?

Yeah, agreed. What happens if we simply remove Node::ToScriptWrappable?

+yukishiino introduced the FromNode trick and might know the background.

[haraken, 2017-04-13 14:34:44]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[jbroman, 2017-04-13 14:36:07]

On 2017/04/13 at 14:25:19, adithyas wrote:
> I'm not sure if Node::ToScriptWrappable is even needed anymore, I think we
could just use static_cast<ScriptWrappable*>?

This is used so that it can be called in source files without including Node.h
(because the static_cast requires having the definition of Node). If that is no
longer an issue (might we worth a quick check to see how many files include
Node.h with this CL that didn't before), then sure, a cast is fine.

[commit-bot: I haz the power, 2017-04-13 16:33:04]

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

[commit-bot: I haz the power, 2017-04-13 16:33:05]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-04-14 07:21:43]

I vaguely remember that #include "Node.h" caused a cycle before and we didn't
want to include "Node.h".

However, the bots seem fine with #include "Node.h" now.

Plus, I'm now working to remove hacks around ToV8(Node*) entirely.  Hopefully,
it'll make the situation much better.

static_cast<ScriptWrappable*> makes a lot sense.

[adithyas, 2017-04-18 17:01:02]

Description was changed from

==========
Move ScriptWrappable::FromNode

This CL moves ScriptWrappable::FromNode to Node::ToScriptWrappable.

BUG=682322
==========

to

==========
Remove ScriptWrappable::FromNode

This CL removes ScriptWrappable::FromNode.

BUG=682322
==========

[adithyas, 2017-04-18 17:04:22]

Looks like yukishiino@'s CL removes the need of having special Node* versions of
v8SetReturnValue, so we don't need FromNode at all anymore. I've removed
FromNode and the special Node* functions from V8Binding (and updated the CL
title).

[jbroman, 2017-04-18 17:11:24]

On 2017/04/18 at 17:04:22, adithyas wrote:
> Looks like yukishiino@'s CL removes the need of having special Node* versions
of v8SetReturnValue, so we don't need FromNode at all anymore. I've removed
FromNode and the special Node* functions from V8Binding (and updated the CL
title).

Mind linking which CL you're referring to?

[adithyas, 2017-04-18 17:18:16]

On 2017/04/18 at 17:11:24, jbroman wrote:
> On 2017/04/18 at 17:04:22, adithyas wrote:
> > Looks like yukishiino@'s CL removes the need of having special Node*
versions of v8SetReturnValue, so we don't need FromNode at all anymore. I've
removed FromNode and the special Node* functions from V8Binding (and updated the
CL title).
> 
> Mind linking which CL you're referring to?

Oops: https://codereview.chromium.org/2693893007

[jbroman, 2017-04-18 17:22:35]

thanks; lgtm if yukishiino agrees

[Yuki, 2017-04-19 07:32:49]

LGTM with an important note.

Note that https://crrev.com/2693893007 is the second attempt, and the first
attempt was reverted due to crash issues.  I'm really afraid that it's possible
that we need to revert the CL of the second attempt if something goes wrong. 
The change in the CL is treating something delicate and/or fragile.

So, LGTM, but remember that I might revert your CL together with mine if we hit
something.

It could be an option to wait for a while before landing this CL, or to go with
PS1 for the time being.  I'm fine with either way.

[Yuki, 2017-04-19 07:38:03]

Just FYI,

We already hit a crash issue at https:://crbug.com/712638 , and I'm now working
to fix it.  At this moment, I'm not planning to revert my CL, but I cannot
promise anything.

[adithyas, 2017-04-25 14:58:12]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-04-25 14:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2808403007/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-25 14:58:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 17:07:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493132292908320,
"parent_rev": "129a77cdf7235f8fa7652366e687adb52c0ba1db", "commit_rev":
"87356e8607d5c60651e22ab2ed406cfc99b512b5"}

[commit-bot: I haz the power, 2017-04-25 17:07:31]

Description was changed from

==========
Remove ScriptWrappable::FromNode

This CL removes ScriptWrappable::FromNode.

BUG=682322
==========

to

==========
Remove ScriptWrappable::FromNode

This CL removes ScriptWrappable::FromNode.

BUG=682322

Review-Url: https://codereview.chromium.org/2808403007
Cr-Commit-Position: refs/heads/master@{#467021}
Committed:
https://chromium.googlesource.com/chromium/src/+/87356e8607d5c60651e22ab2ed40...
==========

[commit-bot: I haz the power, 2017-04-25 17:07:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/87356e8607d5c60651e22ab2ed40...