add crypto module password dialog view and test.

add crypto module password dialog view and test.
move cryto_module_password_dialog.cc out of gtk dir into cyrpto_module_password_dialog_view.cc and crypto_module_password_dialog.cc. crypto_module_password_dialog_view.cc needs views lib, and so it is not linked into all platform.

BUG=97844
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112314

[Emmanuel Saint-loubert-Bié, 2011-10-05 14:33:27]

Alice,

Just a quick drive by comment :-)

Also when you 'Publish' please indicate who you is suppose to be the reviewer.
If there is more than one it can be a source of ambiguity.

-- Emmanuel

http://codereview.chromium.org/8115020/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8115020/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:4422:
'browser/ui/crypto_module_password_dialog_view.h',
Because the section says sources! (note the !) this will exclude these files. Is
that what you want? Also in that case do you need the #if defined(OS_WIN) in the
.cc?

[Greg Spencer (Chromium), 2011-10-05 23:41:16]

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:17: #if defined(OS_WIN) ||
defined(TOOLKIT_VIEWS)
It seems to me that you don't need the "defined(OS_WIN)" part of this: the
headers are views specific, and if TOOLKIT_VIEWS isn't defined on Windows, it
will fail anyhow.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:32: password_(),
No need for this: it'll get initialized regardless.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:67: base::Unretained(this)));
Maybe a similar comment here about why it doesn't need to be ref-counted.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:94: #if defined(OS_WIN) ||
defined(TOOLKIT_VIEWS)
Again, I don't think you need OS_WIN here.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog_unittest.cc (right):

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_unittest.cc:21:
CryptoModulePasswordDialogViewTest() : text_("") {}
No need to initialize text_ with "": that's the default.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_unittest.cc:42: std::string
kPassword = "diAl0g";
const std::string?

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_unittest.cc:46: string16 empty;
const string16?

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog_view.cc (right):

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_view.cc:118: else if (button == 
ui::MessageBoxFlags::DIALOGBUTTON_CANCEL)
remove extra space after the "=="

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_view.cc:125: string16 empty;
probably can be const.

[alicet1, 2011-10-06 22:04:08]

http://codereview.chromium.org/8115020/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8115020/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:4422:
'browser/ui/crypto_module_password_dialog_view.h',
On 2011/10/05 14:33:27, Emmanuel Saint-loubert wrote:
> Because the section says sources! (note the !) this will exclude these files.
Is
> that what you want? Also in that case do you need the #if defined(OS_WIN) in
the
> .cc?

windows shouldn't need this. I'll remove the if define(OS_WIN) in the code.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:17: #if defined(OS_WIN) ||
defined(TOOLKIT_VIEWS)
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> It seems to me that you don't need the "defined(OS_WIN)" part of this: the
> headers are views specific, and if TOOLKIT_VIEWS isn't defined on Windows, it
> will fail anyhow.

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:32: password_(),
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> No need for this: it'll get initialized regardless.

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:67: base::Unretained(this)));
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> Maybe a similar comment here about why it doesn't need to be ref-counted.

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.cc:94: #if defined(OS_WIN) ||
defined(TOOLKIT_VIEWS)
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> Again, I don't think you need OS_WIN here.

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog_unittest.cc (right):

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_unittest.cc:21:
CryptoModulePasswordDialogViewTest() : text_("") {}
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> No need to initialize text_ with "": that's the default.

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_unittest.cc:42: std::string
kPassword = "diAl0g";
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> const std::string?

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_unittest.cc:46: string16 empty;
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> const string16?

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog_view.cc (right):

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_view.cc:118: else if (button == 
ui::MessageBoxFlags::DIALOGBUTTON_CANCEL)
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> remove extra space after the "=="

Done.

http://codereview.chromium.org/8115020/diff/6001/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_view.cc:125: string16 empty;
On 2011/10/05 23:41:16, Greg Spencer (Chromium) wrote:
> probably can be const.

Done.

[Greg Spencer (Chromium), 2011-10-06 23:11:21]

LGTM.

[alicet1, 2011-11-29 00:40:39]

+scott for owners. ptal.

thanx,
alice

[sky, 2011-11-29 00:51:28]

Could you copy the existing files to make it easier to see what's actually
changing (and preserve history).

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
File chrome/browser/ui/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:38:
~CryptoModuleBlockingDialogDelegate() {
virtual

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:39: password_.replace(0,
password_.size(), password_.size(), 0);
This is a bit obscure. How about a comment as to why it's needed.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:43: virtual std::string
RequestPassword(const std::string& slot_name, bool retry,
OVERRIDE and each param on its own line.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:73: void GotPassword(const
char* password) {
nit: newline between 72 and 73.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:80: base::WaitableEvent
event_;
nit: newline between 79 and 80

[alicet1, 2011-11-29 19:04:27]

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
File chrome/browser/ui/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:38:
~CryptoModuleBlockingDialogDelegate() {
On 2011/11/29 00:51:28, sky wrote:
> virtual

Done.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:39: password_.replace(0,
password_.size(), password_.size(), 0);
On 2011/11/29 00:51:28, sky wrote:
> This is a bit obscure. How about a comment as to why it's needed.

Done.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:43: virtual std::string
RequestPassword(const std::string& slot_name, bool retry,
On 2011/11/29 00:51:28, sky wrote:
> OVERRIDE and each param on its own line.

Done.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:73: void GotPassword(const
char* password) {
On 2011/11/29 00:51:28, sky wrote:
> nit: newline between 72 and 73.

Done.

http://codereview.chromium.org/8115020/diff/38001/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:80: base::WaitableEvent
event_;
On 2011/11/29 00:51:28, sky wrote:
> nit: newline between 79 and 80

Done.

http://codereview.chromium.org/8115020/diff/38002/chrome/browser/ui/crypto_mo...
File chrome/browser/ui/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/8115020/diff/38002/chrome/browser/ui/crypto_mo...
chrome/browser/ui/crypto_module_password_dialog.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
I tried re-uploading, but that doesn't show the diff to patch set 1 (when the
move occurred).

here's the link to show the file move:
http://codereview.chromium.org/8115020/diff2/1:38002/chrome/browser/ui/crypto...

[sky, 2011-11-29 21:40:58]

LGTM

[commit-bot: I haz the power, 2011-11-30 20:44:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alicet@chromium.org/8115020/48010

[commit-bot: I haz the power, 2011-11-30 22:33:51]

Change committed as 112314