Fix browser launch on the login screen.

Fix browser launch on the login screen.

For the login screen, if there is some web content displayed, and the user clicks a link that is specified to open in a new window, a new browser window indeed opens, which is highly undesirable at the login screen.
To avoid this, a new delegate method was introduced, which should decide if we want to allow displaying any additional contents.

BUG=443096
TEST=manual

Committed: https://crrev.com/747c10c3711081eac5161630dbbd18c3fb454433
Cr-Commit-Position: refs/heads/master@{#314311}

[Ivan Podogov, 2014-12-17 14:31:39]

ginkage@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Ivan Podogov, 2014-12-18 14:24:17]

PTAL

[dzhioev (left Google), 2014-12-22 18:30:13]

https://codereview.chromium.org/812763003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_web_dialog.h (right):

https://codereview.chromium.org/812763003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_web_dialog.h:86: virtual bool
HandleOpenURLFromTab(
Is this method still needed?

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
ui/web_dialogs/web_dialog_delegate.h:134: virtual bool
HandleShouldCreateWebContents(
I don't think we need to pass all the arguments of ShouldCreateWebContents to
HandleShouldCreateWebContents. For our purpose we don't need any parameters at
all.

[Ivan Podogov, 2014-12-23 06:54:52]

https://codereview.chromium.org/812763003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_web_dialog.h (right):

https://codereview.chromium.org/812763003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_web_dialog.h:86: virtual bool
HandleOpenURLFromTab(
On 2014/12/22 18:30:13, dzhioev wrote:
> Is this method still needed?

Yes, so that we could suppress OpenURL message originating from auto-reload.
Without this method, a browser window still opens when extension is unavailable.

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
ui/web_dialogs/web_dialog_delegate.h:134: virtual bool
HandleShouldCreateWebContents(
On 2014/12/22 18:30:13, dzhioev wrote:
> I don't think we need to pass all the arguments of ShouldCreateWebContents to
> HandleShouldCreateWebContents. For our purpose we don't need any parameters at
> all.

True, but I'm not sure if it is a good idea to leave no parameters at all, and
which we should keep otherwise.

[dzhioev (left Google), 2014-12-23 14:34:25]

https://codereview.chromium.org/812763003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_web_dialog.h (right):

https://codereview.chromium.org/812763003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_web_dialog.h:86: virtual bool
HandleOpenURLFromTab(
On 2014/12/23 06:54:51, Ivan Podogov wrote:
> On 2014/12/22 18:30:13, dzhioev wrote:
> > Is this method still needed?
> 
> Yes, so that we could suppress OpenURL message originating from auto-reload.
> Without this method, a browser window still opens when extension is
unavailable.

I'm not quiet sure if such behaviour is correct. Let's add somebody from
content/OWNERS to the review.

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
ui/web_dialogs/web_dialog_delegate.h:134: virtual bool
HandleShouldCreateWebContents(
On 2014/12/23 06:54:51, Ivan Podogov wrote:
> On 2014/12/22 18:30:13, dzhioev wrote:
> > I don't think we need to pass all the arguments of ShouldCreateWebContents
to
> > HandleShouldCreateWebContents. For our purpose we don't need any parameters
at
> > all.
> 
> True, but I'm not sure if it is a good idea to leave no parameters at all, and
> which we should keep otherwise.

Let's drop all of them. It is not hard to add parameters later, if somebody need
them.

[Ivan Podogov, 2014-12-23 14:34:35]

ginkage@chromium.org changed reviewers:
+ jochen@chromium.org

[Ivan Podogov, 2014-12-23 14:41:37]

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/812763003/diff/20001/ui/web_dialogs/web_dialo...
ui/web_dialogs/web_dialog_delegate.h:134: virtual bool
HandleShouldCreateWebContents(
On 2014/12/23 14:34:25, dzhioev wrote:
> On 2014/12/23 06:54:51, Ivan Podogov wrote:
> > On 2014/12/22 18:30:13, dzhioev wrote:
> > > I don't think we need to pass all the arguments of ShouldCreateWebContents
> to
> > > HandleShouldCreateWebContents. For our purpose we don't need any
parameters
> at
> > > all.
> > 
> > True, but I'm not sure if it is a good idea to leave no parameters at all,
and
> > which we should keep otherwise.
> 
> Let's drop all of them. It is not hard to add parameters later, if somebody
need
> them.

Done.

[dzhioev (left Google), 2014-12-24 08:14:42]

LGTM

[Ivan Podogov, 2014-12-25 13:45:05]

jochen@, please owner review.

[ginkage, 2015-02-02 10:26:16]

ginkage@google.com changed reviewers:
+ sadrul@chromium.org
- jochen@chromium.org

[ginkage, 2015-02-02 10:26:46]

ginkage@google.com changed reviewers:
+ ginkage@google.com

[ginkage, 2015-02-02 10:26:46]

sadrul@, please owner review.

[Ivan Podogov, 2015-02-02 10:29:14]

ginkage@chromium.org changed reviewers:
- ginkage@google.com

[sadrul, 2015-02-03 02:45:21]

https://codereview.chromium.org/812763003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_web_dialog.cc (right):

https://codereview.chromium.org/812763003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:165: return (source &&
!chrome::FindBrowserWithWebContents(source));
Can you add a comment here explaining what the code does?

https://codereview.chromium.org/812763003/diff/40001/ui/views/controls/webvie...
File ui/views/controls/webview/web_dialog_view.cc (right):

https://codereview.chromium.org/812763003/diff/40001/ui/views/controls/webvie...
ui/views/controls/webview/web_dialog_view.cc:347: }
return delegate_ ? delegate_->HandleShouldCreateWebContents() : true;

[Ivan Podogov, 2015-02-03 08:09:08]

Nits fixed.

https://codereview.chromium.org/812763003/diff/40001/ui/views/controls/webvie...
File ui/views/controls/webview/web_dialog_view.cc (right):

https://codereview.chromium.org/812763003/diff/40001/ui/views/controls/webvie...
ui/views/controls/webview/web_dialog_view.cc:347: }
On 2015/02/03 02:45:21, sadrul wrote:
> return delegate_ ? delegate_->HandleShouldCreateWebContents() : true;

I'm really into ternary operations as well, although I prefer to use them a
little less in a shared code. :)

[sadrul, 2015-02-03 08:11:47]

lgtm

[Ivan Podogov, 2015-02-03 10:29:25]

The CQ bit was checked by ginkage@chromium.org

[commit-bot: I haz the power, 2015-02-03 10:29:53]

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

[commit-bot: I haz the power, 2015-02-03 11:15:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-03 11:16:06]

Patchset 4 (id:??) landed as
https://crrev.com/747c10c3711081eac5161630dbbd18c3fb454433
Cr-Commit-Position: refs/heads/master@{#314311}