Change IsSwappedOut to const and do a more restrictive check.

Change IsSwappedOut to const and do a more restrictive check.

BUG=226354, 226350
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195200

[nasko, 2013-04-15 20:21:58]

Charlie,
Can you review this change? This is based on code in our prototype, so I'm
porting over the fixes.
Thanks,
Nasko

[Charlie Reis, 2013-04-15 21:14:02]

https://codereview.chromium.org/14267002/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/14267002/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:982:
RenderViewHostMap::const_iterator iter = swapped_out_hosts_.find(
Good catch.  Can you add a comment here?  Something like:
"Only consider |rvh| swapped out if we can actually find it within
swapped_out_hosts_."

Actually, I wonder if we want to change the method name...  There's already a
RenderViewHostImpl::is_swapped_out(), so we kind of want to indicate that we're
checking whether it's local to this RVHM.  Would RVHM::IsOnSwappedOutList be
better or more confusing?

[nasko, 2013-04-15 21:52:46]

I went with renaming.

https://codereview.chromium.org/14267002/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/14267002/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:982:
RenderViewHostMap::const_iterator iter = swapped_out_hosts_.find(
On 2013/04/15 21:14:02, creis wrote:
> Good catch.  Can you add a comment here?  Something like:
> "Only consider |rvh| swapped out if we can actually find it within
> swapped_out_hosts_."
> 
> Actually, I wonder if we want to change the method name...  There's already a
> RenderViewHostImpl::is_swapped_out(), so we kind of want to indicate that
we're
> checking whether it's local to this RVHM.  Would RVHM::IsOnSwappedOutList be
> better or more confusing?

Rename sounds like the better option. I like your proposal for a new name, so
I've gone through with it.

[Charlie Reis, 2013-04-15 22:00:22]

LGTM!

[commit-bot: I haz the power, 2013-04-18 16:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/14267002/6001

[commit-bot: I haz the power, 2013-04-18 17:59:27]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-18 20:40:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/14267002/6001

[commit-bot: I haz the power, 2013-04-19 13:29:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/14267002/6001

[commit-bot: I haz the power, 2013-04-19 15:07:22]

Change committed as 195200