Add a browser test for the views external protocol dialog.

Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only run on Linux,
Windows, and MacViews.

BUG=671658
Review-Url: https://codereview.chromium.org/2610793002
Cr-Commit-Position: refs/heads/master@{#441869}
Committed: https://chromium.googlesource.com/chromium/src/+/6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507

[dominickn, 2017-01-04 04:16:48]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 04:17:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-01-04 04:24:52]

Description was changed from

==========
Add a browser test for the external protocol dialog on views.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

BUG=671658
==========

to

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

BUG=671658
==========

[dominickn, 2017-01-04 04:24:52]

dominickn@chromium.org changed reviewers:
+ msw@chromium.org

[dominickn, 2017-01-04 04:29:39]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 04:29:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-01-04 04:45:39]

Description was changed from

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

BUG=671658
==========

to

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only available on
Linux, Windows, and MacViews.

BUG=671658
==========

[dominickn, 2017-01-04 04:50:32]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 04:50:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 05:31:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 05:31:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dominickn, 2017-01-04 05:46:46]

Description was changed from

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only available on
Linux, Windows, and MacViews.

BUG=671658
==========

to

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only run on Linux,
Windows, and MacViews.

BUG=671658
==========

[dominickn, 2017-01-04 05:46:50]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 05:47:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-01-04 06:16:16]

PTAL, thanks! This is a follow-up for crrev.com/2559783003 (which I'll land and
merge to M56 if you're okay with the testing here).

[commit-bot: I haz the power, 2017-01-04 06:53:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 06:53:21]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-04 23:32:48]

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

[tapted, 2017-01-04 23:32:49]

I think msw is OOO for another day, so you get a drive-by :)

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.h (right):

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.h:28: void
SetCheckBoxSelectedForTesting(bool checked);
instead of this, you can do

namespace test {
class ExternalProtocolDialogTestApi;
}

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.h:44: const
std::unique_ptr<const ProtocolDialogDelegate> delegate_;
friend class test::ExternalProtocolDialogTestApi;

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
nit: no (c). Also, 2017 :)

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:15: 
namespace test {

class ExternalProtocolDialogTestApi {
 public:
  explicit ExternalProtocolDialogTestApi(ExternalProtocolDialog* dialog)
      : dialog_(dialog) {}

  void SetCheckBoxSelected(bool checked) {
    dialog_->message_box_view_->SetCheckBoxSelected(checked);
  }
};

}  // namespace test

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:30: void
DoAccept(const GURL& url, bool dont_block) const override {
nit: // ExternalProtocolDialogDelegate:

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:46: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:51: :
InProcessBrowserTest(),
this superclass constructor call isn't needed - the rest I would just initialize
at the declarations below

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:70:
dialog_->SetCheckBoxSelectedForTesting(checked);
ExternalProtocolDialogTestApi(dialog_).SetCheckBoxSelected(checked);

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:74:
ExternalProtocolDialog* dialog_;
i.e. = nullptr, etc.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:78: };
 private:
  DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:81:
ShowDialog(GURL("https://www.google.com"));
since it's always google.com, can we just call ShowDialog() without arguments?

https://codereview.chromium.org/2610793002/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (left):

https://codereview.chromium.org/2610793002/diff/60001/chrome/test/BUILD.gn#ol...
chrome/test/BUILD.gn:3128:
"../browser/external_protocol/external_protocol_handler_unittest.cc",
why remove this?

[dominickn, 2017-01-04 23:51:16]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-01-04 23:51:36]

Thanks tapted!

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.h (right):

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.h:28: void
SetCheckBoxSelectedForTesting(bool checked);
On 2017/01/04 23:32:49, tapted wrote:
> instead of this, you can do
> 
> namespace test {
> class ExternalProtocolDialogTestApi;
> }

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.h:44: const
std::unique_ptr<const ProtocolDialogDelegate> delegate_;
On 2017/01/04 23:32:49, tapted wrote:
> friend class test::ExternalProtocolDialogTestApi;

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2017/01/04 23:32:49, tapted wrote:
> nit: no (c). Also, 2017 :)

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:15: 
On 2017/01/04 23:32:49, tapted wrote:
> namespace test {
> 
> class ExternalProtocolDialogTestApi {
>  public:
>   explicit ExternalProtocolDialogTestApi(ExternalProtocolDialog* dialog)
>       : dialog_(dialog) {}
> 
>   void SetCheckBoxSelected(bool checked) {
>     dialog_->message_box_view_->SetCheckBoxSelected(checked);
>   }
> };
> 
> }  // namespace test

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:30: void
DoAccept(const GURL& url, bool dont_block) const override {
On 2017/01/04 23:32:49, tapted wrote:
> nit: // ExternalProtocolDialogDelegate:

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:46: };
On 2017/01/04 23:32:49, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:51: :
InProcessBrowserTest(),
On 2017/01/04 23:32:49, tapted wrote:
> this superclass constructor call isn't needed - the rest I would just
initialize
> at the declarations below

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:70:
dialog_->SetCheckBoxSelectedForTesting(checked);
On 2017/01/04 23:32:49, tapted wrote:
> ExternalProtocolDialogTestApi(dialog_).SetCheckBoxSelected(checked);

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:74:
ExternalProtocolDialog* dialog_;
On 2017/01/04 23:32:49, tapted wrote:
> i.e. = nullptr, etc.

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:78: };
On 2017/01/04 23:32:49, tapted wrote:
>  private:
>   DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/2610793002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:81:
ShowDialog(GURL("https://www.google.com"));
On 2017/01/04 23:32:49, tapted wrote:
> since it's always http://google.com, can we just call ShowDialog() without
arguments?

