Add a passphrase dialog for managed user accounts.

Add a passphrase dialog for managed user accounts.

This changelist is an extract from this changelist:
https://codereview.chromium.org/11783008/
This dialog will be displayed whenever the user wants to access something which
should only be accessible to the manager of the managed account. This
changelist just introduces the dialog without actually using it somewhere,
the tests for this dialog will be added in another changelist.
A screenshot of this dialog can be found here:
https://docs.google.com/a/google.com/drawings/d/1ZSy4TH1k-nOwgza8G-sHc_z_n6-yIzkp7sdHD20ylM4/edit?usp=sharing

BUG=171370
TEST=It compiles.


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

[Adrian Kuegel, 2013-02-12 14:24:52]

Bernhard, could you please take a look at this changelist, especially the files
in chrome/browser/managed_mode ?
James, could you please take a look at the rest?

[Bernhard Bauer, 2013-02-12 14:27:50]

LGTM

[James Hawkins, 2013-02-12 18:57:15]

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
File chrome/browser/resources/managed_user_passphrase_dialog.html (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:22: <span
i18n-content="passphrase_instruction"></span>
nit: passphraseInstruction

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:26: <input
id="passphrase-entry" size="50" type="password" required/>
nit: Do not close single tags.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:28: <!--
TODO(akuegel): Create a link to a page which explains how to reset
Personally I'd just remove this code since it's deadcode and will rot.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:32: <span
i18n-content="forgot_passphrase"></span></a>
nit: </a> needs to be on a new line.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:32: <span
i18n-content="forgot_passphrase"></span></a>
nit: forgotPassphrase

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:38:
i18n-content="cancel_passphrase_button"></button>
nit: cancelPassphraseButton

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:41:
i18n-content="unlock_passphrase_button" disabled></button>
unlockPassphraseButton

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:46: <span
i18n-content="incorrect_passphrase_warning"></span>
nit: incorrectPassphraseWarning

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:51: <script
src="chrome://resources/js/i18n_template2.js"></script>
Why is this down here?  Are you even using this?

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
File chrome/browser/resources/managed_user_passphrase_dialog.js (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.js:10:
chrome.send('DialogClose');
nit: closeDialog

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
File chrome/browser/ui/webui/managed_user_passphrase_dialog.cc (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:13: #include
"base/values.h"
Where is this used?

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:30: class
ManagedUserPassphraseDialogMessageHandler
nit: Document class.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:36: virtual void
RegisterMessages() OVERRIDE;
// content::WebUIMessageHandler implementation.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:39: void
CheckPassphrase(const base::ListValue* args) const;
nit: Document method.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:51:
web_ui()->RegisterMessageCallback("checkPassphrase",
nit: The start of parameter rows must align on the same column.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:58: const base::Value*
passphrase_arg = NULL;
This method could use some whitespace love and some commenting. It's very
unreadable as-is.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:84: const int
kDialogWidth = 400;
These are in the global namespace, which is no bueno.  Need to be in unnamed
namespace.

Now that I saw where you use these, just move them into the method where they're
used.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:110: // This function
is called by the constrained window delegate.
This comment is superfluous.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:129: if (closing_)
Why do you need |closing_|?  Can't you reset the callback_ once it's run and use
that as a signal?

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
File chrome/browser/ui/webui/managed_user_passphrase_dialog.h (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.h:52: void
CreateDataSource(Profile* profile) const;
nit: Document method.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.h:54:
base::Callback<void(bool)> callback_;
Create a documented, well-named typedef for the callback.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.h:55: bool closing_;
nit: Document member variables.

[Adrian Kuegel, 2013-02-13 10:53:36]

Thanks for the review. I tried to address all your comments, but I am still
unsure about the "DialogClose" (see my inlined answer to your comment).

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
File chrome/browser/resources/managed_user_passphrase_dialog.html (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:22: <span
i18n-content="passphrase_instruction"></span>
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: passphraseInstruction

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:26: <input
id="passphrase-entry" size="50" type="password" required/>
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: Do not close single tags.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:28: <!--
TODO(akuegel): Create a link to a page which explains how to reset
Ok, I removed it.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:38:
i18n-content="cancel_passphrase_button"></button>
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: cancelPassphraseButton

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:41:
i18n-content="unlock_passphrase_button" disabled></button>
On 2013/02/12 18:57:16, James Hawkins wrote:
> unlockPassphraseButton

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:46: <span
i18n-content="incorrect_passphrase_warning"></span>
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: incorrectPassphraseWarning

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.html:51: <script
src="chrome://resources/js/i18n_template2.js"></script>
It is used to generated the correct text for the labels and the buttons. It only
works if it is specified after the main html specification. I could move it
after the body tag, though, as it is done for example in
chrome/browser/resources/certificate_viewer.html. But most html pages which need
this script have it directly before the closing </body> tag.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
File chrome/browser/resources/managed_user_passphrase_dialog.js (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.js:10:
chrome.send('DialogClose');
Here I am using a function which is defined in
chrome/browser/ui/webui/constrained_web_dialog_ui.cc. Changing the definition
there would require some changes to other files, too. Should I do this?
I could also define my own function, but I thought reusing an existing function
if possible would be better.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
File chrome/browser/ui/webui/managed_user_passphrase_dialog.cc (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:13: #include
"base/values.h"
For the CheckPassphrase method, which is called from the UI and gets passed a
ListValue parameter.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:30: class
ManagedUserPassphraseDialogMessageHandler
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: Document class.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:36: virtual void
RegisterMessages() OVERRIDE;
On 2013/02/12 18:57:16, James Hawkins wrote:
> // content::WebUIMessageHandler implementation.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:39: void
CheckPassphrase(const base::ListValue* args) const;
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: Document method.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:51:
web_ui()->RegisterMessageCallback("checkPassphrase",
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: The start of parameter rows must align on the same column.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:58: const base::Value*
passphrase_arg = NULL;
On 2013/02/12 18:57:16, James Hawkins wrote:
> This method could use some whitespace love and some commenting. It's very
> unreadable as-is.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:84: const int
kDialogWidth = 400;
On 2013/02/12 18:57:16, James Hawkins wrote:
> These are in the global namespace, which is no bueno.  Need to be in unnamed
> namespace.
> 
> Now that I saw where you use these, just move them into the method where
they're
> used.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:110: // This function
is called by the constrained window delegate.
On 2013/02/12 18:57:16, James Hawkins wrote:
> This comment is superfluous.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:129: if (closing_)
True, I don't need |closing_|. I changed it like you suggested.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
File chrome/browser/ui/webui/managed_user_passphrase_dialog.h (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.h:52: void
CreateDataSource(Profile* profile) const;
On 2013/02/12 18:57:16, James Hawkins wrote:
> nit: Document method.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.h:54:
base::Callback<void(bool)> callback_;
On 2013/02/12 18:57:16, James Hawkins wrote:
> Create a documented, well-named typedef for the callback.

Done.

https://codereview.chromium.org/12218118/diff/1/chrome/browser/ui/webui/manag...
chrome/browser/ui/webui/managed_user_passphrase_dialog.h:55: bool closing_;
This variable is now removed.

[James Hawkins, 2013-02-13 21:46:48]

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
File chrome/browser/resources/managed_user_passphrase_dialog.css (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.css:22: float: right;
Need to handle RTL.

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
File chrome/browser/resources/managed_user_passphrase_dialog.html (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.html:31:
i18n-content="cancelPassphraseButton"></button>
nit: Closing tag of wrapped element must be on a new line.

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.html:34:
i18n-content="unlockPassphraseButton" disabled></button>
nit: Closing tag of wrapped element must be on a new line.

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/ui/webui/m...
File chrome/browser/ui/webui/managed_user_passphrase_dialog.cc (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:30: // This class is
used to handle messages which are passed from the UI.
nit: No ned to say "This class is".  I'd crop everything up to "Handles the case
when..." but phrase it as "Handles the message when the user...".

[James Hawkins, 2013-02-13 21:47:26]

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
File chrome/browser/resources/managed_user_passphrase_dialog.js (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.js:10:
chrome.send('DialogClose');
On 2013/02/13 10:53:36, Adrian Kuegel wrote:
> Here I am using a function which is defined in
> chrome/browser/ui/webui/constrained_web_dialog_ui.cc. Changing the definition
> there would require some changes to other files, too. Should I do this?
> I could also define my own function, but I thought reusing an existing
function
> if possible would be better.

Personally I would rename it, but feel free to do in a followup CL or add a
TODO.

[Adrian Kuegel, 2013-02-14 10:36:11]

I uploaded a new patch. Hopefully I am making some progress towards a LGTM :-)

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
File chrome/browser/resources/managed_user_passphrase_dialog.js (right):

https://codereview.chromium.org/12218118/diff/1/chrome/browser/resources/mana...
chrome/browser/resources/managed_user_passphrase_dialog.js:10:
chrome.send('DialogClose');
Ok, then I will add a TODO and do it in a later CL, because it will take some
time for me to make sure I don't miss a place where this is used. Unfortunately,
'DialogClose' is also used in other places referring to another function, so I
can't just replace all instances in the code (unless I change the definition of
the other function as well).

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
File chrome/browser/resources/managed_user_passphrase_dialog.css (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.css:22: float: right;
I added rtl handling for that button and tested it with arabic language. But
this dialog will probably change anyway when we get feedback from the UI team.
So I would prefer to do polishing in a follow-up CL.

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
File chrome/browser/resources/managed_user_passphrase_dialog.html (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.html:31:
i18n-content="cancelPassphraseButton"></button>
On 2013/02/13 21:46:48, James Hawkins wrote:
> nit: Closing tag of wrapped element must be on a new line.

Done.

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.html:34:
i18n-content="unlockPassphraseButton" disabled></button>
On 2013/02/13 21:46:48, James Hawkins wrote:
> nit: Closing tag of wrapped element must be on a new line.

Done.

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/ui/webui/m...
File chrome/browser/ui/webui/managed_user_passphrase_dialog.cc (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/ui/webui/m...
chrome/browser/ui/webui/managed_user_passphrase_dialog.cc:30: // This class is
used to handle messages which are passed from the UI.
On 2013/02/13 21:46:48, James Hawkins wrote:
> nit: No ned to say "This class is".  I'd crop everything up to "Handles the
case
> when..." but phrase it as "Handles the message when the user...".

Done.

[Adrian Kuegel, 2013-02-15 16:11:16]

@James: can you please take another look at my recent changes?

[James Hawkins, 2013-02-15 17:35:55]

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
File chrome/browser/resources/managed_user_passphrase_dialog.css (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.css:22: float: right;
On 2013/02/14 10:36:12, Adrian Kuegel wrote:
> I added rtl handling for that button and tested it with arabic language. But
> this dialog will probably change anyway when we get feedback from the UI team.
> So I would prefer to do polishing in a follow-up CL.

This isn't a polish issue :-)  Please fix it.

[Adrian Kuegel, 2013-02-16 08:10:50]

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
File chrome/browser/resources/managed_user_passphrase_dialog.css (right):

https://codereview.chromium.org/12218118/diff/14002/chrome/browser/resources/...
chrome/browser/resources/managed_user_passphrase_dialog.css:22: float: right;
But I did! That is what I meant with "I added rtl handling for that button". I
also tested it with arabic language. Should I send you another screenshot?

[Adrian Kuegel, 2013-02-18 09:24:38]

Here is a screenshot of how it looks like with arabic language
https://docs.google.com/a/google.com/file/d/0BzMCIwDgeNezZS1JRzV6RzgzN3c/edit...
Note that the one string which is not shown in arabic is introduced together
with this changelist, so there is no translation yet.

I hope you didn't get the impression that I don't care about fixing or improving
things, because that is not true. For example, I recently fixed a bug (not
introduced by me) in the extensions page related to RTL which I noticed after
you had pointed out that I didn't handle it in my dialog (see
https://codereview.chromium.org/12252036/ ).

I just want to explain my comment about why I would prefer to do further work on
this dialog in a follow-up changelist if possible.

1. This dialog is currently not used, but there are several other changelists
waiting for it.
2. When it will be used, it is behind a flag. All things related to Managed
Users are behind a flag. So there would be no possibility for someone to
accidentally see this dialog in an unfinished state.

[James Hawkins, 2013-02-19 20:47:16]

Totally my bad, Adrian.  I misread what you said and jumped off the CL before
looking!

LGTM

[commit-bot: I haz the power, 2013-02-19 20:54:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/12218118/9035

[commit-bot: I haz the power, 2013-02-20 11:15:25]

Change committed as 183466