Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2941783002:
Dispatch unload event for swapped-out subframes. (Closed)
|
Created:
3 years, 6 months ago by Łukasz Anforowicz Modified:
3 years, 6 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, nasko+codewatch_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid unnecessary direct dispatching of unload events from RFI::OnSwapOut.
There is no need to call DispatchUnloadEvent directly from
RenderFrameImpl::OnSwapOut, because the unload events will
be dispatched even without this call - this will happen via
the following call chain:
- Document::DispatchUnloadEvents
- FrameLoader::DispatchUnloadEvent
- LocalFrame::Detach
- WebFrame::Swap
- RenderFrameImpl::OnSwapOut
This also means that the old TODO from the code doesn't need fixing - we
already dispatch unload events for swapped out subframes.
BUG=733314
TEST=Manualy set main frame's window.onunload and verified it still fires.
Review-Url: https://codereview.chromium.org/2941783002
Cr-Commit-Position: refs/heads/master@{#479538}
Committed: https://chromium.googlesource.com/chromium/src/+/6e8a61ba16f529f02841fb75a50f15ec45726411
Patch Set 1 #Patch Set 2 : Does main frame need special handling at all? #Messages
Total messages: 21 (16 generated)
The CQ bit was checked by lukasza@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@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...
Description was changed from ========== Dispatch unload event for swapped-out subframes. BUG=357782 ========== to ========== Avoid unnecessary dispatching of unload events. BUG=357782 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid unnecessary dispatching of unload events. BUG=357782 ========== to ========== Avoid unnecessary dispatching of unload events. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=357782 TEST=Manualy set main frame's window.onunload and verified it still fires. ==========
Description was changed from ========== Avoid unnecessary dispatching of unload events. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=357782 TEST=Manualy set main frame's window.onunload and verified it still fires. ========== to ========== Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=357782 TEST=Manualy set main frame's window.onunload and verified it still fires. ==========
Description was changed from ========== Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=357782 TEST=Manualy set main frame's window.onunload and verified it still fires. ========== to ========== Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=733314 TEST=Manualy set main frame's window.onunload and verified it still fires. ==========
lukasza@chromium.org changed reviewers: + creis@chromium.org
creis@, can you PTAL?
Great! Thanks for verifying. LGTM.
The CQ bit was checked by lukasza@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": 20001, "attempt_start_ts": 1497481780619030,
"parent_rev": "d6381a81f63b93a0bec890887e129c1a27f6701e", "commit_rev":
"6e8a61ba16f529f02841fb75a50f15ec45726411"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=733314 TEST=Manualy set main frame's window.onunload and verified it still fires. ========== to ========== Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut. There is no need to call DispatchUnloadEvent directly from RenderFrameImpl::OnSwapOut, because the unload events will be dispatched even without this call - this will happen via the following call chain: - Document::DispatchUnloadEvents - FrameLoader::DispatchUnloadEvent - LocalFrame::Detach - WebFrame::Swap - RenderFrameImpl::OnSwapOut This also means that the old TODO from the code doesn't need fixing - we already dispatch unload events for swapped out subframes. BUG=733314 TEST=Manualy set main frame's window.onunload and verified it still fires. Review-Url: https://codereview.chromium.org/2941783002 Cr-Commit-Position: refs/heads/master@{#479538} Committed: https://chromium.googlesource.com/chromium/src/+/6e8a61ba16f529f02841fb75a50f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/6e8a61ba16f529f02841fb75a50f...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2938423002/ by lukasza@chromium.org. The reason for reverting is: This CL is responsible for https://crbug.com/733940. |