I made it a URL argument for future extensibility, but I guess that can happen
when it happens.

https://codereview.chromium.org/2610793002/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (left):

https://codereview.chromium.org/2610793002/diff/60001/chrome/test/BUILD.gn#ol...
chrome/test/BUILD.gn:3128:
"../browser/external_protocol/external_protocol_handler_unittest.cc",
On 2017/01/04 23:32:49, tapted wrote:
> why remove this?

Oops! My new test was originally a unit test, so I got confused when I converted
it to a browser test.

[commit-bot: I haz the power, 2017-01-04 23:51:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-05 00:47:37]

lgtm, but you'll still need msw for OWNERS :)

(Once it lands, hopefully I can hook this in to `TestBrowserDialog` as well -
see https://codereview.chromium.org/2532793002/ - but I'll need to add tweaks to
support non-modal/parentless dialogs ... although I guess also this dialog
should really be tab-modal anyway - argh. Poor neglected dialogs... Mac's
ExternalProtocolDialogController is even more dire - I should kill it, but that
might be blocked on making it tab-modal :/)

[msw, 2017-01-05 01:32:16]

Just some minor comments and a question.

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:31: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:35: class
TestExternalProtocolDialogDelegate
nit: add a short comment?

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:50: void
DoAccept(const GURL& url, bool dont_block) const override {
q: Should these overrides call the base class impls?

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:118:
IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestCancel) {
It seems like we should have an explicit flag for cancel, so it can be
distinguished from the test cases that just close the dialog without accepting
or cancelling it.

[commit-bot: I haz the power, 2017-01-05 01:54:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 01:54:30]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-05 02:00:28]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-01-05 02:00:50]

Thanks!

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:31: };
On 2017/01/05 01:32:16, msw (vacation jan 4 maybe 5) wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:35: class
TestExternalProtocolDialogDelegate
On 2017/01/05 01:32:16, msw (vacation jan 4 maybe 5) wrote:
> nit: add a short comment?

