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

Issue 967383002: PlzNavigate: Avoid duplicate SiteInstance creation during navigation. (Closed)

Created:
5 years, 9 months ago by carlosk
Modified:
5 years, 6 months ago
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PlzNavigate: Avoid duplicate SiteInstance creation during navigation. These changes to RenderFrameHostManager make it so that during a navigation a new SiteInstance won't be created for a URL if a previously created one still exists for that same URL. This also follows more closely how the current navigation implementation works. BUG=376094 TEST=WebContentsImplTest.ActiveContentsCountChangeBrowsingInstance Committed: https://crrev.com/078298a8fcafefc5276f58c657e95b4fc1ff4009 Cr-Commit-Position: refs/heads/master@{#324027}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Moved the logic into GetSiteInstanceForNavigation. #

Total comments: 2

Patch Set 3 : The solution as a rewrite of the SiteInstance determination methods (with rebase). #

Patch Set 4 : Re-split DetermineSiteInstanceForURL into 2 methods as it was before. #

Patch Set 5 : Extract RFH selection logic form GetFrameHostForNavigation. #

Total comments: 21

Patch Set 6 : Address CR comments. #

Patch Set 7 : Merged GetSiteInstanceForNavigation with IsCorrectSiteInstanceForURL. #

Total comments: 45

Patch Set 8 : Rebase #

Patch Set 9 : Consolidated methods into a simpler change; addressed other CR comments. #

Total comments: 28

Patch Set 10 : #

Total comments: 4

Patch Set 11 : Minor changes. #

Patch Set 12 : Add tests to check RFHM::ConvertToSiteInstance works as expected. #

Total comments: 14

Patch Set 13 : General changes and improvements from CR comments. #

Patch Set 14 : Rebase + content-exporting SiteInstanceDescriptor. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -50 lines) Patch
M content/browser/frame_host/navigator_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +139 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 2 chunks +49 lines, -14 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 chunks +88 lines, -36 lines 0 comments Download

Messages

