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

Issue 1142123002: Remove swapped-out usage in --site-per-process. (Closed)

Created:
5 years, 7 months ago by nasko
Modified:
5 years, 3 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove swapped-out usage in --site-per-process. When navigating cross-process, we use swapped-out RenderFrameHost as placeholders. The goal is to replace this with RenderFrameProxyHost, which is already the case for all subframes, but not the main frame. The code is far along that this can be done now for the main frame as well. This CL starts the path to removing the swapped-out RenderFrameHost concept by deleting the RenderFrameHost for the main frame on cross-process navigations and only keeping a RenderFrameProxyHost around. It is doing this for --site-per-process only, so all details can be fleshed out. Subsequent CL will remove the --site-per-process restriction and enable it in all modes of Chrome. BUG=357747 Committed: https://crrev.com/4c0feb652441719f6410ac137ef42b51c7c14a6e Cr-Commit-Position: refs/heads/master@{#333100}

Patch Set 1 #

Patch Set 2 : Disable couple of RVH tests. #

Patch Set 3 : Rebase on ToT #

Patch Set 4 : Hacking on unit tests. #

Patch Set 5 : Rebase on ToT to pick up couple of split out CLs. #

Patch Set 6 : Intermediate checkpoint, lots of stuff should work. #

Patch Set 7 : Rebase on ToT. #

Patch Set 8 : Lots of unit tests fixed. #

Patch Set 9 : Rebase to pick up unit tests changes. #

Patch Set 10 : Changing how RVH is notified of process crash & MockRPH to have real has_connection bit. #

Patch Set 11 : Rebase on ToT. #

Patch Set 12 : Fixes for all types of unit tests and content_browsertests. #

Patch Set 13 : Remove comments and cleanup. #

Patch Set 14 : Fix DisownOpener. #

Total comments: 44

Patch Set 15 : Fixes based on Charlie's review. #

Patch Set 16 : Add FrameReplicationState as a CreateRenderView parameter and remove DEPS exception. #

Patch Set 17 : Update FYI bot config. #

Patch Set 18 : Fix android build break. #

Patch Set 19 : Readd back overriden version of CreateRenderView in TestRenderViewHost. Fixes CleanUpSwappedOutRVHO… #

Total comments: 14

