|
|
Descriptionbinding: 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. #
Messages
Total messages: 37 (27 generated)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== binding: Removes Document::wrap that must be equivalent to Node::wrap. BUG= ========== to ========== 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= ==========
yukishiino@chromium.org changed reviewers: + haraken@chromium.org
Could you review this CL?
https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:426: toV8(frame->document(), context->Global(), m_isolate); Just to confirm: If someone calls toV8(document) before updateDocumentProperty calls toV8(document), we will miss setting m_document. Are you assuming that it should not happen? Then we might want to add some assert about the fact. For example, can we add CHECK(!DOMDataStore::containsWrapper(document)) before calling toV8(document)? https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:427: DCHECK(documentWrapper->IsObject()); Shall we call checkDocumentWrapper?
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/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:426: toV8(frame->document(), context->Global(), m_isolate); On 2016/11/25 15:52:30, haraken wrote: > > Just to confirm: If someone calls toV8(document) before updateDocumentProperty > calls toV8(document), we will miss setting m_document. Are you assuming that it > should not happen? Yes, it shouldn't happen for the main world. > Then we might want to add some assert about the fact. For example, can we add > CHECK(!DOMDataStore::containsWrapper(document)) before calling toV8(document)? Done. https://codereview.chromium.org/2525313004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:427: DCHECK(documentWrapper->IsObject()); On 2016/11/25 15:52:30, haraken wrote: > > Shall we call checkDocumentWrapper? It's meaningless, I think? documentWrapper = toV8(document); checkDocumentWrapper(documentWrapper, document); will succeed anyway.
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I needed a change since Patch Set 3. Could you take another look?
LGTM
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480507051868670, "parent_rev": "0483e5faa05129e1ffcc3b3f622b8c2cdfc3fe36", "commit_rev": "11d96cf69d9f3ea7ff675bd2a8e66a66bf53e470"}
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/87dc599743f71d33466ac56a8db5a53c5f6ce46c Cr-Commit-Position: refs/heads/master@{#435226}
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. . |