Move ClosePage() from RenderViewHost to WebContents.

Move ClosePage() from RenderViewHost to WebContents.

ClosePage is a page-level concept and because of the deprecation of
RenderViewHost, it has to move. WebContents is the object representing
page, so I'm moving this public API to it.

BUG=304341, 478281
Committed: https://crrev.com/c0fceff6214f05360be23a5fc2ee578e9d7707aa
Cr-Commit-Position: refs/heads/master@{#327719}

[nasko, 2015-04-20 20:27:02]

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

[nasko, 2015-04-20 20:27:02]

Charlie,
Can you take a look at this CL? It moves the ClosePage() from RenderViewHost
onto WebContents, as it is a page-level action.
Thanks!
Nasko

[dgozman, 2015-04-21 05:13:31]

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

[dgozman, 2015-04-21 05:13:32]

devtools lgtm

https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:477:
WebContentsImpl::FromRenderFrameHost(render_frame_host_)->ClosePage();
if (web_contents())
  web_contents()->ClosePage();

[Charlie Reis, 2015-04-21 17:28:09]

Nice!  Just some cleanup tasks below.

https://codereview.chromium.org/1094153002/diff/1/chrome/browser/ui/fast_unlo...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/1094153002/diff/1/chrome/browser/ui/fast_unlo...
chrome/browser/ui/fast_unload_controller.cc:418: // Null check render_view_host
here as this gets called on a PostTask
I'm having trouble imagining how the WebContents could have a null RVH without
being deleted.  That said, we do have RVH null checks scattered throughout
WebContents.  Can you remind me why they're necessary?  (This might be a good
thing for Lucas to mention in his doc, or else something we can clean up.)

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:229: void ClosePage();
This needs a comment now that it's not inherited.  Something like what we had in
render_view_host.h, but I think the ResourceDispatcherHost mention was stale.

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_unittest.cc:101:
static_cast<RenderViewHostImpl*>(rvh())->ClosePage();
Would test_rvh() work?

https://codereview.chromium.org/1094153002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:685:
static_cast<RenderViewHostImpl*>(GetRenderViewHost())->ClosePage();
GetRenderViewHostImpl() would take care of the cast for you.

(I may do another CL after this to kill GetRenderViewHostImpl and just make
GetRenderViewHost have a covariant return type, but no need to do that here.)

https://codereview.chromium.org/1094153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4010: void
WebContentsImpl::RouteCloseEvent(RenderViewHost* rvh) {
nit: Blank line before, as long as we're changing this method.

https://codereview.chromium.org/1094153002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1094153002/diff/1/content/public/browser/web_...
content/public/browser/web_contents.h:225: // ResourceDispatcherHost when it is
finished.
This last sentence needs updating.  I think the ACK goes to RVH and ends up
calling Close(rvh) on this WebContents, right?

https://codereview.chromium.org/1094153002/diff/1/extensions/browser/process_...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/1094153002/diff/1/extensions/browser/process_...
extensions/browser/process_manager.cc:809: // RenderViewHost::ClosePage() may
result in calling
nit: Update comment.

[nasko, 2015-04-29 17:09:27]

nasko@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, thestig@chromium.org

[nasko, 2015-04-29 17:09:27]

rdevlin.cronin@chromium.org: Please review changes in extensions/

thestig@chromium.org: Please review changes in chrome/browser/ui

https://codereview.chromium.org/1094153002/diff/1/chrome/browser/ui/fast_unlo...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/1094153002/diff/1/chrome/browser/ui/fast_unlo...
chrome/browser/ui/fast_unload_controller.cc:418: // Null check render_view_host
here as this gets called on a PostTask
On 2015/04/21 17:28:08, Charlie Reis wrote:
> I'm having trouble imagining how the WebContents could have a null RVH without
> being deleted.  That said, we do have RVH null checks scattered throughout
> WebContents.  Can you remind me why they're necessary?  (This might be a good
> thing for Lucas to mention in his doc, or else something we can clean up.)

I don't know what the reason is. I think changing this belongs to another CL and
it is probably something we should try to clean up.

https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:477:
WebContentsImpl::FromRenderFrameHost(render_frame_host_)->ClosePage();
On 2015/04/21 05:13:32, dgozman wrote:
> if (web_contents())
>   web_contents()->ClosePage();

