Create a dialog that warns about possible UI-oddities of the multi-profiles window teleport

Adding a dialog that warns about possible UI-oddities of after teleporting window to another user in multi-profiles mode.

BUG=318870
TBR=pkasting@chromium.org

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

[Mr4D (OOO till 08-26), 2014-01-28 17:33:01]

Close!

https://codereview.chromium.org/148093008/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/148093008/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:1208: +        <message
name="IDS_VISIT_DESKTOP_WARNING_TITLE" desc="The title of dialog that warns user
about oddities he/she can meet after window move to another user desktop.">
Maybe something like
"The title of the dialog which warns the user about oddities which can be seen
when a window gets moved to another desktop."

https://codereview.chromium.org/148093008/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:1211: +        <message
name="IDS_VISIT_DESKTOP_WARNING_MESSAGE" desc="The message on dialog that warns
user about oddities he/she can meet after window move to another user desktop.">
similar to above?

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos.cc
(right):

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos.cc:130: it !=
logged_in_users.end(); ++it) {
optional nit:
Why not doing "it != logged_in_users.end() && show_warning" instead of using the
break?

- or even better -

remove the variable altogether, do an if statement instead and pull the
chromeos::ShowMultiProfilesWarningDialog + a return in there.

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
File chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc (right):

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:31: // Dialog for
multi-profiles teleport warning.
What happened to the old "multi-profiles introduction" dialog?

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:39: //
views:DialogDelegate overrides.
views::Dialog..

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:107: // title
Start capitalized.

[benwells, 2014-01-28 22:00:54]

chrome/browser/ui/views/apps/native_app_window_views.cc lgtm

[merkulova, 2014-01-29 09:11:18]

https://codereview.chromium.org/148093008/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/148093008/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:1208: +        <message
name="IDS_VISIT_DESKTOP_WARNING_TITLE" desc="The title of dialog that warns user
about oddities he/she can meet after window move to another user desktop.">
On 2014/01/28 17:33:02, Mr4D wrote:
> Maybe something like
> "The title of the dialog which warns the user about oddities which can be seen
> when a window gets moved to another desktop."

Done.

https://codereview.chromium.org/148093008/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:1211: +        <message
name="IDS_VISIT_DESKTOP_WARNING_MESSAGE" desc="The message on dialog that warns
user about oddities he/she can meet after window move to another user desktop.">
On 2014/01/28 17:33:02, Mr4D wrote:
> similar to above?

Done.

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos.cc
(right):

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos.cc:130: it !=
logged_in_users.end(); ++it) {
On 2014/01/28 17:33:02, Mr4D wrote:
> optional nit:
> Why not doing "it != logged_in_users.end() && show_warning" instead of using
the
> break?
> 
> - or even better -
> 
> remove the variable altogether, do an if statement instead and pull the
> chromeos::ShowMultiProfilesWarningDialog + a return in there.

Done.

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
File chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc (right):

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:31: // Dialog for
multi-profiles teleport warning.
On 2014/01/28 17:33:02, Mr4D wrote:
> What happened to the old "multi-profiles introduction" dialog?
Hm... Nothing, it exists happily. It's just another dialog with almost same
logic (shown until user leaves 'Dont show again' tick)
Also it's shown at different action (teleport, not one more user sign in).

Have a look on bug description, there is some mock, so you can see the
difference.

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:39: //
views:DialogDelegate overrides.
On 2014/01/28 17:33:02, Mr4D wrote:
> views::Dialog..

Done.

https://codereview.chromium.org/148093008/diff/1/chrome/browser/ui/ash/multi_...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:107: // title
On 2014/01/28 17:33:02, Mr4D wrote:
> Start capitalized.

Done.

[ygorshenin1, 2014-01-29 09:24:04]

https://codereview.chromium.org/148093008/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:1209: +            Careful, this feature may
bite
nit: 2 spaces indent in *.grd files.

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu.cc (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_context_menu.cc:15: }
nit: you can move '}' to the previous line.

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu.h (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_context_menu.h:21: // Executes move
to another profile.
Could you please clarify in the comment what are the purposes of |command_id|
and |window|?

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:48: static int
reciever_user_id;
s/reciever/receiver/

Why do you need this static field?

[merkulova, 2014-01-29 09:46:21]

https://codereview.chromium.org/148093008/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:1209: +            Careful, this feature may
bite
On 2014/01/29 09:24:04, ygorshenin1 wrote:
> nit: 2 spaces indent in *.grd files.

Done.

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu.cc (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_context_menu.cc:15: }
On 2014/01/29 09:24:04, ygorshenin1 wrote:
> nit: you can move '}' to the previous line.

Done.

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu.h (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_context_menu.h:21: // Executes move
to another profile.
On 2014/01/29 09:24:04, ygorshenin1 wrote:
> Could you please clarify in the comment what are the purposes of |command_id|
> and |window|?

Done.

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc (right):

https://codereview.chromium.org/148093008/diff/20001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_warning_dialog.cc:48: static int
reciever_user_id;
On 2014/01/29 09:24:04, ygorshenin1 wrote:
> s/reciever/receiver/
> 
> Why do you need this static field?

It was a trash line, sorry.

[ygorshenin1, 2014-01-29 09:55:27]

lgtm

https://codereview.chromium.org/148093008/diff/40001/chrome/browser/ui/ash/mu...
File chrome/browser/ui/ash/multi_user/multi_user_context_menu.h (right):

https://codereview.chromium.org/148093008/diff/40001/chrome/browser/ui/ash/mu...
chrome/browser/ui/ash/multi_user/multi_user_context_menu.h:21: // Executes move
of a window to another profile.
nit: s/windiw/|window|/
     s/Command_id/|command_id|/

[Mr4D (OOO till 08-26), 2014-01-29 17:16:54]

lgtm

[commit-bot: I haz the power, 2014-01-30 09:38:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/merkulova@chromium.org/148093008/60001

[tfarina, 2014-01-30 12:41:46]

Can you improve your CL description next time?

I'd explain in the subject line what the patch is.

And then in the body I'd give a brief explanation of the patch, what you are
doing, what you are fixing or implementing and why.

Instead of using 'created' I think it is more common to say:

"Create a new warning dialog for the multi-profiles window teleport".

Whatever multi-profiles window teleport is!

[commit-bot: I haz the power, 2014-01-30 13:47:12]

List of reviewers changed. tfarina@chromium.org did a drive-by without LGTM'ing!

[commit-bot: I haz the power, 2014-01-30 15:04:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/merkulova@chromium.org/148093008/60001

[commit-bot: I haz the power, 2014-01-30 18:01:50]

Change committed as 247992

[commit-bot: I haz the power, 2014-01-30 18:01:52]

CQ bit was unchecked on CL. Ignoring.