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

Issue 1851253002: Remove RFHM::IsPendingDeletion (Closed)

Created:
4 years, 8 months ago by nasko
Modified:
4 years, 8 months ago
Reviewers:
alexmos
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_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 RFHM::IsPendingDeletion With RenderFrameHost always being deleted now, this method doesn't make much sense. It is obsolete since all RenderFrameHosts are now deleted and never reused, so the equivalent check is whether the RenderFrameHost is in active state or not. I've ran content_(browser|unit)tests with a CHECK inside RenderFrameHostImpl::OnSwappedOut to ensure is always true and it passes clean. BUG=357747 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/e2ef58ac47ab6760814fb446007f95b1cffe225a Cr-Commit-Position: refs/heads/master@{#385424}

Patch Set 1 #

Patch Set 2 : Rebase on ToT. #

Total comments: 4

Patch Set 3 : Latest fixes. #

Total comments: 1

Patch Set 4 : Fix nit. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -28 lines) Patch
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 2 chunks +2 lines, -3 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.h View 1 2 1 chunk +0 lines, -3 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.cc View 1 2 2 chunks +2 lines, -14 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_browsertest.cc View 1 2 3 2 chunks +4 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_unittest.cc View 1 2 chunks +0 lines, -5 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 19 (10 generated)
nasko
Hey Alex, Can you review this CL for me? It is continuation of removing code ...
4 years, 8 months ago (2016-04-01 23:16:10 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851253002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851253002/20001
4 years, 8 months ago (2016-04-04 17:14:18 UTC) #5
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/44784)
4 years, 8 months ago (2016-04-04 17:24:24 UTC) #7
alexmos
Looks good, though it seems there are a couple more call sites that needs to ...
4 years, 8 months ago (2016-04-04 18:53:57 UTC) #8
nasko
I've updated the CL description and fixed the issues you pointed out. Let me know ...
4 years, 8 months ago (2016-04-05 23:49:39 UTC) #10
alexmos
LGTM > I've updated the CL description and fixed the issues you pointed out. Let ...
4 years, 8 months ago (2016-04-06 00:26:33 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851253002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851253002/80001
4 years, 8 months ago (2016-04-06 08:13:21 UTC) #15
commit-bot: I haz the power
Committed patchset #4 (id:80001)
4 years, 8 months ago (2016-04-06 09:44:32 UTC) #17
commit-bot: I haz the power
4 years, 8 months ago (2016-04-06 09:45:50 UTC) #19
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/e2ef58ac47ab6760814fb446007f95b1cffe225a
Cr-Commit-Position: refs/heads/master@{#385424}

Powered by Google App Engine
This is Rietveld 408576698