Page info model now shows red skull and crossbones on SECURITY_STYLE_AUTHENTI...

Page info model now shows red skull and crossbones on SECURITY_STYLE_AUTHENTICATION_BROKEN, like the Omnibox does.

Started using...
a) the Info icon (blue i) for the first visit section and 
b) the yellow exclamation point for http connections (instead of the skull and crossbones).

BUG=http://crbug.com/52916
TEST=None


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

[wtc, 2010-09-03 19:18:39]

LGTM.

http://codereview.chromium.org/3367007/diff/2002/6001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3367007/diff/2002/6001#newcode5458
chrome/app/generated_resources.grd:5458: Site info
Nit: info => information

assuming we want to avoid abbreviations in the UI.

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
Is it necessary to add this test?

http://codereview.chromium.org/3367007/diff/2002/6004#newcode98
chrome/browser/page_info_model.cc:98: state = ssl.security_style() ==
SECURITY_STYLE_UNAUTHENTICATED ?
This is a good change.

http://codereview.chromium.org/3367007/diff/2002/6005
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3367007/diff/2002/6005#newcode41
chrome/browser/page_info_model.h:41: SECTION_STATE_WARNING_MIXED,
Nit: I think it's better to be verbose here and name this
enum SECTION_STATE_WARNING_MIXED_CONTENT.

Re: the "unable to check certificate revocation" error:
I don't want to show scary UI for this certificate error
because other browsers all ignore this certificate error.

So can you have a SectionInfoState value for the
"unable to check certificate revocation" error?

http://codereview.chromium.org/3367007/diff/2002/6006
File chrome/browser/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6006#newcode192
chrome/browser/views/page_info_bubble_view.cc:192:
status_image_->SetImage(rb.GetBitmapNamed(IDR_PAGEINFO_GOOD));
Please ask someone else to review the heavy-duty
UI code in this file.  I am not qualified...

[Finnur, 2010-09-06 09:33:00]

Jay, can you take over for the bubble UI parts?

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
Yes, I believe so. 

This page: https://websolutions.tsd-inc.com/hengehold/Reservations.aspx

... used to show a red-skull in the omnibox, but once you clicked the skull the
popup would show a green padlock. The omnibox is explicitly checking for this
security style.

http://codereview.chromium.org/3367007/diff/2002/6005
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3367007/diff/2002/6005#newcode41
chrome/browser/page_info_model.h:41: SECTION_STATE_WARNING_MIXED,
I think you are misunderstanding the purpose of this enum. It is meant as a
guidance for which of our different icons we show. OK is green padlock,
mixed_content is gray padlock, warning_http is orange exclamation point and
error is skull and bones.

I assume that unable to "check certificate revocation" could be shown either as
a green padlock or as gray padlock, both of which are already defined...

On 2010/09/03 19:18:39, wtc wrote:
> Nit: I think it's better to be verbose here and name this
> enum SECTION_STATE_WARNING_MIXED_CONTENT.
> 
> Re: the "unable to check certificate revocation" error:
> I don't want to show scary UI for this certificate error
> because other browsers all ignore this certificate error.
> 
> So can you have a SectionInfoState value for the
> "unable to check certificate revocation" error?

http://codereview.chromium.org/3367007/diff/2002/6006
File chrome/browser/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6006#newcode192
chrome/browser/views/page_info_bubble_view.cc:192:
status_image_->SetImage(rb.GetBitmapNamed(IDR_PAGEINFO_GOOD));
Sure.

[wtc, 2010-09-07 18:31:02]

Finnur: thank you for your reply.  I responded to your comments
below.  I hope you will consider at least the second comment.

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
Thank you for the explanation.

I understand why the omnibox needs to check for the
ssl.security_style() != SECURITY_STYLE_AUTHENTICATION_BROKEN
condition.  However, for the page info model, it's less
obvious whether mixed content should change the identity
section because mixed content is covered by the connection
section.

If mixed content should degrade the identity section, I
think it's helpful to say something like "The identity of
this website has been verified by XXX CA.  However, the
page contains some insecure content, so the identity of the
website is not trustworthy."  This makes it clear why we
degraded the identity section in the page info model.

