windows: Show an icon to the right indicating if the host is valid or not....

windows: Show an icon to the right indicating if the host is valid or not.

BUG=34181
TEST=open the exceptions editor, type some text in the host textfield, see the indication.

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

[tfarina (gmail-do not use), 2010-02-19 03:58:35]

Hi Scott, Peter, could you review this to me?

Screenshots below:

http://imgur.com/R8R5A
http://imgur.com/3o9FZ

[sky, 2010-02-19 17:05:57]

http://codereview.chromium.org/650011/diff/1/2
File chrome/browser/views/options/exception_editor_view.cc (right):

http://codereview.chromium.org/650011/diff/1/2#newcode138
chrome/browser/views/options/exception_editor_view.cc:138:
UpdateImageView(host_iv_, ValidHost(UTF16ToUTF8(host_tf_->text())));
This needs to have the same logic as is in IsDialogButtonEnabled. You'll want to
refactor it so both places can share code.

http://codereview.chromium.org/650011/diff/1/3
File chrome/browser/views/options/exception_editor_view.h (right):

http://codereview.chromium.org/650011/diff/1/3#newcode16
chrome/browser/views/options/exception_editor_view.h:16: #include
"views/controls/image_view.h"
Forward declare ImageView.

[Peter Kasting, 2010-02-19 20:51:06]

http://codereview.chromium.org/650011/diff/1/2
File chrome/browser/views/options/exception_editor_view.cc (right):

http://codereview.chromium.org/650011/diff/1/2#newcode195
chrome/browser/views/options/exception_editor_view.cc:195: if (is_valid) {
Nit: Rewrite as:

image_view->SetImage(ResourceBundle::GetSharedInstance().GetBitmapNamed(
    is_valid ? IDR_INPUT_GOOD : IDR_INPUT_ALERT);

http://codereview.chromium.org/650011/diff/1/2#newcode202
chrome/browser/views/options/exception_editor_view.cc:202: }
Make sure this } has a newline after it.  The linter complained, but sometimes
it lies.

[tfarina (gmail-do not use), 2010-02-19 23:10:29]

New snapshot uploaded. Comments addressed.

http://codereview.chromium.org/650011/diff/1/2
File chrome/browser/views/options/exception_editor_view.cc (right):

http://codereview.chromium.org/650011/diff/1/2#newcode138
chrome/browser/views/options/exception_editor_view.cc:138:
UpdateImageView(host_iv_, ValidHost(UTF16ToUTF8(host_tf_->text())));
On 2010/02/19 17:05:57, sky wrote:
> This needs to have the same logic as is in IsDialogButtonEnabled. You'll want
to
> refactor it so both places can share code.

Added new method IsHostValid for this.

http://codereview.chromium.org/650011/diff/1/2#newcode195
chrome/browser/views/options/exception_editor_view.cc:195: if (is_valid) {
On 2010/02/19 20:51:06, Peter Kasting wrote:
> Nit: Rewrite as:
> 
> image_view->SetImage(ResourceBundle::GetSharedInstance().GetBitmapNamed(
>     is_valid ? IDR_INPUT_GOOD : IDR_INPUT_ALERT);

Done.

http://codereview.chromium.org/650011/diff/1/2#newcode202
chrome/browser/views/options/exception_editor_view.cc:202: }
On 2010/02/19 20:51:06, Peter Kasting wrote:
> Make sure this } has a newline after it.  The linter complained, but sometimes
> it lies.

Done.

http://codereview.chromium.org/650011/diff/1/3
File chrome/browser/views/options/exception_editor_view.h (right):

http://codereview.chromium.org/650011/diff/1/3#newcode16
chrome/browser/views/options/exception_editor_view.h:16: #include
"views/controls/image_view.h"
On 2010/02/19 17:05:57, sky wrote:
> Forward declare ImageView.

Done.

[Peter Kasting, 2010-02-19 23:20:06]

LGTM

http://codereview.chromium.org/650011/diff/12/13
File chrome/browser/views/options/exception_editor_view.cc (right):

http://codereview.chromium.org/650011/diff/12/13#newcode22
chrome/browser/views/options/exception_editor_view.cc:22: namespace {
Nit: If you're going to add this namespace, you no longer need "static" on the
two settings arrays.

http://codereview.chromium.org/650011/diff/12/13#newcode192
chrome/browser/views/options/exception_editor_view.cc:192: return
ValidHost(host) &&
Nit: I think it'd be clearer to hoist this to a temp:

bool is_valid_new_host == ValidHost(host) &&
(model_->IndexOfExceptionByHost(host) == -1);
return is_new() ? is_valid_new_host : (!host.empty() && ((host_ == host) ||
is_valid_new_host));

[tfarina (gmail-do not use), 2010-02-19 23:52:26]

http://codereview.chromium.org/650011/diff/12/13
File chrome/browser/views/options/exception_editor_view.cc (right):

http://codereview.chromium.org/650011/diff/12/13#newcode22
chrome/browser/views/options/exception_editor_view.cc:22: namespace {
On 2010/02/19 23:20:06, Peter Kasting wrote:
> Nit: If you're going to add this namespace, you no longer need "static" on the
> two settings arrays.

Done.

http://codereview.chromium.org/650011/diff/12/13#newcode192
chrome/browser/views/options/exception_editor_view.cc:192: return
ValidHost(host) &&
On 2010/02/19 23:20:06, Peter Kasting wrote:
> Nit: I think it'd be clearer to hoist this to a temp:
> 
> bool is_valid_new_host == ValidHost(host) &&
> (model_->IndexOfExceptionByHost(host) == -1);
> return is_new() ? is_valid_new_host : (!host.empty() && ((host_ == host) ||
> is_valid_new_host));

Done.

[tfarina (gmail-do not use), 2010-02-24 00:43:19]

On 2010/02/19 23:20:06, Peter Kasting wrote:
> LGTM
> 

Peter, could you land this to me?

The linux side was already committed.
http://src.chromium.org/viewvc/chrome?view=rev&revision=39802

[Peter Kasting, 2010-02-24 22:01:35]

Landed in r39931.