Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 2525313004: binding: Removes Document::wrap that must be equivalent to Node::wrap. (Closed)

Created:
4 years ago by Yuki
Modified:
4 years ago
Reviewers:
haraken
CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

binding: Removes Document::wrap that must be equivalent to Node::wrap. Removes unnecessary Document::wrap and associateWithWrapper. The special code in these functions were made in order to keep WindowProxy::m_document updated and consistent with toV8(document). But we no longer rely on the hack, and we can remove the dead code. BUG= Committed: https://crrev.com/87dc599743f71d33466ac56a8db5a53c5f6ce46c Cr-Commit-Position: refs/heads/master@{#435226}

Patch Set 1 #

Patch Set 2 : . #

Total comments: 4

Patch Set 3 : Addressed a review comment. #

Patch Set 4 : Removed a wrong CHECK. #

Patch Set 5 : Synced. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -76 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/WindowProxy.h View 1 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp View 1 2 3 6 chunks +30 lines, -37 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 2 3 4 1 chunk +0 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 1 chunk +0 lines, -32 lines 0 comments Download

Messages

Total messages: 37 (27 generated)
Yuki
Could you review this CL?
4 years ago (2016-11-25 14:18:38 UTC) #11
haraken
https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp#newcode426 third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:426: toV8(frame->document(), context->Global(), m_isolate); Just to confirm: If someone calls ...
4 years ago (2016-11-25 15:52:30 UTC) #12
Yuki
I think we can make WindowProxy::updateDocument WindowProxy::updateDocumentProperty be private members in a follow-up CL. https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp ...
4 years ago (2016-11-28 06:35:52 UTC) #13
haraken
LGTM
4 years ago (2016-11-28 07:16:26 UTC) #17
Yuki
I needed a change since Patch Set 3. Could you take another look?
4 years ago (2016-11-30 11:45:32 UTC) #28
haraken
LGTM
4 years ago (2016-11-30 11:56:54 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2525313004/100001
4 years ago (2016-11-30 11:57:51 UTC) #31
commit-bot: I haz the power
Committed patchset #5 (id:100001)
4 years ago (2016-11-30 12:01:27 UTC) #34
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/87dc599743f71d33466ac56a8db5a53c5f6ce46c Cr-Commit-Position: refs/heads/master@{#435226}
4 years ago (2016-11-30 12:04:03 UTC) #36
Yuki
4 years ago (2016-12-01 06:27:52 UTC) #37
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2538403002/ by yukishiino@chromium.org.

The reason for reverting is: CHECK(!frame->document()->containsWrapper()); in
WindowProxy::updateDocumentProperty() is causing crashes at several places.
.

Powered by Google App Engine
This is Rietveld 408576698