http://codereview.chromium.org/3367007/diff/2002/6005
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3367007/diff/2002/6005#newcode41
chrome/browser/page_info_model.h:41: SECTION_STATE_WARNING_MIXED,
On 2010/09/06 09:33:00, Finnur wrote:
> I think you are misunderstanding the purpose of this enum. It is meant as a
> guidance for which of our different icons we show. OK is green padlock,
> mixed_content is gray padlock, warning_http is orange exclamation point and
> error is skull and bones.
> 
> I assume that unable to "check certificate revocation" could be shown either
as
> a green padlock or as gray padlock, both of which are already defined...

I want to use a gray padlock for the "unable to check
certificate revocation" error.

This means I will need to use SECTION_STATE_WARNING_MIXED_CONTENT
for that error.  It's a little confusing.  I can deal with
that with a comment.

Would it be OK to rename these two enum constants something
like SECTION_STATE_WARNING_MINOR and
SECTION_STATE_WARNING_MAJOR?  This way they won't be
specifically tied to "mixed content" and "non-SSL HTTP".

[Jay Civelli, 2010-09-07 20:16:03]

http://codereview.chromium.org/3367007/diff/12001/13006
File chrome/browser/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/3367007/diff/12001/13006#newcode184
chrome/browser/views/page_info_bubble_view.cc:184: link_(NULL) {
Initialize status_image_ to NULL.

[Finnur, 2010-09-08 00:20:11]

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
Maybe I'm interpreting things wrong here, but I believe the intent with this
whole thing is to map the Omnibox status (gray padlock, red skull) into similar
icons in this bubble. So, if you got a red skull and bones status in the
Omnibox, clicking on it should show you the info bubble with a corresponding
section that makes it clear why the Omnibox showed you the skull. This change
makes it clear by turning the Identity section Red (as opposed to just showing
the gray padlock on the Connection section). I checked FF for this site and it
shows: "This site does not provide identity information". I thought (maybe
incorrectly) that meant we should follow suit.

I don't have a good understanding of the underlying mechanism to know how mixed
content even enters the picture here... Is this style only set if we have mixed
content? 

OK, let me turn the question around: If the only problem on this site is that
there is mixed content, why isn't the Omnibox showing the gray padlock instead
of the skull and bones?

On 2010/09/07 18:31:02, wtc wrote:
> Thank you for the explanation.
> 
> I understand why the omnibox needs to check for the
> ssl.security_style() != SECURITY_STYLE_AUTHENTICATION_BROKEN
> condition.  However, for the page info model, it's less
> obvious whether mixed content should change the identity
> section because mixed content is covered by the connection
> section.
> 
> If mixed content should degrade the identity section, I
> think it's helpful to say something like "The identity of
> this website has been verified by XXX CA.  However, the
> page contains some insecure content, so the identity of the
> website is not trustworthy."  This makes it clear why we
> degraded the identity section in the page info model.

http://codereview.chromium.org/3367007/diff/2002/6005
File chrome/browser/page_info_model.h (right):

http://codereview.chromium.org/3367007/diff/2002/6005#newcode41
chrome/browser/page_info_model.h:41: SECTION_STATE_WARNING_MIXED,
> Would it be OK to rename these two enum constants something
> like SECTION_STATE_WARNING_MINOR and
> SECTION_STATE_WARNING_MAJOR?  This way they won't be
> specifically tied to "mixed content" and "non-SSL HTTP".

Sure. I'll make that change. That sounds reasonable.

http://codereview.chromium.org/3367007/diff/12001/13006
File chrome/browser/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/3367007/diff/12001/13006#newcode184
chrome/browser/views/page_info_bubble_view.cc:184: link_(NULL) {
But otherwise LGTY or is judgement pending based on how the discussions go? :)

On 2010/09/07 20:16:03, Jay Civelli wrote:
> Initialize status_image_ to NULL.

[jcivelli, 2010-09-08 00:27:15]

LGTM

[Finnur, 2010-09-08 11:38:30]

Updated to address comments. Wan-Teh, can you take another look (and look at my
reply above). Thanks!

[wtc, 2010-09-09 18:00:51]

LGTM.

My suggestion of "WARNING_MAJOR" and "WARNING_MINOR" instead
of "MIXED_CONTENT" and "HTTP" makes the code harder to
understand for me.  If you feel the same way, you can change
them back.

http://codereview.chromium.org/3367007/diff/32001/33002
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/3367007/diff/32001/33002#newcode247
chrome/app/theme/theme_resources.grd:247: <include
name="IDR_PAGEINFO_WARNING_MAJOR" file="pageinfo_warning.png" type="BINDATA" />
Nit: the PNG files pageinfo_warning.png and pageinfo_mixed.png
should be renamed to match the new names IDR_PAGEINFO_WARNING_MAJOR
and IDR_PAGEINFO_WARNING_MINOR.

http://codereview.chromium.org/3367007/diff/32001/33003
File chrome/browser/gtk/page_info_bubble_gtk.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33003#newcode226
chrome/browser/gtk/page_info_bubble_gtk.cc:226: case
PageInfoModel::SECTION_STATE_WARNING_MINOR:
Should we add a case for PageInfoModel::SECTION_STATE_WARNING_MAJOR
to this switch statement?

http://codereview.chromium.org/3367007/diff/32001/33004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33004#newcode95
chrome/browser/page_info_model.cc:95: // HTTP or bad HTTPS.
After making the change on line 46 above, this comment should
read:
    HTTP, mixed content, or bad HTTPS.

http://codereview.chromium.org/3367007/diff/32001/33004#newcode136
chrome/browser/page_info_model.cc:136: if
(command_line->HasSwitch(switches::kEnableNewPageInfoBubble))
I don't understand why 'state' depends on
switches::kEnableNewPageInfoBubble here.

It seems that the page info "model" should be independent
of the "views".  Page Info Bubble is just a new way to
present/view the page info model.

http://codereview.chromium.org/3367007/diff/32001/33007
File chrome/browser/views/page_info_window_view.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33007#newcode349
chrome/browser/views/page_info_window_view.cc:349: bad_state_icon_ =
rb.GetBitmapNamed(IDR_PAGEINFO_WARNING_MAJOR);
Nit: the bad_state_icon_  member should be renamed
major_warning_state_icon_.

[wtc, 2010-09-09 20:30:29]

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
On 2010/09/08 00:20:11, Finnur wrote:
>
> OK, let me turn the question around: If the only problem on this site is that
> there is mixed content, why isn't the Omnibox showing the gray padlock instead
> of the skull and bones?

I seem to remember that we distinguish between
ssl.displayed_insecure_content() (for example, images loaded
over HTTP) and ssl.ran_insecure_content() (JavaScript loaded
over HTTP).  The latter can cause serious problems.  The
former is less harmful, and is impossible to avoid when you
do Web mail over HTTPS and open an HTML email message with
an embedded image with HTTP URL.

So it seems that if ssl.displayed_insecure_content() is true
but ssl.ran_insecure_content() is false, the Omnibox should
show the gray padlock.

For https://websolutions.tsd-inc.com/hengehold/Reservations.aspx,
both ssl.displayed_insecure_content() and ssl.ran_insecure_content()
are true.  That may be why it gets a red-skull.

I believe the Omnibox's security style is the summary of the
security styles of the Identity and Connection sections of
the page info model.  So a serious error in the Connection
section alone is enough to cause the Omnibox show the red
skull.  Conversely, a red-skull in the Omnibox does not
imply both the Identity and Connection sections have serious
errors.

But this is just my interpretation.  Perhaps when we have
run insecure content, the Identity of a website that has
a good certificate is no longer trustworthy.

[Finnur, 2010-09-09 23:27:16]

Code not updated yet. Need to do more testing. This is just to answer questions,
to get one extra cycle of discussion before the end of day in MTV (hopefully).

> My suggestion of "WARNING_MAJOR" and "WARNING_MINOR"
> instead of "MIXED_CONTENT" and "HTTP" makes the code
> harder to understand for me.  If you feel the same way,
> you can change them back.

I'll think about it.

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
> For https://websolutions.tsd-inc.com/hengehold/Reservations.aspx,
> both ssl.displayed_insecure_content() and ssl.ran_insecure_content()
> are true.  That may be why it gets a red-skull.

The Omnibox shows the skull because of security style ==
SECURITY_STYLE_AUTHENTICATION_BROKEN. I'm going to remove this check in favor of
showing the skull on ran_insecure_content. However, unless ran_insecure_content
always implies SECURITY_STYLE_AUTHENTICATION_BROKEN, then we have a discrepancy
between the Omnibox and the Infobubble.

On 2010/09/09 20:30:29, wtc wrote:
> On 2010/09/08 00:20:11, Finnur wrote:
> >
> > OK, let me turn the question around: If the only problem on this site is
that
> > there is mixed content, why isn't the Omnibox showing the gray padlock
instead
> > of the skull and bones?
> 
> I seem to remember that we distinguish between
> ssl.displayed_insecure_content() (for example, images loaded
> over HTTP) and ssl.ran_insecure_content() (JavaScript loaded
> over HTTP).  The latter can cause serious problems.  The
> former is less harmful, and is impossible to avoid when you
> do Web mail over HTTPS and open an HTML email message with
> an embedded image with HTTP URL.
> 
> So it seems that if ssl.displayed_insecure_content() is true
> but ssl.ran_insecure_content() is false, the Omnibox should
> show the gray padlock.
> 
> For https://websolutions.tsd-inc.com/hengehold/Reservations.aspx,
> both ssl.displayed_insecure_content() and ssl.ran_insecure_content()
> are true.  That may be why it gets a red-skull.
> 
> I believe the Omnibox's security style is the summary of the
> security styles of the Identity and Connection sections of
> the page info model.  So a serious error in the Connection
> section alone is enough to cause the Omnibox show the red
> skull.  Conversely, a red-skull in the Omnibox does not
> imply both the Identity and Connection sections have serious
> errors.
> 
> But this is just my interpretation.  Perhaps when we have
> run insecure content, the Identity of a website that has
> a good certificate is no longer trustworthy.

http://codereview.chromium.org/3367007/diff/32001/33003
File chrome/browser/gtk/page_info_bubble_gtk.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33003#newcode226
chrome/browser/gtk/page_info_bubble_gtk.cc:226: case
PageInfoModel::SECTION_STATE_WARNING_MINOR:
Yes, good catch.

On 2010/09/09 18:00:51, wtc wrote:
> Should we add a case for PageInfoModel::SECTION_STATE_WARNING_MAJOR
> to this switch statement?

http://codereview.chromium.org/3367007/diff/32001/33004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33004#newcode95
chrome/browser/page_info_model.cc:95: // HTTP or bad HTTPS.
Wait... are you saying mixed content sometimes/always results in
SECURITY_STYLE_AUTHENTICATION_BROKEN?

On 2010/09/09 18:00:51, wtc wrote:
> After making the change on line 46 above, this comment should
> read:
>     HTTP, mixed content, or bad HTTPS.

http://codereview.chromium.org/3367007/diff/32001/33004#newcode136
chrome/browser/page_info_model.cc:136: if
(command_line->HasSwitch(switches::kEnableNewPageInfoBubble))
I've added a comment to explain this. This isn't so much a dependence on views
as it is a differentiation between: in the old code this was an error, in the
new code it is a warning.

On 2010/09/09 18:00:51, wtc wrote:
> I don't understand why 'state' depends on
> switches::kEnableNewPageInfoBubble here.
> 
> It seems that the page info "model" should be independent
> of the "views".  Page Info Bubble is just a new way to
> present/view the page info model.

http://codereview.chromium.org/3367007/diff/32001/33007
File chrome/browser/views/page_info_window_view.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33007#newcode349
chrome/browser/views/page_info_window_view.cc:349: bad_state_icon_ =
rb.GetBitmapNamed(IDR_PAGEINFO_WARNING_MAJOR);
This is all throw-away code, once we get settled on the new bubble. I want to
spend as little effort as I can on it. :)

