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

Issue 9965091: Clean up RenderViewHostManager swapping logic. (Closed)

Created:
8 years, 8 months ago by Charlie Reis
Modified:
8 years, 6 months ago
CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Clean up RenderViewHostManager swapping logic. Makes the difference between swapping SiteInstances and swapping BrowsingInstances explicit, and adds CHECKs to enforce invariants more effectively. BUG=123007 TEST=No functionality change. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139933

Patch Set 1 #

Patch Set 2 : Further refactoring #

Patch Set 3 : Distinguish BrowsingInstance swaps. #

Patch Set 4 : Fix DevToolsManagerTest compile #

Patch Set 5 : Cleanup #

Patch Set 6 : Fix merge conflicts #

Patch Set 7 : Fix Ash and Views content clients. #

Patch Set 8 : Fix merge conflict. #

Patch Set 9 : Merge to latest #

Patch Set 10 : Merge and clean up comments. #

Total comments: 8

Patch Set 11 : Fix review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -113 lines) Patch
M chrome/browser/chrome_content_browser_client.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +34 lines, -18 lines 0 comments Download
M content/browser/debugger/devtools_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/web_contents/render_view_host_manager.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +14 lines, -10 lines 0 comments Download
M content/browser/web_contents/render_view_host_manager.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +103 lines, -74 lines 0 comments Download
M content/browser/web_contents/render_view_host_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M content/public/browser/content_browser_client.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -4 lines 0 comments Download
M content/public/browser/content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -3 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Charlie Reis
Darin, here's the refactoring I mentioned yesterday. Can you take a look?
8 years, 8 months ago (2012-04-11 20:49:58 UTC) #1
Charlie Reis
There seem to be real test failures in PlatformAppsBrowserTest, but only on Windows. It's crashing ...
8 years, 8 months ago (2012-04-12 17:52:11 UTC) #2
Charlie Reis
Darin, this cleanup CL is ready for review. Can you take a look?
8 years, 6 months ago (2012-05-30 22:31:49 UTC) #3
darin (slow to review)
Looks good... just some nits / questions: https://chromiumcodereview.appspot.com/9965091/diff/35003/content/browser/debugger/devtools_manager_unittest.cc File content/browser/debugger/devtools_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9965091/diff/35003/content/browser/debugger/devtools_manager_unittest.cc#newcode13 content/browser/debugger/devtools_manager_unittest.cc:13: #include "content/public/browser/browser_context.h" ...
8 years, 6 months ago (2012-05-31 04:43:07 UTC) #4
Charlie Reis
https://chromiumcodereview.appspot.com/9965091/diff/35003/content/browser/debugger/devtools_manager_unittest.cc File content/browser/debugger/devtools_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9965091/diff/35003/content/browser/debugger/devtools_manager_unittest.cc#newcode13 content/browser/debugger/devtools_manager_unittest.cc:13: #include "content/public/browser/browser_context.h" On 2012/05/31 04:43:08, darin wrote: > nit: ...
8 years, 6 months ago (2012-05-31 17:58:22 UTC) #5
darin (slow to review)
On Thu, May 31, 2012 at 10:58 AM, <creis@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/9965091/diff/** > 35003/content/browser/**debugger/devtools_manager_**unittest.cc<https://chromiumcodereview.appspot.com/9965091/diff/35003/content/browser/debugger/devtools_manager_unittest.cc> ...
8 years, 6 months ago (2012-05-31 19:40:20 UTC) #6
darin (slow to review)
LGTM
8 years, 6 months ago (2012-05-31 19:40:56 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/creis@chromium.org/9965091/48002
8 years, 6 months ago (2012-05-31 19:42:19 UTC) #8
commit-bot: I haz the power
8 years, 6 months ago (2012-06-01 00:12:30 UTC) #9
Change committed as 139933

Powered by Google App Engine
This is Rietveld 408576698