Let the browser handle external navigations from DevTools.

Let the browser handle external navigations from DevTools.

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

[vsevik, 2013-03-06 13:49:28]

pfeldman@: Could you please review this?

jamesr@: Could you please OWNER review this?

cevans@: Could you please security review this?

[pfeldman, 2013-03-06 14:00:35]

https://codereview.chromium.org/12531004/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12531004/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3058:
old_url.SchemeIs(chrome::kChromeDevToolsScheme);
Charlie: can we do
HasWebUIScheme(old_url) instead to cover all such cases? I.e. we imply that any
WebUIScheme URL might have 
BINDINGS_POLICY_WEB_UI. Is check for cumulative_bindings above necessary then?

[Charlie Reis, 2013-03-06 18:18:25]

https://codereview.chromium.org/12531004/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12531004/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3058:
old_url.SchemeIs(chrome::kChromeDevToolsScheme);
On 2013/03/06 14:00:35, pfeldman wrote:
> Charlie: can we do
> HasWebUIScheme(old_url) instead to cover all such cases? I.e. we imply that
any
> WebUIScheme URL might have 
> BINDINGS_POLICY_WEB_UI.

This seems ok, because it's broader than before.  We will now send navigations
to the browser for WebUI schemes that don't have bindings, in addition to those
that have bindings.  I can't think of cases that will break, and we'll likely
hear about it if it does cause problems.


> Is check for cumulative_bindings above necessary then?

We definitely cannot remove the cumulative_bindings check, even if we do that. 
This is an important defense in depth that covers cases where bindings were
inadvertently granted.  Please leave it in place.

[vsevik, 2013-03-06 19:06:52]

Charlie, could you please take another look then?

[Charlie Reis, 2013-03-06 19:24:27]

Is it possible to write a DevTools test for this?  We're planning to refactor
this logic and I don't want to regress your case without realizing it.

https://codereview.chromium.org/12531004/diff/8001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12531004/diff/8001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3053: HasWebUIScheme(old_url) ||
nit: Please move above previous line, so that the two HasWebUIScheme calls are
near each other.

[vsevik, 2013-03-07 11:34:12]

PTAL

[pfeldman, 2013-03-07 15:24:56]

devtools lgtm

[Charlie Reis, 2013-03-07 18:12:24]

Thanks for the test.  LGTM.

[commit-bot: I haz the power, 2013-03-07 18:21:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vsevik@chromium.org/12531004/12001

[commit-bot: I haz the power, 2013-03-07 22:19:02]

Change committed as 186793