On 2010/09/09 18:00:51, wtc wrote:
> Nit: the bad_state_icon_  member should be renamed
> major_warning_state_icon_.

[wtc, 2010-09-09 23:43:22]

Finnur, thank you for being considerate.  I found the code
that answers your questions.

http://codereview.chromium.org/3367007/diff/2002/6004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
chrome/browser/page_info_model.cc:46: ssl.security_style() !=
SECURITY_STYLE_AUTHENTICATION_BROKEN &&
On 2010/09/09 23:27:16, Finnur wrote:
>
> However, unless ran_insecure_content
> always implies SECURITY_STYLE_AUTHENTICATION_BROKEN, then we have a
discrepancy
> between the Omnibox and the Infobubble.

I believe ran_insecure_content always implies
SECURITY_STYLE_AUTHENTICATION_BROKEN.  See the following
code in ssl_policy.cc:

  SiteInstance* site_instance = entry->site_instance();
  // Note that |site_instance| can be NULL here because NavigationEntries don't
  // necessarily have site instances.  Without a process, the entry can't
  // possibly have insecure content.  See bug http://crbug.com/12423.
  if (site_instance &&
      backend_->DidHostRunInsecureContent(entry->url().host(),
                                          site_instance->GetProcess()->id())) {
    entry->ssl().set_security_style(SECURITY_STYLE_AUTHENTICATION_BROKEN);
    entry->ssl().set_ran_insecure_content();
    return;
  }

  if (tab_contents->displayed_insecure_content())
    entry->ssl().set_displayed_insecure_content();

