Omnibox M5 work, part 1: Security changes...

Omnibox M5 work, part 1: Security changes

* Remove yellow background
* Move lock icon from right side to left side (only on Win)
* Change iconography
* Change scheme colors
* Add label for "Untrusted website"
* Remove tooltip on label

This also simplifies the LocationBarView code on Windows now that LocationBarImageView is unnecessary, and reorders a few things to try and be in more consistent/physical order.

BUG=27570
TEST=Visit various https sites and see that the security icon is on the left, the scheme is colored, and there is no yellow background.

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

[Peter Kasting, 2010-03-17 00:53:33]

This change is not complete: Mac and Linux have not been updated to match the
global changes, and as a result neither will compile.  This is large enough that
I wanted to give you a head start on reviewing before adding all that in.

[sky, 2010-03-17 17:38:09]

LGTM so far.

http://codereview.chromium.org/1056002/diff/16001/10018
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/1056002/diff/16001/10018#newcode110
chrome/browser/toolbar_model.cc:110: GURL
url(GetNavigationController()->GetActiveEntry()->url());
GetActiveEntry can return NULL.

http://codereview.chromium.org/1056002/diff/16001/10018#newcode189
chrome/browser/toolbar_model.cc:189: for (size_t i = 0; i < errors.size(); ++i)
{
Hath hell frozen over? You're not using an iterator? WOW!

http://codereview.chromium.org/1056002/diff/16001/10017
File chrome/browser/toolbar_model.h (right):

http://codereview.chromium.org/1056002/diff/16001/10017#newcode21
chrome/browser/toolbar_model.h:21: enum SecurityLevel {
How about a description of this enum?

[Peter Kasting, 2010-03-17 17:44:44]

http://codereview.chromium.org/1056002/diff/16001/10018
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/1056002/diff/16001/10018#newcode110
chrome/browser/toolbar_model.cc:110: GURL
url(GetNavigationController()->GetActiveEntry()->url());
On 2010/03/17 17:38:09, sky wrote:
> GetActiveEntry can return NULL.

But it can't be NULL here, because if it was, GetSecurityLevel() would have
returned NONE.

http://codereview.chromium.org/1056002/diff/16001/10018#newcode189
chrome/browser/toolbar_model.cc:189: for (size_t i = 0; i < errors.size(); ++i)
{
On 2010/03/17 17:38:09, sky wrote:
> Hath hell frozen over? You're not using an iterator? WOW!

I was just cleaning up the old code, so I didn't change it.  I'm happy to switch
if you'd like :)

http://codereview.chromium.org/1056002/diff/16001/10017
File chrome/browser/toolbar_model.h (right):

http://codereview.chromium.org/1056002/diff/16001/10017#newcode21
chrome/browser/toolbar_model.h:21: enum SecurityLevel {
On 2010/03/17 17:38:09, sky wrote:
> How about a description of this enum?

Sure.

[sky, 2010-03-17 17:48:41]

On Wed, Mar 17, 2010 at 9:44 AM,  <pkasting@chromium.org> wrote:
>
> http://codereview.chromium.org/1056002/diff/16001/10018
> File chrome/browser/toolbar_model.cc (right):
>
> http://codereview.chromium.org/1056002/diff/16001/10018#newcode110
> chrome/browser/toolbar_model.cc:110: GURL
> url(GetNavigationController()->GetActiveEntry()->url());
> On 2010/03/17 17:38:09, sky wrote:
>>
>> GetActiveEntry can return NULL.
>
> But it can't be NULL here, because if it was, GetSecurityLevel() would
> have returned NONE.

Ok.

> http://codereview.chromium.org/1056002/diff/16001/10018#newcode189
> chrome/browser/toolbar_model.cc:189: for (size_t i = 0; i <
> errors.size(); ++i) {
> On 2010/03/17 17:38:09, sky wrote:
>>
>> Hath hell frozen over? You're not using an iterator? WOW!
>
> I was just cleaning up the old code, so I didn't change it.  I'm happy
> to switch if you'd like :)

You know my feelings on using iterators for vectors.

  -Scott

[jcampan, 2010-03-17 23:10:11]

http://codereview.chromium.org/1056002/diff/16001/10018
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/1056002/diff/16001/10018#newcode75
chrome/browser/toolbar_model.cc:75: return SECURITY_WARNING;
In the past we decided to show pages with mixed-contents as unauthenticated
rather than security warning. Has a new decision been made?

http://codereview.chromium.org/1056002/diff/16001/10018#newcode184
chrome/browser/toolbar_model.cc:184: } else if (errors.size() == 1) {
1U for it to compile on Unix.

[Peter Kasting, 2010-03-17 23:17:36]

http://codereview.chromium.org/1056002/diff/16001/10018
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/1056002/diff/16001/10018#newcode75
chrome/browser/toolbar_model.cc:75: return SECURITY_WARNING;
On 2010/03/17 23:10:11, jcampan wrote:
> In the past we decided to show pages with mixed-contents as unauthenticated
> rather than security warning.

I'm confused.  The old GetIcon() code returned WARNING_ICON in this case, which
we displayed at the end of the address bar.  We're changing the look of the icon
(to a lock with a triangle overlaid), but in both cases there's been an
indicator, not nothing.

http://codereview.chromium.org/1056002/diff/16001/10018#newcode184
chrome/browser/toolbar_model.cc:184: } else if (errors.size() == 1) {
On 2010/03/17 23:10:11, jcampan wrote:
> 1U for it to compile on Unix.

I'm pretty sure we only need the U in our testing macros because the compiler
can't deduce the right types.  I'll make sure the trybots compile once I've got
the Linux side done, though.

[jcampan, 2010-03-18 15:54:44]

LGTM

http://codereview.chromium.org/1056002/diff/16001/10018
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/1056002/diff/16001/10018#newcode75
chrome/browser/toolbar_model.cc:75: return SECURITY_WARNING;
On 2010/03/17 23:17:36, Peter Kasting wrote:
> On 2010/03/17 23:10:11, jcampan wrote:
> > In the past we decided to show pages with mixed-contents as unauthenticated
> > rather than security warning.
> 
> I'm confused.  The old GetIcon() code returned WARNING_ICON in this case,
which
> we displayed at the end of the address bar.  We're changing the look of the
icon
> (to a lock with a triangle overlaid), but in both cases there's been an
> indicator, not nothing.
You are right, I was confused with the background. We don't show a red
background for mixed-contents but still a warning icon.

[Peter Kasting, 2010-03-22 22:43:00]

Latest version now works on all platforms.  Adding people to review Mac and GTK
sides.  Please note that moving the icon to the left of the address is only done
on Windows right now and needs to be added for the other platforms -- I'll note
that on the relevant bugs.

[sky, 2010-03-22 23:32:20]

The windows side LGTM

[Scott Hess - ex-Googler, 2010-03-23 18:13:17]

Mac code LGTM.

http://codereview.chromium.org/1056002/diff/64021/66021
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/1056002/diff/64021/66021#newcode460
chrome/browser/cocoa/location_bar_view_mac.mm:460: // address.
Is this an indication that you plan to do it?  If you plan to delegate to me,
TODO(shess):...

http://codereview.chromium.org/1056002/diff/64021/66021#newcode580
chrome/browser/cocoa/location_bar_view_mac.mm:580: // TODO(pkasting): Use a data
structure to avoid this copy-and-pasting.
The switch() could initialize a scoped_nsobject<NSImage>*, and then all the
common code could come after the switch.

[Evan Stade, 2010-03-23 22:36:11]

http://codereview.chromium.org/1056002/diff/96001/97013
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/96001/97013#newcode581
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:581:
gtk_widget_modify_base(text_view_, GTK_STATE_NORMAL,
why are you throwing out all the gtk theming code?

http://codereview.chromium.org/1056002/diff/96001/97015
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/96001/97015#newcode200
chrome/browser/gtk/location_bar_view_gtk.cc:200:
gtk_widget_hide(GTK_WIDGET(ev_secure_icon_image_));
I don't think this is necessary -- default should be hidden

[Peter Kasting, 2010-03-23 22:51:17]

http://codereview.chromium.org/1056002/diff/96001/97013
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/96001/97013#newcode581
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:581:
gtk_widget_modify_base(text_view_, GTK_STATE_NORMAL,
On 2010/03/23 22:36:12, Evan Stade wrote:
> why are you throwing out all the gtk theming code?

I think I did this wrong.  I was trying to get rid of the bits to change the
background color to yellow, and instead I threw out the other half of the
conditional.  Let me try this again.

http://codereview.chromium.org/1056002/diff/96001/97015
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/96001/97015#newcode200
chrome/browser/gtk/location_bar_view_gtk.cc:200:
gtk_widget_hide(GTK_WIDGET(ev_secure_icon_image_));
On 2010/03/23 22:36:12, Evan Stade wrote:
> I don't think this is necessary -- default should be hidden

OK, I was just preserving the old code's behavior.  I can take these out as long
as you're sure it's OK.

[Evan Stade, 2010-03-23 22:53:42]

http://codereview.chromium.org/1056002/diff/96001/97015
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/96001/97015#newcode200
chrome/browser/gtk/location_bar_view_gtk.cc:200:
gtk_widget_hide(GTK_WIDGET(ev_secure_icon_image_));
On 2010/03/23 22:51:18, Peter Kasting wrote:
> On 2010/03/23 22:36:12, Evan Stade wrote:
> > I don't think this is necessary -- default should be hidden
> 
> OK, I was just preserving the old code's behavior.  I can take these out as
long
> as you're sure it's OK.

right---well as long as it _seems_ to work without these hide() calls I'd say
it's safe to remove them

[Peter Kasting, 2010-03-23 23:18:51]

On Tue, Mar 23, 2010 at 3:53 PM, <estade@chromium.org> wrote:

> http://codereview.chromium.org/1056002/diff/96001/97015#newcode200
> chrome/browser/gtk/location_bar_view_gtk.cc:200:
> gtk_widget_hide(GTK_WIDGET(ev_secure_icon_image_));
> On 2010/03/23 22:51:18, Peter Kasting wrote:
>
>> On 2010/03/23 22:36:12, Evan Stade wrote:
>> > I don't think this is necessary -- default should be hidden
>>
>
>  OK, I was just preserving the old code's behavior.  I can take these
>>
> out as long
>
>> as you're sure it's OK.
>>
>
> right---well as long as it _seems_ to work without these hide() calls
> I'd say it's safe to remove them


I don't know; I don't have a Linux box I'm testing this on, I'm writing it
blind.  That said, we hide all these in SetSecurityIcon(), which is called
in Update(), so I'd think we should be OK.

http://codereview.chromium.org/1056002
>

[Peter Kasting, 2010-03-23 23:35:00]

New snap up

[Evan Stade, 2010-03-24 00:05:54]

do you plan to test this on linux?

http://codereview.chromium.org/1056002/diff/114001/115013
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/114001/115013#newcode614
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:614: void
AutocompleteEditViewGtk::SetBaseColor() {
did you mean to add this function twice?

[Peter Kasting, 2010-03-24 00:10:23]

On 2010/03/24 00:05:54, Evan Stade wrote:
> do you plan to test this on linux?

This is part one of four or five successive changes for M5.  Ben is finding me
someone to help get all the Linux bits in properly and make sure everything is
correct; until then I planned to check this in once the trybots OK'd it.

http://codereview.chromium.org/1056002/diff/114001/115013
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/1056002/diff/114001/115013#newcode614
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:614: void
AutocompleteEditViewGtk::SetBaseColor() {
On 2010/03/24 00:05:54, Evan Stade wrote:
> did you mean to add this function twice?

Oh FFS.  New snap up, again :/

[Evan Stade, 2010-03-24 00:18:42]

assuming it looks good in gtk theme mode (particularly with dark themes), LGTM

[Peter Kasting, 2010-03-24 00:20:27]

On 2010/03/24 00:18:42, Evan Stade wrote:
> assuming it looks good in gtk theme mode (particularly with dark themes), LGTM

I strongly suspect that at least some themes don't look good -- but that's true
of the old code too.  I wrote code a while back to automatically adjust colors
for readability over various different backgrounds, but we only seem to call it
on Windows.

[Evan Stade, 2010-03-24 00:25:43]

pretend I said "looks no worse in gtk theme mode"