Remove RFHM::GetNavigatingWebUI() and RFHM::web_ui().

Remove RFHM::GetNavigatingWebUI() and RFHM::web_ui().

After the removal of the WebUI from RFHM and of the pending WebUI these helper
methods didn't make much sense anymore.

This is based off of https://codereview.chromium.org/1352813006

BUG=508850

[carlosk, 2015-10-30 18:55:43]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[carlosk, 2015-10-30 18:55:44]

clamy@, nasko@: PTAL.

Removing the remaining WebUI getters from RFHM. This still depends on
https://codereview.chromium.org/1352813006 landing first.

https://codereview.chromium.org/1418853003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1418853003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:858: WebUI*
WebContentsImpl::GetWebUI() const {
I kept the implementation of GetWebUI() working the same as it was before and it
matches the description in the public WebContents API. But IMO its name is very
misleading.

[clamy, 2015-11-03 12:30:43]

Thanks! Lgtm, I find code is more readable now.

[carlosk, 2015-11-04 10:05:09]

On 2015/11/03 12:30:43, clamy wrote:
> Thanks! Lgtm, I find code is more readable now.

Thanks clamy@.

nasko@, could you PTAL?

[nasko, 2015-11-04 17:23:26]

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:663: // proper focus
setting).
Is the detail about focus setting important?

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2105:
DCHECK(!current_web_ui_is_navigating_ || render_frame_host_->web_ui());
I thought this variable is only needed to support GetNavigatingWebUI.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1551: //
clear GetNavigatingWebUI() when it commits.
This is stale comment now. GetNavigatingWebUI doesn't exist anymore.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:846: return
GetRenderManager()->pending_frame_host()->web_ui();
This version won't work with PlzNavigate, would it?

https://codereview.chromium.org/1418853003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:920: :
frame_tree_.root()->current_frame_host()->web_ui();
Seems inconsistent to use GetRenderManager() for two of the three calls here.
Let's be consistent.

[carlosk, 2015-11-04 21:50:38]

Thanks.

This will still need a rebase to catch up on the updates in the base change but
I'll do that tomorrow.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:663: // proper focus
setting).
On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> Is the detail about focus setting important?

I chose to mention it here because I spent some time on a request by creis@ [1]
to make this method care *only* about cross-site navigations and move all
current RFH commit logic to its own RFHI::OnDidCommitProvisionalLoad. But that
effort failed precisely because I didn't notice this detail until later on.

Finally, I added this note to facilitate a next effort to execute that change.

[1]
https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_...

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2105:
DCHECK(!current_web_ui_is_navigating_ || render_frame_host_->web_ui());
On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> I thought this variable is only needed to support GetNavigatingWebUI.

Oops! My bad! I forgot to remove it but it was already not doing anything really
useful.

Done now.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1551: //
clear GetNavigatingWebUI() when it commits.
On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> This is stale comment now. GetNavigatingWebUI doesn't exist anymore.

Done.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:846: return
GetRenderManager()->pending_frame_host()->web_ui();
On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> This version won't work with PlzNavigate, would it?

Indeed and nice catch! Fixed.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:920: :
frame_tree_.root()->current_frame_host()->web_ui();
On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> Seems inconsistent to use GetRenderManager() for two of the three calls here.
> Let's be consistent.

And it didn't support PlzNavigate neither. All fixed now.

(in fact I tried using GetMainFrame before but that one can't be used here
because it's not const)

[nasko, 2015-11-05 00:46:43]

Looks good. I'll stamp it once the base CL lands and this one is cleanly rebased
and passes the bots.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:663: // proper focus
setting).
On 2015/11/04 21:50:38, carlosk wrote:
> On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> > Is the detail about focus setting important?
> 
> I chose to mention it here because I spent some time on a request by creis@
[1]
> to make this method care *only* about cross-site navigations and move all
> current RFH commit logic to its own RFHI::OnDidCommitProvisionalLoad. But that
> effort failed precisely because I didn't notice this detail until later on.
> 
> Finally, I added this note to facilitate a next effort to execute that change.
> 
> [1]
>
https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_...

