Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 88503002: Have the unload event execute in background on cross-site navigations (Closed)

Created:
5 years, 8 months ago by clamy
Modified:
5 years, 6 months ago
Reviewers:
pasko, Charlie Reis, jam, nasko, ppi
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, ojan
Visibility:
Public.

Description

Have the unload event execute in background on cross-site navigations Cross-site navigations trigger renderer swap. This CL makes it so that the swap does not need for the old renderer unload event to be fired. Instead, it will be executed in the background, and the new renderer will be swapped in immediately. BUG=323528 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249676

Patch Set 1 #

Total comments: 2

Patch Set 2 : Keeping the old rvh alive for up to 1s #

Total comments: 10

Patch Set 3 : Removed SwapOutAck #

Patch Set 4 : Removed booleans in the RVH #

Patch Set 5 : Rebase + fixed bug in refcounting in FrameTree #

Patch Set 6 : Added unit tests #

Total comments: 39

Patch Set 7 : Rebase + addressed some of Nasko's comments #

Total comments: 16

Patch Set 8 : Addressed Nasko's comments #

Total comments: 4

Patch Set 9 : Addressed Nasko's comments #

Total comments: 38

Patch Set 10 : Addressed Charlie's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+634 lines, -201 lines) Patch
M chrome/browser/renderer_host/web_cache_manager_browsertest.cc View 1 2 3 1 chunk +7 lines, -4 lines 0 comments Download
M content/browser/frame_host/frame_tree.h View 1 2 3 4 5 6 7 8 9 2 chunks +14 lines, -6 lines 0 comments Download
M content/browser/frame_host/frame_tree.cc View 1 2 3 4 5 6 7 8 9 4 chunks +51 lines, -18 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_unittest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.h View 1 2 3 4 5 6 4 chunks +12 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.cc View 1 2 3 4 5 6 7 8 9 9 chunks +65 lines, -13 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 13 chunks +243 lines, -21 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.h View 1 2 3 4 5 6 7 3 chunks +2 lines, -5 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.h View 1 2 3 4 5 6 7 8 9 11 chunks +85 lines, -24 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.cc View 1 2 3 4 5 6 7 8 9 27 chunks +110 lines, -81 lines 0 comments Download
M content/browser/renderer_host/render_view_host_unittest.cc View 1 2 3 3 chunks +7 lines, -9 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 4 chunks +4 lines, -4 lines 0 comments Download
M content/browser/web_contents/web_contents_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +4 lines, -2 lines 0 comments Download
M content/public/browser/render_process_host.h View 1 2 3 4 5 6 7 2 chunks +5 lines, -0 lines 0 comments Download
M content/public/test/mock_render_process_host.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M content/public/test/mock_render_process_host.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -0 lines 0 comments Download
M content/public/test/test_renderer_host.cc View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M content/test/test_render_view_host.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -8 lines 0 comments Download
M content/test/test_web_contents.cc View 1 2 3 4 5 6 1 chunk +3 lines, -4 lines 0 comments Download

Messages