Done.

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:50: void
DoAccept(const GURL& url, bool dont_block) const override {
On 2017/01/05 01:32:16, msw (vacation jan 4 maybe 5) wrote:
> q: Should these overrides call the base class impls?

It's not really important because this test is designed to ensure that the
dialog class tells the delegate the correct values (i.e. |dont_block|).

Calling the base impl also has the somewhat comical side effect of actually
trying to launch the provided URL if the dialog was Accepted. That means it
opens your default browser and sends it to google.com. (I've changed the URL to
be a telnet:// to ensure it actually is an external one now).

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:118:
IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestCancel) {
On 2017/01/05 01:32:15, msw (vacation jan 4 maybe 5) wrote:
> It seems like we should have an explicit flag for cancel, so it can be
> distinguished from the test cases that just close the dialog without accepting
> or cancelling it.

Close() is overridden to call DoCancel() (see crrev.com/2559783003); the test
here is that Close() never sets |dont_block| to be true despite the state of the
checkbox.

[commit-bot: I haz the power, 2017-01-05 02:00:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 03:21:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 03:21:04]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-05 15:57:53]

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:50: void
DoAccept(const GURL& url, bool dont_block) const override {
On 2017/01/05 02:00:50, dominickn wrote:
> On 2017/01/05 01:32:16, msw (vacation jan 4 maybe 5) wrote:
> > q: Should these overrides call the base class impls?
> 
> It's not really important because this test is designed to ensure that the
> dialog class tells the delegate the correct values (i.e. |dont_block|).
> 
> Calling the base impl also has the somewhat comical side effect of actually
> trying to launch the provided URL if the dialog was Accepted. That means it
> opens your default browser and sends it to http://google.com. (I've changed
the URL to
> be a telnet:// to ensure it actually is an external one now).

Acknowledged.

https://codereview.chromium.org/2610793002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:118:
IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestCancel) {
On 2017/01/05 02:00:50, dominickn wrote:
> On 2017/01/05 01:32:15, msw (vacation jan 4 maybe 5) wrote:
> > It seems like we should have an explicit flag for cancel, so it can be
> > distinguished from the test cases that just close the dialog without
accepting
> > or cancelling it.
> 
> Close() is overridden to call DoCancel() (see crrev.com/2559783003); the test
> here is that Close() never sets |dont_block| to be true despite the state of
the
> checkbox.

Ack. I still suggest testing for |cancel|

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:37: // Wrapper
dialog delegate that sets |called|, |accept|, |cancel|, and
Implement |cancel| or remove its mention.

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:87:
GURL("telnet://29128"), render_process_host_id, routing_id,
I'm curious what this points to, hopefully something innocuous, even though I
know it's not being accessed by test execution.

[dominickn, 2017-01-05 23:11:44]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-01-05 23:11:56]

Now explicitly checking for Cancel. PTAL, thanks!

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:37: // Wrapper
dialog delegate that sets |called|, |accept|, |cancel|, and
On 2017/01/05 15:57:53, msw (vacation jan 4 maybe 5) wrote:
> Implement |cancel| or remove its mention.

Done.

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:87:
GURL("telnet://29128"), render_process_host_id, routing_id,
On 2017/01/05 15:57:53, msw (vacation jan 4 maybe 5) wrote:
> I'm curious what this points to, hopefully something innocuous, even though I
> know it's not being accessed by test execution.

This URL is completely arbitrary. telnet URLs are identical to HTTP/HTTPS URLs
aside from the scheme, so having the numbers in there should ensure that it
never goes anywhere relevant.

[commit-bot: I haz the power, 2017-01-05 23:12:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 00:29:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 00:29:58]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-06 00:54:00]

lgtm with a nit

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:87:
GURL("telnet://29128"), render_process_host_id, routing_id,
On 2017/01/05 23:11:55, dominickn wrote:
> On 2017/01/05 15:57:53, msw (vacation jan 4 maybe 5) wrote:
> > I'm curious what this points to, hopefully something innocuous, even though
I
> > know it's not being accessed by test execution.
> 
> This URL is completely arbitrary. telnet URLs are identical to HTTP/HTTPS URLs
> aside from the scheme, so having the numbers in there should ensure that it
> never goes anywhere relevant.

gotcha, maybe do 12345 or something that's more obviously unimportant?

[dominickn, 2017-01-06 02:42:24]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 02:43:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-01-06 02:43:16]

Thanks!

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog_browsertest.cc (right):

https://codereview.chromium.org/2610793002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog_browsertest.cc:87:
GURL("telnet://29128"), render_process_host_id, routing_id,
On 2017/01/06 00:54:00, msw (vacation jan 4 maybe 5) wrote:
> On 2017/01/05 23:11:55, dominickn wrote:
> > On 2017/01/05 15:57:53, msw (vacation jan 4 maybe 5) wrote:
> > > I'm curious what this points to, hopefully something innocuous, even
though
> I
> > > know it's not being accessed by test execution.
> > 
> > This URL is completely arbitrary. telnet URLs are identical to HTTP/HTTPS
URLs
> > aside from the scheme, so having the numbers in there should ensure that it
> > never goes anywhere relevant.
> 
> gotcha, maybe do 12345 or something that's more obviously unimportant?

Done.

[msw, 2017-01-06 03:27:42]

lgtm

[commit-bot: I haz the power, 2017-01-06 04:00:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 04:00:27]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-06 04:03:46]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-01-06 04:03:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2610793002/#ps140001
(title: "Address nit")

[commit-bot: I haz the power, 2017-01-06 04:04:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 04:07:46]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483675426193990,
"parent_rev": "ddcf54461471ed503e3bd02f0d9563108ef1913e", "commit_rev":
"6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507"}

[commit-bot: I haz the power, 2017-01-06 04:08:15]

Description was changed from

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only run on Linux,
Windows, and MacViews.

BUG=671658
==========

to

==========
Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only run on Linux,
Windows, and MacViews.

BUG=671658

Review-Url: https://codereview.chromium.org/2610793002
Cr-Commit-Position: refs/heads/master@{#441869}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f0a6f4bcb0e0b8b06b79259ae66...
==========

[commit-bot: I haz the power, 2017-01-06 04:08:16]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6f0a6f4bcb0e0b8b06b79259ae66...