Switch RenderFrameHostManager to use RenderProcessHostObserver, remove NOTIFICATION_RENDERER_PROCES…

Switch RenderFrameHostManager to use RenderProcessHostObserver, remove NOTIFICATION_RENDERER_PROCESS_CLOSING.

BUG=170921
TEST=everything still works

Committed: https://crrev.com/013e398d964020455a55d95b7b1d1cfe92036e28
Cr-Commit-Position: refs/heads/master@{#339766}

[Avi (use Gerrit), 2015-07-21 17:28:16]

avi@chromium.org changed reviewers:
+ creis@chromium.org

[Avi (use Gerrit), 2015-07-21 17:28:16]

It's... green!

[ncarter (slow), 2015-07-21 19:07:14]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2015-07-21 19:07:15]

I dig this.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:52: typedef
base::hash_map<int32, RenderFrameProxyHost*> MapType;
using MapType = base::hash_map<int32, RenderFrameProxyHost*>;

(assuming the language about aliases on https://chromium-cpp.appspot.com/ is
normative)

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:59:
MapType::const_iterator begin() { return map_.begin(); }
Should these accessors be const, to fit the flavor of them returning
const_iterators?

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:694: for (const auto&
pair : *proxy_hosts_) {
This is cool.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:768:
RenderFrameProxyHost* proxy = scoped_proxy.get();
You could simplify this a little by eliminating the |scoped_proxy| temporary,
assigning the result of "new RFPH()"  to |proxy|, and passing
make_scoped_ptr(proxy) to Add(). Honestly it's a no-win situation here and I'm
okay with what you have too. But my bias tips very slightly toward avoiding a
nulled-out |scoped_proxy| variable, since we've had exactly that kind of
use-scopedptr-after-Pass() bug in this function before ( my fault:
https://codereview.chromium.org/1074843002/diff/60001/content/browser/frame_h...
).

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:810:
RenderFrameProxyHost* proxy = scoped_proxy.get();
Ditto.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1640:
proxy_hosts_->Remove(instance->GetId());
Is it worth adding a "// deletes |proxy|" or "proxy = NULL;" here?

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1658: proxy =
scoped_proxy.get();
Ditto

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1745: proxy =
scoped_proxy.get();
Ditto

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1795:
RenderFrameProxyHost* proxy = scoped_proxy.get();
Ditto

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2078:
CHECK(proxy_hosts_->Get(render_frame_host_->GetSiteInstance()->GetId()) ==
CHECK(!proxy_hosts->Get(...)) ?

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.h:716: std::map<int,
RenderFrameProxyHost*> GetAllProxyHostsForTesting();
Not clear what the ints keys are here -- add a comment?

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.h:716: std::map<int,
RenderFrameProxyHost*> GetAllProxyHostsForTesting();
If you made this public, we wouldn't need "friend class FrameTreeVisualizer".
Which seems like the right tradeoff, since the intersection of "ForTesting" and
RFHM being content-internal gives a fair amount of protection?

[Note how I resisted the urge to suggest FriendZone]

https://codereview.chromium.org/1245903004/diff/1/content/public/browser/rend...
File content/public/browser/render_process_host_observer.h (right):

https://codereview.chromium.org/1245903004/diff/1/content/public/browser/rend...
content/public/browser/render_process_host_observer.h:21: // used for further
navigations.
I think this gets a little confusing, especially with how RenderProcessExited
has STILL_RUNNING as an option for |status|. So, the more comments here the
better -- in particular, assume that the reader needs to tell the difference
between all the functions here. My mental model is something like:

 - WillExit: don't place any new objects in this renderer process (unless ...
it's recreated???)
 - Exited: you will never again hear from objects that were in this renderer
process (... but watch out, they might be reborn under the same ids in a new
child process!)
 - HostDestroyed: destroy any objects tied to the rph, that aren't tied to the
lifetime of any single child process.

[Avi (use Gerrit), 2015-07-21 20:45:16]

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:52: typedef
base::hash_map<int32, RenderFrameProxyHost*> MapType;
On 2015/07/21 19:07:14, ncarter wrote:
> using MapType = base::hash_map<int32, RenderFrameProxyHost*>;
> 
> (assuming the language about aliases on https://chromium-cpp.appspot.com/ is
> normative)

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:59:
MapType::const_iterator begin() { return map_.begin(); }
On 2015/07/21 19:07:15, ncarter wrote:
> Should these accessors be const, to fit the flavor of them returning
> const_iterators?

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:694: for (const auto&
pair : *proxy_hosts_) {
On 2015/07/21 19:07:14, ncarter wrote:
> This is cool.

Thanks!

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:768:
RenderFrameProxyHost* proxy = scoped_proxy.get();
On 2015/07/21 19:07:14, ncarter wrote:
> You could simplify this a little by eliminating the |scoped_proxy| temporary,
> assigning the result of "new RFPH()"  to |proxy|, and passing
> make_scoped_ptr(proxy) to Add(). Honestly it's a no-win situation here and I'm
> okay with what you have too. But my bias tips very slightly toward avoiding a
> nulled-out |scoped_proxy| variable, since we've had exactly that kind of
> use-scopedptr-after-Pass() bug in this function before ( my fault:
>
https://codereview.chromium.org/1074843002/diff/60001/content/browser/frame_h...
> ).

That's a little cleaner; I'll go with that. I just want to ensure that Add()
takes a scoped_ptr to make its ownership transfer explicit.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:810:
RenderFrameProxyHost* proxy = scoped_proxy.get();
On 2015/07/21 19:07:14, ncarter wrote:
> Ditto.

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1640:
proxy_hosts_->Remove(instance->GetId());
On 2015/07/21 19:07:14, ncarter wrote:
> Is it worth adding a "// deletes |proxy|" or "proxy = NULL;" here?

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1658: proxy =
scoped_proxy.get();
On 2015/07/21 19:07:14, ncarter wrote:
> Ditto

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1745: proxy =
scoped_proxy.get();
On 2015/07/21 19:07:14, ncarter wrote:
> Ditto

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1795:
RenderFrameProxyHost* proxy = scoped_proxy.get();
On 2015/07/21 19:07:14, ncarter wrote:
> Ditto

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2078:
CHECK(proxy_hosts_->Get(render_frame_host_->GetSiteInstance()->GetId()) ==
On 2015/07/21 19:07:14, ncarter wrote:
> CHECK(!proxy_hosts->Get(...)) ?

Done.

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1245903004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.h:716: std::map<int,
RenderFrameProxyHost*> GetAllProxyHostsForTesting();
On 2015/07/21 19:07:15, ncarter wrote:
> Not clear what the ints keys are here -- add a comment?

Done.

https://codereview.chromium.org/1245903004/diff/1/content/public/browser/rend...
File content/public/browser/render_process_host_observer.h (right):

https://codereview.chromium.org/1245903004/diff/1/content/public/browser/rend...
content/public/browser/render_process_host_observer.h:21: // used for further
navigations.
On 2015/07/21 19:07:15, ncarter wrote:
> I think this gets a little confusing, especially with how RenderProcessExited
> has STILL_RUNNING as an option for |status|. So, the more comments here the
> better -- in particular, assume that the reader needs to tell the difference
> between all the functions here. My mental model is something like:
> 
>  - WillExit: don't place any new objects in this renderer process (unless ...
> it's recreated???)
>  - Exited: you will never again hear from objects that were in this renderer
> process (... but watch out, they might be reborn under the same ids in a new
> child process!)
>  - HostDestroyed: destroy any objects tied to the rph, that aren't tied to the
> lifetime of any single child process.

Done.

[ncarter (slow), 2015-07-21 20:58:29]

lgtm

https://codereview.chromium.org/1245903004/diff/20001/content/public/browser/...
File content/public/browser/render_process_host_observer.h (right):

https://codereview.chromium.org/1245903004/diff/20001/content/public/browser/...
content/public/browser/render_process_host_observer.h:23: // first step in an
orderly shutdown of a render process, do not expect that
This is a lot clearer, thanks!

[Avi (use Gerrit), 2015-07-21 20:59:34]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-07-21 21:00:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1245903004/20001

[commit-bot: I haz the power, 2015-07-21 22:28:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-21 22:29:35]

Patchset 2 (id:??) landed as
https://crrev.com/013e398d964020455a55d95b7b1d1cfe92036e28
Cr-Commit-Position: refs/heads/master@{#339766}