Let's put that as a separate sentence then explaining what the pitfall might be.
As it is, it doesn't seem important and is easily confusing.

[carlosk, 2015-11-05 13:40:42]

Thanks.

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1418853003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:663: // proper focus
setting).
On 2015/11/05 00:46:43, nasko (slow to review) wrote:
> On 2015/11/04 21:50:38, carlosk wrote:
> > On 2015/11/04 17:23:26, nasko (slow to review) wrote:
> > > Is the detail about focus setting important?
> > 
> > I chose to mention it here because I spent some time on a request by creis@
> [1]
> > to make this method care *only* about cross-site navigations and move all
> > current RFH commit logic to its own RFHI::OnDidCommitProvisionalLoad. But
that
> > effort failed precisely because I didn't notice this detail until later on.
> > 
> > Finally, I added this note to facilitate a next effort to execute that
change.
> > 
> > [1]
> >
>
https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_...
> 
> Let's put that as a separate sentence then explaining what the pitfall might
be.
> As it is, it doesn't seem important and is easily confusing.

Done.

[Charlie Reis, 2015-11-09 07:02:01]

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

[Charlie Reis, 2015-11-09 07:02:02]

This is great.  Much nicer not to have the WebUI accessors on RFHM anymore.

I'm not entirely sure why this wasn't included in the main CL
(https://codereview.chromium.org/1426403006/), since that would have avoided
introducing things like GetNavigatingWebUI() just to later remove them.  I
suppose I'm ok with it being separate if it felt like a smaller delta, but I
wouldn't mind seeing them combined either.

Just nits below, and I'll defer to Nasko's stamp once he's ready.

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:660: // If the current
RenderFrameHost has a WebUI it must be committed
nit: End sentence with period.

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:661: // Note: for when
one tries to move same-site commit logic into
nit: s/for when/If/

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:662: // RenderFrameHost
itself mind that the focus setting logic inside
nit: comma after "itself"

[carlosk, 2015-11-10 10:28:55]

On 2015/11/09 07:02:02, Charlie Reis (slow til 11-16) wrote:
> I'm not entirely sure why this wasn't included in the main CL
> (https://codereview.chromium.org/1426403006/), since that would have avoided
> introducing things like GetNavigatingWebUI() just to later remove them.  I
> suppose I'm ok with it being separate if it felt like a smaller delta, but I
> wouldn't mind seeing them combined either.

As I had to revert the base patch I'll work on this round of comments here and
then merge this patch into the new re-land one. But yes, as I got some comments
that the original patch was getting to big I did this split to keep that one
smaller.

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:660: // If the current
RenderFrameHost has a WebUI it must be committed
On 2015/11/09 07:02:02, Charlie Reis (slow til 11-16) wrote:
> nit: End sentence with period.

Done.

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:661: // Note: for when
one tries to move same-site commit logic into
On 2015/11/09 07:02:02, Charlie Reis (slow til 11-16) wrote:
> nit: s/for when/If/

Done.

https://codereview.chromium.org/1418853003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:662: // RenderFrameHost
itself mind that the focus setting logic inside
On 2015/11/09 07:02:02, Charlie Reis (slow til 11-16) wrote:
> nit: comma after "itself"

Done.

[carlosk, 2015-11-10 10:30:00]

Closing this one as it will be merged into
https://codereview.chromium.org/1426403006/

[carlosk, 2015-11-10 10:30:11]

Description was changed from

==========
Remove RFHM::GetNavigatingWebUI() and RFHM::web_ui().

After the removal of the WebUI from RFHM and of the pending WebUI these helper
methods didn't make much sense anymore.

This is based off of https://codereview.chromium.org/1352813006

BUG=508850
==========

to

==========
Remove RFHM::GetNavigatingWebUI() and RFHM::web_ui().

After the removal of the WebUI from RFHM and of the pending WebUI these helper
methods didn't make much sense anymore.

This is based off of https://codereview.chromium.org/1352813006

BUG=508850
==========