Total messages: 53 (16 generated)
carlosk
clamy@, nasko@: PTAL. fdegans@: FYI. As suggested I made a new CL for this change, ...
5 years, 9 months ago (2015-03-02 18:34:51 UTC) #3
nasko
I'll take a look how this fits in the other CL, but on it's own ...
5 years, 9 months ago (2015-03-02 23:02:09 UTC) #4
nasko
https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/render_frame_host_manager.cc#newcode1184 content/browser/frame_host/render_frame_host_manager.cc:1184: speculative_render_frame_host_->GetSiteInstance()->GetSiteURL() == On 2015/03/02 23:02:09, nasko wrote: > This ...
5 years, 9 months ago (2015-03-02 23:19:47 UTC) #5
Charlie Reis
I don't think we should be making this change. It's hiding the fact that we ...
5 years, 9 months ago (2015-03-02 23:52:40 UTC) #7
carlosk
On 2015/03/02 23:52:40, Charlie Reis wrote: > I don't think we should be making this ...
5 years, 9 months ago (2015-03-03 13:42:41 UTC) #8
Charlie Reis
Sorry for the delay; I wanted to think about the possibilities of what to expose. ...
5 years, 9 months ago (2015-03-05 04:27:11 UTC) #9
carlosk
Thanks Charlie for the thorough response. I already read through your response (more than once) ...
5 years, 9 months ago (2015-03-05 16:59:55 UTC) #10
Charlie Reis
On 2015/03/05 16:59:55, carlosk wrote: > Thanks Charlie for the thorough response. I already read ...
5 years, 9 months ago (2015-03-05 17:13:35 UTC) #11
carlosk
That previous comment did indeed help. :) On 2015/03/05 04:27:11, Charlie Reis wrote: > Sorry ...
5 years, 9 months ago (2015-03-06 16:03:54 UTC) #12
carlosk
On 2015/03/06 16:03:54, carlosk wrote: > That previous comment did indeed help. :) > > ...
5 years, 9 months ago (2015-03-06 16:09:34 UTC) #13
Charlie Reis
On 2015/03/06 16:09:34, carlosk (OoO until March 16th) wrote: > On 2015/03/06 16:03:54, carlosk wrote: ...
5 years, 9 months ago (2015-03-06 23:15:32 UTC) #14
carlosk
Thanks! On 2015/03/06 23:15:32, Charlie Reis wrote: > On 2015/03/06 16:09:34, carlosk (OoO until March ...
5 years, 9 months ago (2015-03-17 12:38:45 UTC) #15
carlosk
This implements what was discussed previously. Right now render_frame_host_manager.cc has 2200+ LoC. I think the ...
5 years, 9 months ago (2015-03-23 14:31:19 UTC) #16
clamy
Thanks! I'm not sure we have yet the right interface for the logic of selecting ...
5 years, 9 months ago (2015-03-23 15:26:44 UTC) #17
carlosk
Thanks. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_host/render_frame_host_manager.h File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_host/render_frame_host_manager.h#newcode26 content/browser/frame_host/render_frame_host_manager.h:26: struct SiteInstanceDescriptor; On 2015/03/23 15:26:44, clamy wrote: > ...
5 years, 9 months ago (2015-03-23 17:39:19 UTC) #18
carlosk
Just and extra note. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_host/render_frame_host_manager.cc#newcode824 content/browser/frame_host/render_frame_host_manager.cc:824: default: { NOTREACHED(); } On ...
5 years, 9 months ago (2015-03-23 17:43:02 UTC) #19
Charlie Reis
Sorry-- I tried to get to this today but didn't have time, due to an ...
5 years, 9 months ago (2015-03-24 00:02:14 UTC) #20
carlosk
Following clamy@'s suggestion I merged IsCorrectSiteInstanceForURL into GetSiteInstanceForNavigation (see comments on this) what made this ...
5 years, 9 months ago (2015-03-24 15:30:06 UTC) #23
Charlie Reis
Round of comments below. I can see where this is heading, and I think it ...
5 years, 9 months ago (2015-03-25 00:09:26 UTC) #24
carlosk
Thanks! With the latest suggestions from creis@ the change is now smaller as both RFHM::GetFrameHostForNavigation ...
5 years, 8 months ago (2015-03-30 14:37:38 UTC) #26
Charlie Reis
Thanks-- it's much easier to reason about the CL now that it's focused on the ...
5 years, 8 months ago (2015-03-31 06:18:19 UTC) #27
carlosk
Thanks. https://chromiumcodereview.appspot.com/967383002/diff/140001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/140001/content/browser/frame_host/render_frame_host_manager.cc#newcode1136 content/browser/frame_host/render_frame_host_manager.cc:1136: CHECK_NE(current_instance, descriptor.existent_site_instance); On 2015/03/31 06:18:18, Charlie Reis wrote: ...
5 years, 8 months ago (2015-03-31 16:38:20 UTC) #28
Charlie Reis
Great. This looks good, and it's ready to land apart from tests. I'd suggest some ...
5 years, 8 months ago (2015-03-31 20:19:58 UTC) #29
carlosk
Thanks once more. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (left): https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/frame_host/render_frame_host_manager.cc#oldcode1038 content/browser/frame_host/render_frame_host_manager.cc:1038: // Determine if we need a ...
5 years, 8 months ago (2015-04-01 15:25:30 UTC) #31
Charlie Reis
Did you see my request for tests in the last comment? Sorry if that slipped ...
5 years, 8 months ago (2015-04-01 16:08:28 UTC) #32
carlosk
On 2015/04/01 16:08:28, Charlie Reis wrote: > Did you see my request for tests in ...
5 years, 8 months ago (2015-04-02 17:15:06 UTC) #33
Charlie Reis
Great-- that's a nice and comprehensive test. LGTM with nits. https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_host/navigator_impl_unittest.cc File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_host/navigator_impl_unittest.cc#newcode1010 ...
5 years, 8 months ago (2015-04-02 21:23:25 UTC) #34
carlosk
Thanks! https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/frame_host/navigator_impl_unittest.cc File content/browser/frame_host/navigator_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/frame_host/navigator_impl_unittest.cc#newcode1010 content/browser/frame_host/navigator_impl_unittest.cc:1010: TEST_F(NavigatorTestWithBrowserSideNavigation, On 2015/04/02 21:23:25, Charlie Reis wrote: > ...
5 years, 8 months ago (2015-04-03 16:11:19 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967383002/320001
5 years, 8 months ago (2015-04-03 16:11:47 UTC) #39
commit-bot: I haz the power
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from ...
5 years, 8 months ago (2015-04-03 16:11:54 UTC) #41
Charlie Reis
Thanks, still LGTM. Looks like clamy@ is marked as a required reviewer as well.
5 years, 8 months ago (2015-04-03 17:28:58 UTC) #42
carlosk
Removed clamy@'s asterisk.
5 years, 8 months ago (2015-04-03 17:35:15 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967383002/320001
5 years, 8 months ago (2015-04-03 17:36:23 UTC) #46
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/26125)
5 years, 8 months ago (2015-04-03 19:02:10 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967383002/340001
5 years, 8 months ago (2015-04-07 09:43:32 UTC) #51
commit-bot: I haz the power
Committed patchset #14 (id:340001)
5 years, 8 months ago (2015-04-07 10:33:29 UTC) #52
commit-bot: I haz the power
5 years, 8 months ago (2015-04-07 10:34:19 UTC) #53
Message was sent while issue was closed.
Patchset 14 (id:??) landed as
https://crrev.com/078298a8fcafefc5276f58c657e95b4fc1ff4009
Cr-Commit-Position: refs/heads/master@{#324027}

Powered by Google App Engine
This is Rietveld 408576698