Total messages: 36 (0 generated)
clamy
@creis: Please take a look. @pasko: FYI
5 years, 8 months ago (2013-11-26 15:08:26 UTC) #1
Charlie Reis
[+ojan in CC] https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/renderer_host/render_view_host_impl.cc File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/renderer_host/render_view_host_impl.cc#newcode106 content/browser/renderer_host/render_view_host_impl.cc:106: // swapping renderers. The old renderer ...
5 years, 8 months ago (2013-11-26 19:35:00 UTC) #2
clamy
On 2013/11/26 19:35:00, creis wrote: > [+ojan in CC] > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/renderer_host/render_view_host_impl.cc > File content/browser/renderer_host/render_view_host_impl.cc ...
5 years, 8 months ago (2013-11-27 18:25:39 UTC) #3
Charlie Reis
On 2013/11/27 18:25:39, clamy wrote: > On 2013/11/26 19:35:00, creis wrote: > > [+ojan in ...
5 years, 8 months ago (2013-11-27 20:54:26 UTC) #4
clamy
On 2013/11/27 20:54:26, creis wrote: > On 2013/11/27 18:25:39, clamy wrote: > > On 2013/11/26 ...
5 years, 8 months ago (2013-12-06 17:33:50 UTC) #5
Charlie Reis
On 2013/12/06 17:33:50, clamy wrote: > Well the idea is that we need to keep ...
5 years, 8 months ago (2013-12-07 00:42:32 UTC) #6
Charlie Reis
This doesn't yet match what I was picturing. I'll try to describe what I was ...
5 years, 8 months ago (2013-12-09 20:12:29 UTC) #7
pasko
Fly-by comments. I am not expert in this area, so take my comments as just ...
5 years, 8 months ago (2013-12-10 19:31:47 UTC) #8
pasko
https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/renderer_host/render_view_host_impl.cc File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/renderer_host/render_view_host_impl.cc#newcode755 content/browser/renderer_host/render_view_host_impl.cc:755: need_to_perform_clean_up_on_swapped_out_ = false; when is it the case that ...
5 years, 8 months ago (2013-12-10 19:59:29 UTC) #9
Charlie Reis
On 2013/12/10 19:31:47, pasko wrote: > Fly-by comments. I am not expert in this area, ...
5 years, 8 months ago (2013-12-10 20:47:51 UTC) #10
pasko
On 2013/12/10 20:47:51, creis wrote: > > I like the simplicity of your sugestion: make ...
5 years, 8 months ago (2013-12-10 22:46:42 UTC) #11
Charlie Reis
On 2013/12/10 22:46:42, pasko wrote: > On 2013/12/10 20:47:51, creis wrote: > > > I ...
5 years, 8 months ago (2013-12-11 02:28:41 UTC) #12
clamy
I have uploaded an initial version of the CL, which should be quite close to ...
5 years, 8 months ago (2013-12-11 16:21:14 UTC) #13
Charlie Reis
On 2013/12/11 16:21:14, clamy wrote: > I have uploaded an initial version of the CL, ...
5 years, 8 months ago (2013-12-12 02:20:01 UTC) #14
clamy
+nasko@ and ppi@
5 years, 8 months ago (2013-12-16 22:21:12 UTC) #15
clamy
On 2013/12/16 22:21:12, clamy wrote: > +nasko@ and ppi@ @nasko: I have uploaded my latest ...
5 years, 7 months ago (2014-01-17 18:57:54 UTC) #16
clamy
@nasko: PTAL I have cleaned up the development version, and the patch should now be ...
5 years, 7 months ago (2014-01-21 21:52:18 UTC) #17
nasko
I'm sending you some of the comments, as I need to review the test files ...
5 years, 7 months ago (2014-01-23 23:34:58 UTC) #18
clamy
Thanks for your comments :) ! https://chromiumcodereview.appspot.com/88503002/diff/1180001/chrome/test/base/browser_with_test_window_test.cc File chrome/test/base/browser_with_test_window_test.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/chrome/test/base/browser_with_test_window_test.cc#newcode153 chrome/test/base/browser_with_test_window_test.cc:153: // Simulate the ...
5 years, 6 months ago (2014-01-24 17:01:54 UTC) #19
nasko
Replied to some of your comments and reviewed the tests. I think it is shaping ...
5 years, 6 months ago (2014-01-27 19:09:50 UTC) #20
clamy
Thanks for the review :) - I tried to address all of your comments with ...
5 years, 6 months ago (2014-01-28 12:27:20 UTC) #21
nasko
Just a couple of minor comments and I think it is ready for Charlie to ...
5 years, 6 months ago (2014-01-29 22:49:52 UTC) #22
clamy
Thanks! We're hoping to land this ASAP as we expect some nice performance improvements :) ...
5 years, 6 months ago (2014-01-30 12:36:53 UTC) #23
Charlie Reis
Glad to see you've all been making progress on this! My first day back from ...
5 years, 6 months ago (2014-01-31 01:40:16 UTC) #24
Charlie Reis
This looks like a great overhaul. It's a bit complex in places (like the multimap ...
5 years, 6 months ago (2014-01-31 19:11:54 UTC) #25
nasko
I don't have anything else other than a great job! LGTM
5 years, 6 months ago (2014-01-31 23:27:05 UTC) #26
clamy
@creis, creis: Thank you very much for your detailed and speedy review! It has been ...
5 years, 6 months ago (2014-02-04 22:39:40 UTC) #27
jam
lgtm, nice https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/browser/render_process_host.h File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/browser/render_process_host.h#newcode223 content/public/browser/render_process_host.h:223: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) = 0; ...
5 years, 6 months ago (2014-02-04 23:42:36 UTC) #28
clamy
Thanks for the review! https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/browser/render_process_host.h File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/browser/render_process_host.h#newcode223 content/public/browser/render_process_host.h:223: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) ...
5 years, 6 months ago (2014-02-05 14:21:41 UTC) #29
clamy
The CQ bit was checked by clamy@chromium.org
5 years, 6 months ago (2014-02-05 14:21:51 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/88503002/1740001
5 years, 6 months ago (2014-02-05 14:25:10 UTC) #31
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 6 months ago (2014-02-05 15:45:32 UTC) #32
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=221746
5 years, 6 months ago (2014-02-05 15:45:33 UTC) #33
clamy
The CQ bit was checked by clamy@chromium.org
5 years, 6 months ago (2014-02-07 10:54:25 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/88503002/1740001
5 years, 6 months ago (2014-02-07 10:54:33 UTC) #35
commit-bot: I haz the power
5 years, 6 months ago (2014-02-07 12:12:02 UTC) #36
Message was sent while issue was closed.
Change committed as 249676

Powered by Google App Engine
This is Rietveld 408576698