That looks fine to me. What I found strange is few places in this file use
WebContents* web_contents = GetWebContents(); which just returns web_contents().
Should this be cleaned up?

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:229: void ClosePage();
On 2015/04/21 17:28:08, Charlie Reis wrote:
> This needs a comment now that it's not inherited.  Something like what we had
in
> render_view_host.h, but I think the ResourceDispatcherHost mention was stale.

Done.

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_unittest.cc:101:
static_cast<RenderViewHostImpl*>(rvh())->ClosePage();
On 2015/04/21 17:28:08, Charlie Reis wrote:
> Would test_rvh() work?

Done.

https://codereview.chromium.org/1094153002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:685:
static_cast<RenderViewHostImpl*>(GetRenderViewHost())->ClosePage();
On 2015/04/21 17:28:08, Charlie Reis wrote:
> GetRenderViewHostImpl() would take care of the cast for you.
> 
> (I may do another CL after this to kill GetRenderViewHostImpl and just make
> GetRenderViewHost have a covariant return type, but no need to do that here.)

Thank you!

https://codereview.chromium.org/1094153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4010: void
WebContentsImpl::RouteCloseEvent(RenderViewHost* rvh) {
On 2015/04/21 17:28:08, Charlie Reis wrote:
> nit: Blank line before, as long as we're changing this method.

Done.

https://codereview.chromium.org/1094153002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1094153002/diff/1/content/public/browser/web_...
content/public/browser/web_contents.h:225: // ResourceDispatcherHost when it is
finished.
On 2015/04/21 17:28:09, Charlie Reis wrote:
> This last sentence needs updating.  I think the ACK goes to RVH and ends up
> calling Close(rvh) on this WebContents, right?

Done.

https://codereview.chromium.org/1094153002/diff/1/extensions/browser/process_...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/1094153002/diff/1/extensions/browser/process_...
extensions/browser/process_manager.cc:809: // RenderViewHost::ClosePage() may
result in calling
On 2015/04/21 17:28:09, Charlie Reis wrote:
> nit: Update comment.

Done.

[Devlin, 2015-04-29 17:15:46]

extensions lgtm

[dgozman, 2015-04-29 17:23:42]

https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:477:
WebContentsImpl::FromRenderFrameHost(render_frame_host_)->ClosePage();
On 2015/04/29 17:09:27, nasko wrote:
> On 2015/04/21 05:13:32, dgozman wrote:
> > if (web_contents())
> >   web_contents()->ClosePage();
> 
> That looks fine to me. What I found strange is few places in this file use
> WebContents* web_contents = GetWebContents(); which just returns
web_contents().
> Should this be cleaned up?

web_contents() is a protected method of WCO, and we need a public one, which is
called GetWebContents(). I personally don't have preference for one over
another, but I'm open for improvements if you have any suggestions.

[nasko, 2015-04-29 17:24:32]

On 2015/04/29 17:23:42, dgozman wrote:
>
https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
> File content/browser/devtools/render_frame_devtools_agent_host.cc (right):
> 
>
https://codereview.chromium.org/1094153002/diff/1/content/browser/devtools/re...
> content/browser/devtools/render_frame_devtools_agent_host.cc:477:
> WebContentsImpl::FromRenderFrameHost(render_frame_host_)->ClosePage();
> On 2015/04/29 17:09:27, nasko wrote:
> > On 2015/04/21 05:13:32, dgozman wrote:
> > > if (web_contents())
> > >   web_contents()->ClosePage();
> > 
> > That looks fine to me. What I found strange is few places in this file use
> > WebContents* web_contents = GetWebContents(); which just returns
> web_contents().
> > Should this be cleaned up?
> 
> web_contents() is a protected method of WCO, and we need a public one, which
is
> called GetWebContents(). I personally don't have preference for one over
> another, but I'm open for improvements if you have any suggestions.

Well, internally the class has access to web_contents(), so no reason to call
GetWebContents, right?

[Charlie Reis, 2015-04-29 17:31:18]

Thanks.  LGTM with nits.

https://codereview.chromium.org/1094153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/1094153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:229: // Tells the renderer
process to run the page's unload handler. An ack is sent
An ack -> A ClosePage_ACK

https://codereview.chromium.org/1094153002/diff/40001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1094153002/diff/40001/content/public/browser/...
content/public/browser/web_contents.h:224: // handler.  A ClosePage_ACK message
is sent when it is finished.
We probably don't need to mention the ACK here, since it's not exposed by the
public WebContents API.  (Mentioning it in RenderViewHostImpl::ClosePage does
make sense.)

https://codereview.chromium.org/1094153002/diff/40001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/1094153002/diff/40001/extensions/browser/proc...
extensions/browser/process_manager.cc:808:
content::WebContents::FromRenderFrameHost(frame)->ClosePage();
The interstitial CL reminded me that this might return null.  Should we add a
null check?

[Lei Zhang, 2015-04-29 20:04:26]

You still have a bot that's failing to compile.

[Lei Zhang, 2015-04-29 20:04:42]

chrome/ lgtm

[nasko, 2015-04-29 21:14:02]

Patchset #4 (id:60001) has been deleted

[nasko, 2015-04-29 21:15:33]

Lei: the cast bot was a new one, so it didn't fail before. It is fixed now.

https://codereview.chromium.org/1094153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/1094153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:229: // Tells the renderer
process to run the page's unload handler. An ack is sent
On 2015/04/29 17:31:18, Charlie Reis wrote:
> An ack -> A ClosePage_ACK

Done.

https://codereview.chromium.org/1094153002/diff/40001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1094153002/diff/40001/content/public/browser/...
content/public/browser/web_contents.h:224: // handler.  A ClosePage_ACK message
is sent when it is finished.
On 2015/04/29 17:31:18, Charlie Reis wrote:
> We probably don't need to mention the ACK here, since it's not exposed by the
> public WebContents API.  (Mentioning it in RenderViewHostImpl::ClosePage does
> make sense.)

Done.

https://codereview.chromium.org/1094153002/diff/40001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/1094153002/diff/40001/extensions/browser/proc...
extensions/browser/process_manager.cc:808:
content::WebContents::FromRenderFrameHost(frame)->ClosePage();
On 2015/04/29 17:31:18, Charlie Reis wrote:
> The interstitial CL reminded me that this might return null.  Should we add a
> null check?

Should we ever be getting a background page RFH that has interstitial page? I
don't think it is possible, so I'm inclined to leave this as is and monitor if
we get any crashes.

[nasko, 2015-04-29 21:17:43]

nasko@chromium.org changed reviewers:
+ lcwu@chromium.org

[nasko, 2015-04-29 21:17:43]

lcwu@, can you do OWNERS review in chromecast/?

[lcwu1, 2015-04-29 21:45:53]

chromecast/ lgtm.

[gunsch, 2015-04-29 21:51:06]

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

[gunsch, 2015-04-29 21:51:06]

for chromecast/, cast_window_android.cc also has a call to ClosePage that needs
to be updated (though it looks like the trybot is catching that).

[Charlie Reis, 2015-04-29 22:21:48]

https://codereview.chromium.org/1094153002/diff/40001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/1094153002/diff/40001/extensions/browser/proc...
extensions/browser/process_manager.cc:808:
content::WebContents::FromRenderFrameHost(frame)->ClosePage();
On 2015/04/29 21:15:33, nasko wrote:
> On 2015/04/29 17:31:18, Charlie Reis wrote:
> > The interstitial CL reminded me that this might return null.  Should we add
a
> > null check?
> 
> Should we ever be getting a background page RFH that has interstitial page? I
> don't think it is possible, so I'm inclined to leave this as is and monitor if
> we get any crashes.

I hadn't thought of a specific case it could happen; it just seemed unclear if
that was an invariant you intended to enforce.  Feel free to leave and monitor
for crashes.

[nasko, 2015-04-30 15:48:49]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-04-30 15:48:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, thestig@chromium.org, creis@chromium.org,
rdevlin.cronin@chromium.org, lcwu@chromium.org
Link to the patchset: https://codereview.chromium.org/1094153002/#ps100001
(title: "Fix chromecast android compile failure.")

[commit-bot: I haz the power, 2015-04-30 15:49:10]

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

[commit-bot: I haz the power, 2015-04-30 15:54:02]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-04-30 15:55:37]

Patchset 5 (id:??) landed as
https://crrev.com/c0fceff6214f05360be23a5fc2ee578e9d7707aa
Cr-Commit-Position: refs/heads/master@{#327719}