Use a standard message box for DesktopCastingWarningView.

Use a standard message box for DesktopCastingWarningView.

This is even simpler than what I did for TeleportWarningView, which
means I should probably go back and re-rewrite that one.

BUG=658410
Committed: https://crrev.com/5f1173f1e5a5678cc6c7cc9046939447398eaf19
Cr-Commit-Position: refs/heads/master@{#428440}

[Evan Stade, 2016-10-24 20:14:09]

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

[commit-bot: I haz the power, 2016-10-24 20:14:51]

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-10-24 21:45:48]

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

[commit-bot: I haz the power, 2016-10-24 21:45:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-10-25 22:04:46]

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

[commit-bot: I haz the power, 2016-10-25 22:05:31]

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-10-25 23:34:20]

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

[commit-bot: I haz the power, 2016-10-25 23:34:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-10-27 16:35:05]

estade@chromium.org changed reviewers:
+ derat@chromium.org

[Evan Stade, 2016-10-27 16:35:05]

+derat, mind taking a first look since you did the last one? I'll find
appropriate owners as needed later on.

[Evan Stade, 2016-10-27 16:36:47]

https://codereview.chromium.org/2441403003/diff/40001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2441403003/diff/40001/components/constrained_...
components/constrained_window/constrained_window_views.cc:224: parent ?
constrained_window_views_client->GetModalDialogHost(parent)
note that this is not a behavioral change as the call already returns null
output for null input --- this just short circuits the lack of a
constrained_window_views_client

[Evan Stade, 2016-10-27 16:36:53]

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

[commit-bot: I haz the power, 2016-10-27 16:37:20]

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

[Daniel Erat, 2016-10-27 17:23:19]

no comments beyond this one, but i'll let others more familiar with dialogs give
the el gee tee em.

https://codereview.chromium.org/2441403003/diff/40001/chrome/browser/ui/simpl...
File chrome/browser/ui/simple_message_box.h (right):

https://codereview.chromium.org/2441403003/diff/40001/chrome/browser/ui/simpl...
chrome/browser/ui/simple_message_box.h:66: bool CloseMessageBoxForTest(bool
accept);
nit: consider WARN_UNUSED_RESULT if tests ought to always check the return value

[commit-bot: I haz the power, 2016-10-27 17:29:37]

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

[commit-bot: I haz the power, 2016-10-27 17:29:38]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-27 19:11:18]

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

[Evan Stade, 2016-10-27 19:11:18]

https://codereview.chromium.org/2441403003/diff/40001/chrome/browser/ui/simpl...
File chrome/browser/ui/simple_message_box.h (right):

https://codereview.chromium.org/2441403003/diff/40001/chrome/browser/ui/simpl...
chrome/browser/ui/simple_message_box.h:66: bool CloseMessageBoxForTest(bool
accept);
On 2016/10/27 17:23:19, Daniel Erat wrote:
> nit: consider WARN_UNUSED_RESULT if tests ought to always check the return
value

Done.

[Evan Stade, 2016-10-27 19:11:23]

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

[commit-bot: I haz the power, 2016-10-27 19:11: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, 2016-10-27 19:42:32]

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

[commit-bot: I haz the power, 2016-10-27 19:42:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-10-27 21:46:45]

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/user_switch_util.cc (right):

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/user_switch_util.cc:38: if
(chrome::ShowQuestionMessageBox(
aside: It's a little unfortunate that this adds a chrome dep in what's
effectively ash code, but it's obviously much cleaner.

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/user_switch_util.cc:40:
l10n_util::GetStringUTF16(IDS_DESKTOP_CASTING_ACTIVE_MESSAGE),
Shouldn't this be IDS_DESKTOP_CASTING_ACTIVE_TITLE? Did you test this?

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

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/simple_message_box_views.cc:118: g_current_message_box =
this;
Should this care if there's any other dialog already showing?

https://codereview.chromium.org/2441403003/diff/60001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2441403003/diff/60001/components/constrained_...
components/constrained_window/constrained_window_views.cc:205:
DCHECK(constrained_window_views_client);
Double checking that this works okay (ie. DCHECK resolve to an empty statement
when off and this doesn't need curly braces). You could just do DCHECK(!parent
|| constrained_window_views_client)?

[Evan Stade, 2016-10-28 17:31:55]

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/user_switch_util.cc (right):

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/user_switch_util.cc:38: if
(chrome::ShowQuestionMessageBox(
On 2016/10/27 21:46:44, msw wrote:
> aside: It's a little unfortunate that this adds a chrome dep in what's
> effectively ash code, but it's obviously much cleaner.

Acknowledged.

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/user_switch_util.cc:40:
l10n_util::GetStringUTF16(IDS_DESKTOP_CASTING_ACTIVE_MESSAGE),
On 2016/10/27 21:46:44, msw wrote:
> Shouldn't this be IDS_DESKTOP_CASTING_ACTIVE_TITLE? Did you test this?

yes, it should be, and yes I tested it. (I don't know how to make this dialog
appear naturally; I had to tweak the code slightly to see it.) I guess I scanned
the dialog too quickly to notice the wrong text.

screenshot: https://screenshot.googleplex.com/B5dVEoasXJL.png

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

https://codereview.chromium.org/2441403003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/simple_message_box_views.cc:118: g_current_message_box =
this;
On 2016/10/27 21:46:45, msw wrote:
> Should this care if there's any other dialog already showing?

Given how it's presently used (only for testing), I guess not? Do you think it
should?

https://codereview.chromium.org/2441403003/diff/60001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2441403003/diff/60001/components/constrained_...
components/constrained_window/constrained_window_views.cc:205:
DCHECK(constrained_window_views_client);
On 2016/10/27 21:46:45, msw wrote:
> Double checking that this works okay (ie. DCHECK resolve to an empty statement
> when off and this doesn't need curly braces).

yes, it works. If it didn't, wouldn't the release build fail due to parent_view
scoping?

> You could just do DCHECK(!parent
> || constrained_window_views_client)?

I thought that was less descriptive since you wouldn't know which arm had
failed, but now that I think about it I guess it's the same since both arms must
fail for the dcheck to fail.

[Evan Stade, 2016-10-28 17:32:00]

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

[commit-bot: I haz the power, 2016-10-28 17:32:13]

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

[msw, 2016-10-28 17:41:44]

lgtm

[Evan Stade, 2016-10-28 17:50:18]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-10-28 17:50:39]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-10-28 17:51:02]

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-10-28 18:43:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-28 19:09:05]

Description was changed from

==========
Use a standard message box for DesktopCastingWarningView.

This is even simpler than what I did for TeleportWarningView, which
means I should probably go back and re-rewrite that one.

BUG=658410
==========

to

==========
Use a standard message box for DesktopCastingWarningView.

This is even simpler than what I did for TeleportWarningView, which
means I should probably go back and re-rewrite that one.

BUG=658410

Committed: https://crrev.com/5f1173f1e5a5678cc6c7cc9046939447398eaf19
Cr-Commit-Position: refs/heads/master@{#428440}
==========

[commit-bot: I haz the power, 2016-10-28 19:09:06]

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

[kjellander_chromium, 2016-11-02 11:57:25]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2469853003/ by kjellander@chromium.org.

The reason for reverting is: Speculative revert for flaky interactive_ui_tests
on Mac10.10 Tests. See crbug.com/661550..