Patch Set 20 : Another round of fixes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+487 lines, -214 lines) Patch
M chrome/browser/ui/zoom/zoom_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/content_settings_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +8 lines, -4 lines 0 comments Download
M components/visitedlink/test/visitedlink_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/frame_host/frame_tree.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +12 lines, -5 lines 0 comments Download
M content/browser/frame_host/interstitial_page_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigator_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 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 10 11 12 13 14 15 16 17 18 19 14 chunks +83 lines, -30 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 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 10 11 12 13 14 15 29 chunks +155 lines, -63 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +11 lines, -5 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +7 lines, -8 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -2 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +25 lines, -12 lines 0 comments Download
M content/browser/web_contents/web_contents_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +12 lines, -9 lines 0 comments Download
M content/public/test/test_renderer_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +6 lines, -5 lines 0 comments Download
M content/renderer/accessibility/renderer_accessibility_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -0 lines 0 comments Download
M content/renderer/history_entry.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -1 line 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 8 chunks +29 lines, -18 lines 0 comments Download
M content/renderer/render_frame_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +12 lines, -7 lines 0 comments Download
M content/renderer/render_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +13 lines, -0 lines 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +42 lines, -29 lines 0 comments Download
M content/test/test_render_frame_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M content/test/test_render_view_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +8 lines, -0 lines 0 comments Download
M content/test/test_render_view_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +12 lines, -0 lines 0 comments Download
M content/test/test_web_contents.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +6 lines, -4 lines 0 comments Download
M content/test/test_web_contents.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +4 lines, -1 line 0 comments Download
M testing/buildbot/chromium.fyi.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +2 lines, -2 lines 0 comments Download
M ui/views/controls/webview/webview_interactive_uitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 31 (13 generated)
nasko
Hey Charlie, I think this CL is ready to start reviewing. There are a couple ...
5 years, 6 months ago (2015-06-03 14:17:33 UTC) #8
nasko
Actually adding Charlie :)
5 years, 6 months ago (2015-06-03 18:18:02 UTC) #10
Charlie Reis
Wow, this looks pretty good! Some thoughts below, but I'm very excited to get this ...
5 years, 6 months ago (2015-06-04 00:02:10 UTC) #11
nasko
Fixes. One remaining part will be in a follow-up patch - passing the frame replication ...
5 years, 6 months ago (2015-06-04 14:57:14 UTC) #12
nasko
Removed DEPS exception. The last issue to address - comments in code on what to ...
5 years, 6 months ago (2015-06-04 16:45:08 UTC) #13
Charlie Reis
Great-- just a few issues below, and I think you're ready to start getting owners ...
5 years, 6 months ago (2015-06-04 22:27:32 UTC) #16
nasko
Fixes for the last round of review. Adding OWNERS reviewers: sky@ - components/visitedlink/ thestig@ - ...
5 years, 6 months ago (2015-06-04 23:38:37 UTC) #18
sky
LGTM
5 years, 6 months ago (2015-06-04 23:47:47 UTC) #19
Lei Zhang
lgtm
5 years, 6 months ago (2015-06-04 23:53:22 UTC) #20
sadrul
On 2015/06/04 23:38:37, nasko wrote: > Fixes for the last round of review. Adding OWNERS ...
5 years, 6 months ago (2015-06-04 23:55:38 UTC) #21
jam
lgtm
5 years, 6 months ago (2015-06-05 04:28:47 UTC) #22
Charlie Reis
LGTM! CQ'ing. https://codereview.chromium.org/1142123002/diff/410020/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1142123002/diff/410020/content/browser/frame_host/render_frame_host_manager.cc#newcode1545 content/browser/frame_host/render_frame_host_manager.cc:1545: if (is_site_per_process) { On 2015/06/04 23:38:36, nasko ...
5 years, 6 months ago (2015-06-05 18:28:45 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1142123002/510001
5 years, 6 months ago (2015-06-05 18:30:43 UTC) #25
commit-bot: I haz the power
Committed patchset #20 (id:510001)
5 years, 6 months ago (2015-06-05 18:37:14 UTC) #26
commit-bot: I haz the power
Patchset 20 (id:??) landed as https://crrev.com/4c0feb652441719f6410ac137ef42b51c7c14a6e Cr-Commit-Position: refs/heads/master@{#333100}
5 years, 6 months ago (2015-06-05 19:06:14 UTC) #27
pgorszkowski
https://codereview.chromium.org/1142123002/diff/380001/content/renderer/render_view_impl.cc File content/renderer/render_view_impl.cc (left): https://codereview.chromium.org/1142123002/diff/380001/content/renderer/render_view_impl.cc#oldcode752 content/renderer/render_view_impl.cc:752: Moving this part before ApplyWebPreferences causes that first, empty ...
5 years, 3 months ago (2015-09-14 13:26:10 UTC) #29
nasko
On 2015/09/14 13:26:10, pgorszkowski wrote: > https://codereview.chromium.org/1142123002/diff/380001/content/renderer/render_view_impl.cc > File content/renderer/render_view_impl.cc (left): > > https://codereview.chromium.org/1142123002/diff/380001/content/renderer/render_view_impl.cc#oldcode752 > ...
5 years, 3 months ago (2015-09-16 21:25:50 UTC) #30
Charlie Reis
5 years, 3 months ago (2015-09-16 21:39:51 UTC) #31
Message was sent while issue was closed.
On 2015/09/16 21:25:50, nasko (slow to review) wrote:
> On 2015/09/14 13:26:10, pgorszkowski wrote:
> >
>
https://codereview.chromium.org/1142123002/diff/380001/content/renderer/rende...
> > File content/renderer/render_view_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/1142123002/diff/380001/content/renderer/rende...
> > content/renderer/render_view_impl.cc:752: 
> > Moving this part before ApplyWebPreferences causes that first, empty site
will
> > not have a webkit_preferences(for example background will be default =
white).
> > Am I right?
> 
> I have not investigated this. Is it a problem? If so, why not file a bug and
we
> can discuss there?

I think this matters, right?   It's incorrect to show white on the initial page
if the content embedder has set a preference to show black, etc.

But yes, a bug report would be the right place to discuss.

Powered by Google App Engine
This is Rietveld 408576698