Fix regression due to navigation refactoring

Fix regression due to navigation refactoring

This fixes a regression introduced by CL
https://chromiumcodereview.appspot.com/379143002/.

BUG=405812, 408684
Committed: https://crrev.com/b5ddc3c48acb931e248444bec6a9317df06f9905
Cr-Commit-Position: refs/heads/master@{#295274}

[clamy, 2014-08-21 12:22:56]

@nasko: PTAL. This fixes a regression introduced by the previous PlzNavigate CL
(the cloud print dialog window was not shown anymore).

[nasko, 2014-08-21 13:50:36]

Codewise it looks good, but can you describe what the problem was? It isn't
immediately obvious.

[clamy, 2014-08-21 16:01:04]

I've investigated the problem a little more, and it seems to be the following:
Navigator is asked to navigate to an entry, with the url
chrome://cloudprintresources/. It calls RFHM::Navigate, which ends up creating a
new RenderView. The creation of the RenderView triggers a call to
internal_cloud_print_helpers::CloudPrintFlowHandler::RegisterMessages (through
WebUIImpl::AddMessageHandler). This function then sets the url of the pending
entry in the NavigationController to
https://www.google.com/cloudprint/client/dialog.html?hl=en-US. So when
RFHM::Navigate returns, the url in the entry has changed.

With my previous patch, we created the FrameMsg_Navigate_Params before the call
to RFHM::Navigate instead of after, so we ask to navigate to the original url
(which returns an error). I am a bit wondering about the behavior of cloudprint
there. It seems we will end up using a RFH with an unrelated site instance and
WebUI privileges to display an external url. Should I go with this patch that
fixes the apparent issue, or ask the cloudprint people to change their code?

[Charlie Reis, 2014-08-21 16:05:11]

On 2014/08/21 16:01:04, clamy wrote:
> I've investigated the problem a little more, and it seems to be the following:
> Navigator is asked to navigate to an entry, with the url
> chrome://cloudprintresources/. It calls RFHM::Navigate, which ends up creating
a
> new RenderView. The creation of the RenderView triggers a call to
> internal_cloud_print_helpers::CloudPrintFlowHandler::RegisterMessages (through
> WebUIImpl::AddMessageHandler). This function then sets the url of the pending
> entry in the NavigationController to
> https://www.google.com/cloudprint/client/dialog.html?hl=en-US. So when
> RFHM::Navigate returns, the url in the entry has changed.
> 
> With my previous patch, we created the FrameMsg_Navigate_Params before the
call
> to RFHM::Navigate instead of after, so we ask to navigate to the original url
> (which returns an error). I am a bit wondering about the behavior of
cloudprint
> there. It seems we will end up using a RFH with an unrelated site instance and
> WebUI privileges to display an external url. Should I go with this patch that
> fixes the apparent issue, or ask the cloudprint people to change their code?

Yikes.  Yeah, your concern about the cloudprint behavior is completely right--
it sounds like they're loading a web page into a WebUI SiteInstance (scary). 
This CL is probably ok as a regression fix, but we need to address that
behavior.  Can you check with them?

[Vitaly Buka (NO REVIEWS), 2014-09-16 18:26:15]

nasko, creis: Would you like to approve this change? I guess we agreed that we
are going to remove this in near future releases.
Branch is soon, we'd like to verify fixed before that.

[Charlie Reis, 2014-09-16 21:13:20]

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

[Charlie Reis, 2014-09-16 21:13:20]

Yes, now that we have a bug on file to move Cloud Print away from the WebUI
issue, this LGTM as a regression fix.  Let's mention the bug to make sure we
don't lose track of it.

https://chromiumcodereview.appspot.com/494083002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/494083002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:382: // Create the navigation
parameters.
Please add something like:
// TODO(vitalybuka): Move this before AboutToNavigateRenderFrame once
http://crbug.com/408684 is fixed.

[clamy, 2014-09-17 13:33:46]

Thanks!

https://chromiumcodereview.appspot.com/494083002/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/494083002/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:382: // Create the navigation
parameters.
On 2014/09/16 21:13:20, Charlie Reis wrote:
> Please add something like:
> // TODO(vitalybuka): Move this before AboutToNavigateRenderFrame once
> http://crbug.com/408684 is fixed.

Done.

[clamy, 2014-09-17 13:33:50]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-09-17 13:33:59]

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

[commit-bot: I haz the power, 2014-09-17 14:40:25]

Committed patchset #2 (id:20001) as d43b65de6767c1881612db32907fff518c4b8971

[commit-bot: I haz the power, 2014-09-17 14:41:04]

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