Remove ToEventTarget

Remove ToEventTarget

https://crrev.com/2500363002 makes the special handling of DOMWindow in toEventTarget unnecessary. We can now inline V8EventTarget::toImplWithTypeCheck instead of using toEventTarget.

BUG=

Review-Url: https://codereview.chromium.org/2697543003
Cr-Commit-Position: refs/heads/master@{#450401}
Committed: https://chromium.googlesource.com/chromium/src/+/86cf3df9dfe584e7663daa42017ca7629459e740

[adithyas, 2017-02-13 15:25:03]

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

[commit-bot: I haz the power, 2017-02-13 15:25:24]

Dry run: 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-02-13 17:06:02]

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

[commit-bot: I haz the power, 2017-02-13 17:06:03]

Dry run: This issue passed the CQ dry run.

[adithyas, 2017-02-13 18:14:23]

Description was changed from

==========
Remove window check in ToEventTarget

BUG=
==========

to

==========
Remove ToEventTarget

https://crrev.com/2500363002 makes the special handling of DOMWindow in
toEventTarget unnecessary. We can now inline V8EventTarget::toImplWithTypeCheck
instead of using toEventTarget.

BUG=
==========

[adithyas, 2017-02-13 18:17:30]

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

[commit-bot: I haz the power, 2017-02-13 18:18:14]

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

[adithyas, 2017-02-13 18:25:46]

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

[jbroman, 2017-02-13 19:20:21]

lgtm, but wait for yukishiino to confirm.

IIUC, this works because WindowProxy::setupWindowPrototypeChain makes sure that
the ScriptWrappable internal fields always match the actual DOMWindow wrapper
object (the global object, as opposed to the global proxy).

If this is okay, you can probably also replace toDOMWindow(v8::Isolate*,
v8::Local<v8::Value>) with V8Window::toImplWithTypeCheck.

[jbroman, 2017-02-13 19:27:19]

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForCore.cpp
(right):

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForCore.cpp:232: bool
DictionaryHelper::get(const Dictionary& dictionary,
This can be deleted entirely, because you've just changed this to exactly match
the default implementation, here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8DocumentCustom.cpp
(right):

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8DocumentCustom.cpp:121: }
else if (!V8EventTarget::toImplWithTypeCheck(info.GetIsolate(), v8Target)) {
nit: this can be V8EventTarget::hasInstance(info.GetIsolate(), v8Target). It's
both clearer, and it skips extracting the impl pointer, since this call site
only cares about the nullness.

[adithyas, 2017-02-13 19:57:39]

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForCore.cpp
(right):

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForCore.cpp:232: bool
DictionaryHelper::get(const Dictionary& dictionary,
On 2017/02/13 at 19:27:19, jbroman wrote:
> This can be deleted entirely, because you've just changed this to exactly
match the default implementation, here:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

Removed, thanks!

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8DocumentCustom.cpp
(right):

https://codereview.chromium.org/2697543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8DocumentCustom.cpp:121: }
else if (!V8EventTarget::toImplWithTypeCheck(info.GetIsolate(), v8Target)) {
On 2017/02/13 at 19:27:19, jbroman wrote:
> nit: this can be V8EventTarget::hasInstance(info.GetIsolate(), v8Target). It's
both clearer, and it skips extracting the impl pointer, since this call site
only cares about the nullness.

Fixed.

[haraken, 2017-02-13 23:32:39]

LGTM

[Yuki, 2017-02-14 02:44:22]

LGTM.

jbroman> If this is okay, you can probably also replace
toDOMWindow(v8::Isolate*, v8::Local<v8::Value>) with
V8Window::toImplWithTypeCheck.

Note that toDOMWindow looks for a window instance through the prototype chain
while V8Window::toImplWithTypeCheck does not.  So, there is a difference.

However, I agree that we may be able to replace it, because the spec doesn't
require to look through the prototype chain (for major cases at least).  I
didn't check all the call sites, but I guess it should be okay and more correct.

    var obj = {};
    obj.__proto__ = window;
    domObj.foo.call(obj, ...);

In general, this should throw a TypeError because |obj| is not a platform
object.

[jbroman, 2017-02-14 04:06:41]

On 2017/02/14 at 02:44:22, yukishiino wrote:
> LGTM.
> 
> jbroman> If this is okay, you can probably also replace
toDOMWindow(v8::Isolate*, v8::Local<v8::Value>) with
V8Window::toImplWithTypeCheck.
> 
> Note that toDOMWindow looks for a window instance through the prototype chain
while V8Window::toImplWithTypeCheck does not.  So, there is a difference.
> 
> However, I agree that we may be able to replace it, because the spec doesn't
require to look through the prototype chain (for major cases at least).  I
didn't check all the call sites, but I guess it should be okay and more correct.
> 
>     var obj = {};
>     obj.__proto__ = window;
>     domObj.foo.call(obj, ...);
> 
> In general, this should throw a TypeError because |obj| is not a platform
object.

Yeah. My impression was that these were a result of the window proxy and window
object being separate, and so are attempts to find the window object (to extract
the impl). If so, IIUC that's no longer necessary. I really hope that web
content hasn't begun to expect that Object.create(window) can be passed to such
methods. :/

[adithyas, 2017-02-14 15:49:08]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-02-14 15:49:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2697543003/#ps40001
(title: "Code review changes")

[commit-bot: I haz the power, 2017-02-14 15:49:37]

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-02-14 17:48:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487087348669750,
"parent_rev": "867223fdd3327e622cb42836fa1ccc2d66465fec", "commit_rev":
"86cf3df9dfe584e7663daa42017ca7629459e740"}

[commit-bot: I haz the power, 2017-02-14 17:49:14]

Description was changed from

==========
Remove ToEventTarget

https://crrev.com/2500363002 makes the special handling of DOMWindow in
toEventTarget unnecessary. We can now inline V8EventTarget::toImplWithTypeCheck
instead of using toEventTarget.

BUG=
==========

to

==========
Remove ToEventTarget

https://crrev.com/2500363002 makes the special handling of DOMWindow in
toEventTarget unnecessary. We can now inline V8EventTarget::toImplWithTypeCheck
instead of using toEventTarget.

BUG=

Review-Url: https://codereview.chromium.org/2697543003
Cr-Commit-Position: refs/heads/master@{#450401}
Committed:
https://chromium.googlesource.com/chromium/src/+/86cf3df9dfe584e7663daa42017c...
==========

[commit-bot: I haz the power, 2017-02-14 17:49:15]

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