Only delete old frames when a new main frame navigation commits

Only delete old frames when a new main frame navigation commits

Also, use RVH* to identify frames, not the process id. when we run out of
processes, several unrelated RVHs might share a process.

BUG=136090, 139117
TEST=browser_tests:*WebNavigation*, unit_tests:FrameNavigation*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149161

[jochen (gone - plz use gerrit), 2012-07-30 11:45:46]

plz review

[Matt Perry, 2012-07-30 11:52:43]

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:37:
content::RenderViewHost* render_view_host;
Is render_view_host guaranteed to outlive a FrameID? The webnavigation API seems
to be assuming that it will always get the final navigation event for all RVHs.
Maybe you should listen to RENDER_VIEW_HOST_DESTROYED as well, to be safe.

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right):

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/web_navigation_api.h:43:
content::RenderViewHost* render_view_host_in_process(int process_id) const;
should use CamelCase for non-inline functions.

[jochen (gone - plz use gerrit), 2012-07-30 15:33:17]

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:37:
content::RenderViewHost* render_view_host;
On 2012/07/30 11:52:43, Matt Perry wrote:
> Is render_view_host guaranteed to outlive a FrameID? The webnavigation API
seems
> to be assuming that it will always get the final navigation event for all
RVHs.
> Maybe you should listen to RENDER_VIEW_HOST_DESTROYED as well, to be safe.

Done.

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right):

https://chromiumcodereview.appspot.com/10835033/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/web_navigation_api.h:43:
content::RenderViewHost* render_view_host_in_process(int process_id) const;
On 2012/07/30 11:52:43, Matt Perry wrote:
> should use CamelCase for non-inline functions.

Done.

[Matt Perry, 2012-07-31 09:58:09]

lgtm

https://chromiumcodereview.appspot.com/10835033/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://chromiumcodereview.appspot.com/10835033/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:320:
SendErrorEvents(web_contents(), render_view_host);
Technically this is dangerous. This notification is sent from the RVH
destructor. SendErrorEvents ends up invoking a virtual method (GetProcess()) on
the RVH that is being destroyed. I think this particular case is fine, but a
purist would say this is not allowed. (It's dangerous because the vtable refers
to the RVH only - not a derived class - while in the destructor, as all derived
classes have invoked their destructors).

[jochen (gone - plz use gerrit), 2012-07-31 10:12:32]

https://chromiumcodereview.appspot.com/10835033/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://chromiumcodereview.appspot.com/10835033/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:320:
SendErrorEvents(web_contents(), render_view_host);
I agree that it's a bit borderline :-/