Don't treat Esc like Cancel in the external protocol dialog on Views.

Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog. Until UI to edit external protocol
settings is implemented, this permanently prevents any dialogs from
the page for that scheme.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

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

[dominickn, 2016-12-08 07:33:58]

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

[commit-bot: I haz the power, 2016-12-08 07:34:10]

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, 2016-12-08 08:26:24]

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

[commit-bot: I haz the power, 2016-12-08 08:26:25]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-12-08 08:58:51]

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

[dominickn, 2016-12-08 08:58:51]

PTAL, thanks!

[dominickn, 2016-12-08 09:00:07]

Description was changed from

==========
Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog.

This CL addresses part of the bug by ensuring that closing the external
protocl dialog without interacting with the buttons ignores the checkbox
state.

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

to

==========
Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

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

[dominickn, 2016-12-08 09:00:48]

Description was changed from

==========
Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

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

to

==========
Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog. Until UI to edit external protocol
settings is implemented, this permanently prevents any dialogs from
the page for that scheme.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

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

[msw, 2016-12-08 21:37:48]

https://codereview.chromium.org/2559783003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2559783003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/external_protocol_dialog.cc:104: bool
ExternalProtocolDialog::Close() {
Add a comment in the function body explaining why this override exists, and why
it shouldn't call the base class Close function. Add a unit test to ensure this
doesn't regress.

[dominickn, 2016-12-12 02:06:21]

On 2016/12/08 21:37:48, msw wrote:
>
https://codereview.chromium.org/2559783003/diff/1/chrome/browser/ui/views/ext...
> File chrome/browser/ui/views/external_protocol_dialog.cc (right):
> 
>
https://codereview.chromium.org/2559783003/diff/1/chrome/browser/ui/views/ext...
> chrome/browser/ui/views/external_protocol_dialog.cc:104: bool
> ExternalProtocolDialog::Close() {
> Add a comment in the function body explaining why this override exists, and
why
> it shouldn't call the base class Close function. Add a unit test to ensure
this
> doesn't regress.

Thanks. I'm currently working on a test; however, this CL will be merged to M56.
In the name of keeping the merge small, do you mind if I upload the test
separately? Happy for you to review both CLs before landing.

[msw, 2016-12-12 18:30:28]

On 2016/12/12 02:06:21, dominickn wrote:
> On 2016/12/08 21:37:48, msw wrote:
> >
>
https://codereview.chromium.org/2559783003/diff/1/chrome/browser/ui/views/ext...
> > File chrome/browser/ui/views/external_protocol_dialog.cc (right):
> > 
> >
>
https://codereview.chromium.org/2559783003/diff/1/chrome/browser/ui/views/ext...
> > chrome/browser/ui/views/external_protocol_dialog.cc:104: bool
> > ExternalProtocolDialog::Close() {
> > Add a comment in the function body explaining why this override exists, and
> why
> > it shouldn't call the base class Close function. Add a unit test to ensure
> this
> > doesn't regress.
> 
> Thanks. I'm currently working on a test; however, this CL will be merged to
M56.
> In the name of keeping the merge small, do you mind if I upload the test
> separately? Happy for you to review both CLs before landing.

Merging a test shouldn't be a problem, but ok, lgtm.

[dominickn, 2016-12-15 05:15:23]

message: On 2016/12/12 18:30:28, msw wrote:
> On 2016/12/12 02:06:21, dominickn wrote:
> > Thanks. I'm currently working on a test; however, this CL will be merged to
> M56.
> > In the name of keeping the merge small, do you mind if I upload the test
> > separately? Happy for you to review both CLs before landing.
> 
> Merging a test shouldn't be a problem, but ok, lgtm.

Thanks! The main reason is that there are currently no tests for the external
protocol handler, so adding a new one involves editing chrome/test/BUILD.gn.
There have been changes since M56 branched around the place where I would have
had to add the test file, so the merge wouldn't have been clean.

As I mentioned, I'll try and have the test up for you to look at before landing
this.

[dominickn, 2017-01-06 02:41:37]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:42:17]

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 03:27:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483670497261200,
"parent_rev": "20e3d92371109d1bf4e2f26040bbaac964794da1", "commit_rev":
"fc29407956cd9349e12be316b7f0007c9b2471ab"}

[commit-bot: I haz the power, 2017-01-06 03:27:45]

Description was changed from

==========
Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog. Until UI to edit external protocol
settings is implemented, this permanently prevents any dialogs from
the page for that scheme.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

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

to

==========
Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog. Until UI to edit external protocol
settings is implemented, this permanently prevents any dialogs from
the page for that scheme.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

BUG=671658

Review-Url: https://codereview.chromium.org/2559783003
Cr-Commit-Position: refs/heads/master@{#441863}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc29407956cd9349e12be316b7f0...
==========

[commit-bot: I haz the power, 2017-01-06 03:27:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/fc29407956cd9349e12be316b7f0...