Note that displayed_insecure_content() alone does NOT
imply SECURITY_STYLE_AUTHENTICATION_BROKEN.

[Finnur, 2010-09-10 10:13:16]

Thanks for the snippet. It really helped clarify things. I have made adjustments
based on your comments (removed the check for the style and added a check for
ran_insecure_contents instead).

> My suggestion of "WARNING_MAJOR" and "WARNING_MINOR" instead
> of "MIXED_CONTENT" and "HTTP" makes the code harder to
> understand for me.  If you feel the same way, you can change
> them back.

I struggled with this, but decided not to change it back. The reason is that
this feels more correct. For example, we want to use the orange exclamation
point as bad state for the Site Information section, and setting the state to
SECTION_STATE_WARNING_HTTP in that case doesn't make sense.

I've submitted my changes to the try server and will check in once green. Thanks
for your comments and help.

On 2010/09/09 23:43:22, wtc wrote:
> Finnur, thank you for being considerate.  I found the code
> that answers your questions.
> 
> http://codereview.chromium.org/3367007/diff/2002/6004
> File chrome/browser/page_info_model.cc (right):
> 
> http://codereview.chromium.org/3367007/diff/2002/6004#newcode46
> chrome/browser/page_info_model.cc:46: ssl.security_style() !=
> SECURITY_STYLE_AUTHENTICATION_BROKEN &&
> On 2010/09/09 23:27:16, Finnur wrote:
> >
> > However, unless ran_insecure_content
> > always implies SECURITY_STYLE_AUTHENTICATION_BROKEN, then we have a
> discrepancy
> > between the Omnibox and the Infobubble.
> 
> I believe ran_insecure_content always implies
> SECURITY_STYLE_AUTHENTICATION_BROKEN.  See the following
> code in ssl_policy.cc:
> 
>   SiteInstance* site_instance = entry->site_instance();
>   // Note that |site_instance| can be NULL here because NavigationEntries
don't
>   // necessarily have site instances.  Without a process, the entry can't
>   // possibly have insecure content.  See bug http://crbug.com/12423.
>   if (site_instance &&
>       backend_->DidHostRunInsecureContent(entry->url().host(),
>                                           site_instance->GetProcess()->id()))
{
>     entry->ssl().set_security_style(SECURITY_STYLE_AUTHENTICATION_BROKEN);
>     entry->ssl().set_ran_insecure_content();
>     return;
>   }
> 
>   if (tab_contents->displayed_insecure_content())
>     entry->ssl().set_displayed_insecure_content();
> 
> Note that displayed_insecure_content() alone does NOT
> imply SECURITY_STYLE_AUTHENTICATION_BROKEN.

[Finnur, 2010-09-10 10:13:40]

Oops, forgot to Reply doesn't publish comments entered. Here they are...

http://codereview.chromium.org/3367007/diff/32001/33002
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/3367007/diff/32001/33002#newcode247
chrome/app/theme/theme_resources.grd:247: <include
name="IDR_PAGEINFO_WARNING_MAJOR" file="pageinfo_warning.png" type="BINDATA" />
I'll do that in a followup cl so I can still use the try servers (they don't
like such changes). 

On 2010/09/09 18:00:51, wtc wrote:
> Nit: the PNG files pageinfo_warning.png and pageinfo_mixed.png
> should be renamed to match the new names IDR_PAGEINFO_WARNING_MAJOR
> and IDR_PAGEINFO_WARNING_MINOR.

http://codereview.chromium.org/3367007/diff/32001/33004
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3367007/diff/32001/33004#newcode95
chrome/browser/page_info_model.cc:95: // HTTP or bad HTTPS.
Remove line 46, so this no longer applies.

[Finnur, 2010-09-10 10:14:21]

Correction: Forgot *that* reply doesn't publish comments entered.

[wtc, 2010-09-10 19:42:36]

LGTM.