Add button to page info to revoke user certificate decisions.

Add button to page info to revoke user certificate decisions

Currently, if a user clicks 'proceed' through an SSL interstitial, their only
way to revoke this decision is to restart their browser session. This commit
provides an explicit button in the page info to revoke this decision if the
user is using the remember certificate error decisions experiment. Since
decisions are remembered across browser restarts for these users, it's
important to provide them with a mechanism to revoke their decision later.

BUG=262615
NOTRY=true

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

[jww, 2014-07-26 02:28:49]

Hi everyone. This CL adds a button so users can choose to stop allowing an
invalid certificate they've clicked 'proceed' through in an interstitial in the
past (ainsle@, CC'd, is the designer who looked this over).

Since this touches the page info on different platforms, I've requested a few
different reviewers:

tedchoc@chromium.org: Please review changes in chrome/android and
chrome/browser/ui/android

creis@chromium.org: Please review changes in content/browser/ssl/*

pkasting@chromium.org: Please review changes in chrome/browser/ui/ (not
android).

Thanks!

[Peter Kasting, 2014-07-26 02:34:11]

FYI, I can't review ObjC code.  I can review the non-cocoa UI pieces.

[Peter Kasting, 2014-07-26 02:40:40]

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:385:
const ui::Event& event) {
Why do we still have this function if it's empty?

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:829:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
Nit: Wrap like this:

    content::SSLHostState::GetFor(context)->RevokeAllowAndDenyPreferences(
        presenter_->GetSiteURL().host());

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165:
views::Link* reset_decisions_button_;
Your comment and variable name call this a button, but the type is Link.  These
need to be consistent.  If clicking this takes some action, as opposed to going
to another page or something, this should actually be a button, not a link.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:537: // Check if a user
decision has been made to allow or deny some certificates
Nit: What does the "some" inserted in this sentence mean?  The sentence reads
more like I expect without that.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.h:121: // permissions and
site information.
Nit: Remove this comment.  The comment on the member declaration already gives
this information.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.h:122: const GURL&
GetSiteURL() const { return site_url_; }
Nit: Inlined accessors should be named unix_hacker()-style, with the same name
as the underlying member.

Place this up with the other accessors, in the same relative order as the
members are declared in.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings_ui.h:98: // Specifies if the
user has made an explicit decision about how some
Nit: "some" again.

[jww, 2014-07-26 16:17:48]

On 2014/07/26 02:34:11, Peter Kasting wrote:
> FYI, I can't review ObjC code.  I can review the non-cocoa UI pieces.

Thanks for the clarification, Peter!

rsesek@, can you take a look at the cocoa files? Thanks!

[jww, 2014-07-27 17:29:26]

pkasting@, I addressed some of your comments and replied to others.

I also made one larger refactor because I missed a presubmit warning earlier. I
needed to move ssl_host_state.h to content/public/browser/ssl_host_state.h, and
thus now have conten/browser/ssl/ssl_host_state_impl[.cc|.h] as well.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:385:
const ui::Event& event) {
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Why do we still have this function if it's empty?

The removal of GetWidget()->Close(); is an accident, so I'll change that back.
Not sure where this method is used at all, though.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:829:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Nit: Wrap like this:
> 
>     content::SSLHostState::GetFor(context)->RevokeAllowAndDenyPreferences(
>         presenter_->GetSiteURL().host());

This is actually a git-cl format change. Happy to change to your style if you
have a strong preference.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165:
views::Link* reset_decisions_button_;
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Your comment and variable name call this a button, but the type is Link. 
These
> need to be consistent.  If clicking this takes some action, as opposed to
going
> to another page or something, this should actually be a button, not a link.

After going over the designs with ainsle@, this a link-like text even though it
is resetting the security decisions (and only closing the page info). This is
also consistent with the new SSL error interstitial, where, for example, the
proceed decision itself is a link, not a button. Would it still be better to
change the comment and variable this to 'link'?

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:537: // Check if a user
decision has been made to allow or deny some certificates
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Nit: What does the "some" inserted in this sentence mean?  The sentence reads
> more like I expect without that.

Yup, the "some" was extraneous. Fixed.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.h:121: // permissions and
site information.
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Nit: Remove this comment.  The comment on the member declaration already gives
> this information.

Done.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.h:122: const GURL&
GetSiteURL() const { return site_url_; }
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Nit: Inlined accessors should be named unix_hacker()-style, with the same name
> as the underlying member.
> 
> Place this up with the other accessors, in the same relative order as the
> members are declared in.

Done.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings_ui.h:98: // Specifies if the
user has made an explicit decision about how some
On 2014/07/26 02:40:39, Peter Kasting wrote:
> Nit: "some" again.

This one is actually slightly different, so I rewrote the comment to hopefully
increase clarity.

[Peter Kasting, 2014-07-28 11:07:45]

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:829:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
On 2014/07/27 17:29:26, jww wrote:
> On 2014/07/26 02:40:39, Peter Kasting wrote:
> > Nit: Wrap like this:
> > 
> >     content::SSLHostState::GetFor(context)->RevokeAllowAndDenyPreferences(
> >         presenter_->GetSiteURL().host());
> 
> This is actually a git-cl format change. Happy to change to your style if you
> have a strong preference.

I'm not sure I'd say "strong" preference, but I've never seen where in the style
guide (which otherwise says to wrap after operators) the exception for the dot
and arrow operators is defined -- but git-cl format will happily wrap before
them.

Personally, this is something like reason number 752 of 1034 never to run git-cl
format.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:165:
views::Link* reset_decisions_button_;
On 2014/07/27 17:29:26, jww wrote:
> On 2014/07/26 02:40:39, Peter Kasting wrote:
> > Your comment and variable name call this a button, but the type is Link. 
> These
> > need to be consistent.  If clicking this takes some action, as opposed to
> going
> > to another page or something, this should actually be a button, not a link.
> 
> After going over the designs with ainsle@, this a link-like text even though
it
> is resetting the security decisions (and only closing the page info). This is
> also consistent with the new SSL error interstitial, where, for example, the
> proceed decision itself is a link, not a button. Would it still be better to
> change the comment and variable this to 'link'?

An interstitial is web content, and a clickable target that causes that
interstitial to be navigated away makes sense to be a link.  The page info
dropdown is native UI, and buttons that take actions to affect browser state
should be buttons, not links.  We used to be inconsistent about this, e.g. by
having a "Remove" link (instead of button) on the star bubble, but we've fixed
all the cases I'm aware of, so I'm going to stand firm on this one.  If
necessary, put me in touch with Alex directly.

[Robert Sesek, 2014-07-28 15:42:51]

cocoa/ LGTM

https://codereview.chromium.org/418133012/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):

https://codereview.chromium.org/418133012/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:524:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
Whatever you and pkasting decide about wrapping, apply it here, too. I prefer
the style Peter's describing, fwiw.

[jww, 2014-07-28 19:10:00]

I've (hopefully) addressed the nits in the CL other than the change from link to
button. I'll try to get that change up by the end of the day.

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:829:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
On 2014/07/28 11:07:44, Peter Kasting wrote:
> On 2014/07/27 17:29:26, jww wrote:
> > On 2014/07/26 02:40:39, Peter Kasting wrote:
> > > Nit: Wrap like this:
> > > 
> > >     content::SSLHostState::GetFor(context)->RevokeAllowAndDenyPreferences(
> > >         presenter_->GetSiteURL().host());
> > 
> > This is actually a git-cl format change. Happy to change to your style if
you
> > have a strong preference.
> 
> I'm not sure I'd say "strong" preference, but I've never seen where in the
style
> guide (which otherwise says to wrap after operators) the exception for the dot
> and arrow operators is defined -- but git-cl format will happily wrap before
> them.
> 
> Personally, this is something like reason number 752 of 1034 never to run
git-cl
> format.

Done.

https://codereview.chromium.org/418133012/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):

https://codereview.chromium.org/418133012/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:524:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
On 2014/07/28 15:42:51, rsesek wrote:
> Whatever you and pkasting decide about wrapping, apply it here, too. I prefer
> the style Peter's describing, fwiw.

Done.

[Ted C, 2014-07-28 20:15:50]

lgtm w/ style nits

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:47:
private final long mNativeWebsiteSettingsPopup;
I would move this up with the other final fields.

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:155:
mCertificateLayout.addView(mResetCertDecisions);
I find it kind of odd that this is a text view that takes a "destructive" action
where the other links in this dialog are just taking you to some other page.

But this looks to be the same general idea as the other platforms, so I guess
this is fine.

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:206:
nativeResetCertDecisions(mNativeWebsiteSettingsPopup, mWebContents);
+2 indent since you're in java land

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:227:
private native void nativeResetCertDecisions(long
nativeWebsiteSettingsPopupAndroid, WebContents webContents);
line length limit in java is 100, so you need to wrap these params.

https://codereview.chromium.org/418133012/diff/80001/chrome/browser/ui/androi...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/418133012/diff/80001/chrome/browser/ui/androi...
chrome/browser/ui/android/website_settings_popup_android.cc:120:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
same line wrapping nit as the other files

[Charlie Reis, 2014-07-28 20:16:25]

[+jam for content/public additions]

https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
File content/public/browser/ssl_host_state.h (right):

https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
content/public/browser/ssl_host_state.h:31: class SSLHostState :
NON_EXPORTED_BASE(base::SupportsUserData::Data),
We're generally very stingy with adding new concepts and APIs to the public
content API, so I'm skeptical about moving this whole class into content/public.

Are all of these methods needed outside content?  Can we limit the public
interface to the bare minimum?

[jam, 2014-07-28 23:06:21]

https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
File content/public/browser/ssl_host_state.h (right):

https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
content/public/browser/ssl_host_state.h:31: class SSLHostState :
NON_EXPORTED_BASE(base::SupportsUserData::Data),
On 2014/07/28 20:16:24, Charlie Reis wrote:
> We're generally very stingy with adding new concepts and APIs to the public
> content API, so I'm skeptical about moving this whole class into
content/public.
> 
> Are all of these methods needed outside content?  Can we limit the public
> interface to the bare minimum?

correct, Joe: see http://www.chromium.org/developers/content-module/content-api
for the guidelines
it looks like only RevokeAllowAndDenyPreferences and HasAllowedOrDeniedCert are
called. Do you expect to use more from SSLHostState? If so, then having an
interface (with only the called methods being public) is ok. However if it's
just these two, perhaps they can be static methods inside BrowserContext?

[jam, 2014-07-31 21:45:15]

On 2014/07/28 23:06:21, jam wrote:
>
https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
> File content/public/browser/ssl_host_state.h (right):
> 
>
https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
> content/public/browser/ssl_host_state.h:31: class SSLHostState :
> NON_EXPORTED_BASE(base::SupportsUserData::Data),
> On 2014/07/28 20:16:24, Charlie Reis wrote:
> > We're generally very stingy with adding new concepts and APIs to the public
> > content API, so I'm skeptical about moving this whole class into
> content/public.
> > 
> > Are all of these methods needed outside content?  Can we limit the public
> > interface to the bare minimum?
> 
> correct, Joe: see
http://www.chromium.org/developers/content-module/content-api
> for the guidelines
> it looks like only RevokeAllowAndDenyPreferences and HasAllowedOrDeniedCert
are
> called. Do you expect to use more from SSLHostState? If so, then having an
> interface (with only the called methods being public) is ok. However if it's
> just these two, perhaps they can be static methods inside BrowserContext?

btw since it looks like there's something resembling an SSLHostStateDelegate
being added in
https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...,
i'm fine with adding an SSLHostState interface here with just the required
methods. then it can also have a SetDelegate method to take the new interface

[jww, 2014-08-06 00:51:47]

pkasting, all links are now buttons. Please let me know if this looks good.

jam@, I reduced the SSLHostState API as per our discussion, but since all the
call sites have a BrowserContext, I think we should just remove the SSLHostState
API and have them call GetSSLHostStateDelegate() on the BrowserContext
explicitly. Seems like that eliminates a now completely unnecessary class. What
do you think?

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:47:
private final long mNativeWebsiteSettingsPopup;
On 2014/07/28 20:15:49, Ted C wrote:
> I would move this up with the other final fields.

Done.

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:155:
mCertificateLayout.addView(mResetCertDecisions);
On 2014/07/28 20:15:50, Ted C wrote:
> I find it kind of odd that this is a text view that takes a "destructive"
action
> where the other links in this dialog are just taking you to some other page.
> 
> But this looks to be the same general idea as the other platforms, so I guess
> this is fine.
As per pkasting's comment, I've changed these all to buttons.

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:206:
nativeResetCertDecisions(mNativeWebsiteSettingsPopup, mWebContents);
On 2014/07/28 20:15:49, Ted C wrote:
> +2 indent since you're in java land

Done.

https://codereview.chromium.org/418133012/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:227:
private native void nativeResetCertDecisions(long
nativeWebsiteSettingsPopupAndroid, WebContents webContents);
On 2014/07/28 20:15:49, Ted C wrote:
> line length limit in java is 100, so you need to wrap these params.

Done.

https://codereview.chromium.org/418133012/diff/80001/chrome/browser/ui/androi...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/418133012/diff/80001/chrome/browser/ui/androi...
chrome/browser/ui/android/website_settings_popup_android.cc:120:
->RevokeAllowAndDenyPreferences(presenter_->GetSiteURL().host());
On 2014/07/28 20:15:50, Ted C wrote:
> same line wrapping nit as the other files

Done.

https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
File content/public/browser/ssl_host_state.h (right):

https://codereview.chromium.org/418133012/diff/80001/content/public/browser/s...
content/public/browser/ssl_host_state.h:31: class SSLHostState :
NON_EXPORTED_BASE(base::SupportsUserData::Data),
On 2014/07/28 23:06:20, jam wrote:
> On 2014/07/28 20:16:24, Charlie Reis wrote:
> > We're generally very stingy with adding new concepts and APIs to the public
> > content API, so I'm skeptical about moving this whole class into
> content/public.
> > 
> > Are all of these methods needed outside content?  Can we limit the public
> > interface to the bare minimum?
> 
> correct, Joe: see
http://www.chromium.org/developers/content-module/content-api
> for the guidelines
> it looks like only RevokeAllowAndDenyPreferences and HasAllowedOrDeniedCert
are
> called. Do you expect to use more from SSLHostState? If so, then having an
> interface (with only the called methods being public) is ok. However if it's
> just these two, perhaps they can be static methods inside BrowserContext?

I particularly like the BrowserContext suggestion; I'll add the methods there.

[Ted C, 2014-08-06 01:01:05]

still lgtm (small indenting nit)

https://codereview.chromium.org/418133012/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/418133012/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:239:
long nativeWebsiteSettingsPopupAndroid, WebContents webContents);
+4 indent

[jww, 2014-08-06 01:16:08]

https://codereview.chromium.org/418133012/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/418133012/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:239:
long nativeWebsiteSettingsPopupAndroid, WebContents webContents);
On 2014/08/06 01:01:05, Ted C wrote:
> +4 indent

Darn, I thought I got all of those :-) Done.

[Peter Kasting, 2014-08-06 01:19:07]

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:399:
base::Bind(&WebsiteSettingsPopupView::HandleButtonPressedAsync,
Do you really need to do this asynchronously?  The LinkClicked() machinery has
commentary explaining why it does things that way, but your newly added code has
no such commentary.

Honestly, I'm uncomfortable even with how the link click handling here is done. 
I don't think handling actions that can destroy the window should be
accomplished by making the click handling async, because then the click handling
gets pushed behind whatever else is in the message queue, which could be who
knows how much stuff.

The normal way we deal with things like this is to disconnect and/or hide the
widget synchronously, but tell it to close asynchronously, so that from the
user's perspective, everything we do appears synchronous.  Another way is to
ensure that the calling framework doesn't attempt to do any further processing
after this point.

I'm not going to tell you to fix all this in this patch, but it would be nice to
not add more such code unless we really need it; and especially if we DO need
it, I hope you can come back as a followup and fix this architecture.  You might
want to ask sky for advice as well, he's more familiar with event
handling/widget closure than I am and might be able to consult on technical
details.

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:539:
content::SSLHostState host_state(profile);
Nit: Optional: Could just inline "content::SSLHostState(profile).HasAllowed..."
into next line

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.h:184: // errors for this
host should be handled.
Nit: See comment in website_settings_ui.h about this sort of descriptor

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:99: // at least one
certificate error for this host.
Nit: This comment is a little odd, mostly because of the "at least one error"
bit.  I'd normally have expected the comment to say something like "how to
handle certificate errors", i.e. what we care about is that this affects how we
deal with errors going forward.

Maybe the comment should say something about what this actually means, e.g. "Set
when the user has explicitly acted on a previous certificate error for this
host.  When this is true, ...<description of what we'll do in response>"?

[jww, 2014-08-06 17:58:19]

sky@, can you take a look at the handler changes in
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc as per
pkasting's comment? (In particular, his comment in patch set 7 in
website_settings_popup_view.cc) Thanks!

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:399:
base::Bind(&WebsiteSettingsPopupView::HandleButtonPressedAsync,
On 2014/08/06 01:19:07, Peter Kasting wrote:
> Do you really need to do this asynchronously?  The LinkClicked() machinery has
> commentary explaining why it does things that way, but your newly added code
has
> no such commentary.
This UI stuff is a bit of a mystery to me, and I was really just trying to mimic
the behavior of the already working link click handler. Superficially, it
appears that nothing crazy is happening when I make it synchronous, so I'll make
it synchronous in the next patch.
> 
> Honestly, I'm uncomfortable even with how the link click handling here is
done. 
> I don't think handling actions that can destroy the window should be
> accomplished by making the click handling async, because then the click
handling
> gets pushed behind whatever else is in the message queue, which could be who
> knows how much stuff.
> 
> The normal way we deal with things like this is to disconnect and/or hide the
> widget synchronously, but tell it to close asynchronously, so that from the
> user's perspective, everything we do appears synchronous.  Another way is to
> ensure that the calling framework doesn't attempt to do any further processing
> after this point.
> 
> I'm not going to tell you to fix all this in this patch, but it would be nice
to
> not add more such code unless we really need it; and especially if we DO need
> it, I hope you can come back as a followup and fix this architecture.  You
might
> want to ask sky for advice as well, he's more familiar with event
> handling/widget closure than I am and might be able to consult on technical
> details.

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:539:
content::SSLHostState host_state(profile);
On 2014/08/06 01:19:07, Peter Kasting wrote:
> Nit: Optional: Could just inline
"content::SSLHostState(profile).HasAllowed..."
> into next line

Done.

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.h:184: // errors for this
host should be handled.
On 2014/08/06 01:19:07, Peter Kasting wrote:
> Nit: See comment in website_settings_ui.h about this sort of descriptor

Done.

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:99: // at least one
certificate error for this host.
On 2014/08/06 01:19:07, Peter Kasting wrote:
> Nit: This comment is a little odd, mostly because of the "at least one error"
> bit.  I'd normally have expected the comment to say something like "how to
> handle certificate errors", i.e. what we care about is that this affects how
we
> deal with errors going forward.
> 
> Maybe the comment should say something about what this actually means, e.g.
"Set
> when the user has explicitly acted on a previous certificate error for this
> host.  When this is true, ...<description of what we'll do in response>"?

Done.

[sky, 2014-08-06 18:11:22]

On 2014/08/06 17:58:19, jww wrote:
> sky@, can you take a look at the handler changes in
> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc as per
> pkasting's comment? (In particular, his comment in patch set 7 in
> website_settings_popup_view.cc) Thanks!

Totally agree with Peter.
LabelButton is suppose to correctly deal with being deleted while notifiying its
listener. I'm also skeptical of the comment in LinkClicked. Most times when a
widget is closed it's async (close=hide followed by task posted to delete). The
exception to that is if someone explicitly invokes CloseNow (or deletes the
widget).

That said, you should be able to verify this by running under asan or heap
checking enabled.

> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
> File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> (right):
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:399:
> base::Bind(&WebsiteSettingsPopupView::HandleButtonPressedAsync,
> On 2014/08/06 01:19:07, Peter Kasting wrote:
> > Do you really need to do this asynchronously?  The LinkClicked() machinery
has
> > commentary explaining why it does things that way, but your newly added code
> has
> > no such commentary.
> This UI stuff is a bit of a mystery to me, and I was really just trying to
mimic
> the behavior of the already working link click handler. Superficially, it
> appears that nothing crazy is happening when I make it synchronous, so I'll
make
> it synchronous in the next patch.
> > 
> > Honestly, I'm uncomfortable even with how the link click handling here is
> done. 
> > I don't think handling actions that can destroy the window should be
> > accomplished by making the click handling async, because then the click
> handling
> > gets pushed behind whatever else is in the message queue, which could be who
> > knows how much stuff.
> > 
> > The normal way we deal with things like this is to disconnect and/or hide
the
> > widget synchronously, but tell it to close asynchronously, so that from the
> > user's perspective, everything we do appears synchronous.  Another way is to
> > ensure that the calling framework doesn't attempt to do any further
processing
> > after this point.
> > 
> > I'm not going to tell you to fix all this in this patch, but it would be
nice
> to
> > not add more such code unless we really need it; and especially if we DO
need
> > it, I hope you can come back as a followup and fix this architecture.  You
> might
> > want to ask sky for advice as well, he's more familiar with event
> > handling/widget closure than I am and might be able to consult on technical
> > details.
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings.cc (right):
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings.cc:539:
> content::SSLHostState host_state(profile);
> On 2014/08/06 01:19:07, Peter Kasting wrote:
> > Nit: Optional: Could just inline
> "content::SSLHostState(profile).HasAllowed..."
> > into next line
> 
> Done.
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings.h (right):
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings.h:184: // errors for this
> host should be handled.
> On 2014/08/06 01:19:07, Peter Kasting wrote:
> > Nit: See comment in website_settings_ui.h about this sort of descriptor
> 
> Done.
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings_ui.h (right):
> 
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings_ui.h:99: // at least one
> certificate error for this host.
> On 2014/08/06 01:19:07, Peter Kasting wrote:
> > Nit: This comment is a little odd, mostly because of the "at least one
error"
> > bit.  I'd normally have expected the comment to say something like "how to
> > handle certificate errors", i.e. what we care about is that this affects how
> we
> > deal with errors going forward.
> > 
> > Maybe the comment should say something about what this actually means, e.g.
> "Set
> > when the user has explicitly acted on a previous certificate error for this
> > host.  When this is true, ...<description of what we'll do in response>"?
> 
> Done.

[jww, 2014-08-06 20:49:04]

On 2014/08/06 18:11:22, sky wrote:
> On 2014/08/06 17:58:19, jww wrote:
> > sky@, can you take a look at the handler changes in
> > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc as
per
> > pkasting's comment? (In particular, his comment in patch set 7 in
> > website_settings_popup_view.cc) Thanks!
> 
> Totally agree with Peter.
> LabelButton is suppose to correctly deal with being deleted while notifiying
its
> listener. I'm also skeptical of the comment in LinkClicked. Most times when a
> widget is closed it's async (close=hide followed by task posted to delete).
The
> exception to that is if someone explicitly invokes CloseNow (or deletes the
> widget).
> 
> That said, you should be able to verify this by running under asan or heap
> checking enabled.
Thanks! Ran my new non-async version in ASAN and all seems okay with the world.
pkasting, how does the new async version look to you?
> 
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/views...
> > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:399:
> > base::Bind(&WebsiteSettingsPopupView::HandleButtonPressedAsync,
> > On 2014/08/06 01:19:07, Peter Kasting wrote:
> > > Do you really need to do this asynchronously?  The LinkClicked() machinery
> has
> > > commentary explaining why it does things that way, but your newly added
code
> > has
> > > no such commentary.
> > This UI stuff is a bit of a mystery to me, and I was really just trying to
> mimic
> > the behavior of the already working link click handler. Superficially, it
> > appears that nothing crazy is happening when I make it synchronous, so I'll
> make
> > it synchronous in the next patch.
> > > 
> > > Honestly, I'm uncomfortable even with how the link click handling here is
> > done. 
> > > I don't think handling actions that can destroy the window should be
> > > accomplished by making the click handling async, because then the click
> > handling
> > > gets pushed behind whatever else is in the message queue, which could be
who
> > > knows how much stuff.
> > > 
> > > The normal way we deal with things like this is to disconnect and/or hide
> the
> > > widget synchronously, but tell it to close asynchronously, so that from
the
> > > user's perspective, everything we do appears synchronous.  Another way is
to
> > > ensure that the calling framework doesn't attempt to do any further
> processing
> > > after this point.
> > > 
> > > I'm not going to tell you to fix all this in this patch, but it would be
> nice
> > to
> > > not add more such code unless we really need it; and especially if we DO
> need
> > > it, I hope you can come back as a followup and fix this architecture.  You
> > might
> > > want to ask sky for advice as well, he's more familiar with event
> > > handling/widget closure than I am and might be able to consult on
technical
> > > details.
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> > File chrome/browser/ui/website_settings/website_settings.cc (right):
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> > chrome/browser/ui/website_settings/website_settings.cc:539:
> > content::SSLHostState host_state(profile);
> > On 2014/08/06 01:19:07, Peter Kasting wrote:
> > > Nit: Optional: Could just inline
> > "content::SSLHostState(profile).HasAllowed..."
> > > into next line
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> > File chrome/browser/ui/website_settings/website_settings.h (right):
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> > chrome/browser/ui/website_settings/website_settings.h:184: // errors for
this
> > host should be handled.
> > On 2014/08/06 01:19:07, Peter Kasting wrote:
> > > Nit: See comment in website_settings_ui.h about this sort of descriptor
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> > File chrome/browser/ui/website_settings/website_settings_ui.h (right):
> > 
> >
>
https://codereview.chromium.org/418133012/diff/120001/chrome/browser/ui/websi...
> > chrome/browser/ui/website_settings/website_settings_ui.h:99: // at least one
> > certificate error for this host.
> > On 2014/08/06 01:19:07, Peter Kasting wrote:
> > > Nit: This comment is a little odd, mostly because of the "at least one
> error"
> > > bit.  I'd normally have expected the comment to say something like "how to
> > > handle certificate errors", i.e. what we care about is that this affects
how
> > we
> > > deal with errors going forward.
> > > 
> > > Maybe the comment should say something about what this actually means,
e.g.
> > "Set
> > > when the user has explicitly acted on a previous certificate error for
this
> > > host.  When this is true, ...<description of what we'll do in response>"?
> > 
> > Done.

[jam, 2014-08-06 21:27:27]

just looked at content

https://codereview.chromium.org/418133012/diff/180001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.h (left):

https://codereview.chromium.org/418133012/diff/180001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
these 2 files should stay in content/browser/ssl, and should be
ssl_host_state_impl.{h,cc}. the public directory should only have a header with
the interface

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
File content/public/browser/ssl_host_state.h (right):

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:32: : NON_EXPORTED_BASE(public
base::NonThreadSafe) {
nit: get rid of the NonThreadSafe, if you really want it, put it on the
implementation and not the public interface.

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:44: virtual bool
HasAllowedOrDeniedCert(const std::string& host);
nit: document

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:46: void
set_delegate(SSLHostStateDelegate* delegate) { delegate_ = delegate; }
this would be a pure method called SetDelegate

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:54: SSLHostStateDelegate* delegate_;
nit: see http://www.chromium.org/developers/content-module/content-api, this
should only be an interface. the data members can go on the implementation

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:25: class SSLHostStateDelegate
: NON_EXPORTED_BASE(public base::NonThreadSafe) {
remove the NonThreadSafe part, we don't put that on public interface. they're
all assumed to be used on the UI thread unless otherwise mentioned. also, the
embedder shouldn't double check that content is calling it on the right thread

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:27: SSLHostStateDelegate() {}
nit: not needed

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:67:
DISALLOW_COPY_AND_ASSIGN(SSLHostStateDelegate);
nit:not needed on an abstract interface..

[jww, 2014-08-06 21:36:02]

On 2014/08/06 21:27:27, jam wrote:
> just looked at content
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/browser/ssl/ssl...
> File content/browser/ssl/ssl_host_state.h (left):
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/browser/ssl/ssl...
> content/browser/ssl/ssl_host_state.h:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> these 2 files should stay in content/browser/ssl, and should be
> ssl_host_state_impl.{h,cc}. the public directory should only have a header
with
> the interface
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> File content/public/browser/ssl_host_state.h (right):
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state.h:32: : NON_EXPORTED_BASE(public
> base::NonThreadSafe) {
> nit: get rid of the NonThreadSafe, if you really want it, put it on the
> implementation and not the public interface.
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state.h:44: virtual bool
> HasAllowedOrDeniedCert(const std::string& host);
> nit: document
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state.h:46: void
> set_delegate(SSLHostStateDelegate* delegate) { delegate_ = delegate; }
> this would be a pure method called SetDelegate
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state.h:54: SSLHostStateDelegate* delegate_;
> nit: see http://www.chromium.org/developers/content-module/content-api, this
> should only be an interface. the data members can go on the implementation
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> File content/public/browser/ssl_host_state_delegate.h (right):
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state_delegate.h:25: class
SSLHostStateDelegate
> : NON_EXPORTED_BASE(public base::NonThreadSafe) {
> remove the NonThreadSafe part, we don't put that on public interface. they're
> all assumed to be used on the UI thread unless otherwise mentioned. also, the
> embedder shouldn't double check that content is calling it on the right thread
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state_delegate.h:27: SSLHostStateDelegate() {}
> nit: not needed
> 
>
https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
> content/public/browser/ssl_host_state_delegate.h:67:
> DISALLOW_COPY_AND_ASSIGN(SSLHostStateDelegate);
> nit:not needed on an abstract interface..

jam@, what do you think about not using SSLHostState at all anymore? All of the
locations that use it have access to a BrowserContext, which now has a
GetSSLHostStateDelegate() method, so they could theoretically call that and then
call the revoke methods directly? Or am I missing some extra value we get from
having the separate SSLHostState class?

[Peter Kasting, 2014-08-07 03:56:58]

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:390:
base::Bind(&WebsiteSettingsPopupView::HandleLinkClickedAsync,
So can we get rid of this async stuff as well?  Or were you going to do that as
a separate patch?

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:394:
void WebsiteSettingsPopupView::ButtonPressed(views::Button* button,
Nit: Keep function definition order matching declaration order

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.h:186: // bypass the SSL
error for this host.
(See comment in website_settings_ui.h )

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the SSL
error for this host.
This comment is much better now!  Maybe the variable should be named
"[have_]bypassed_ssl_error" or "show_revoke_button" or something to be more
directly connected to the description you give.

It seems a little odd as well that this comment sounds like this is only about
"bypassing an error" but in the code we check if the user has allowed _or
denied_ a cert.  Is that an actual mismatch or am I just misinterpreting the
code?

[jww, 2014-08-08 16:48:50]

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:390:
base::Bind(&WebsiteSettingsPopupView::HandleLinkClickedAsync,
On 2014/08/07 03:56:57, Peter Kasting wrote:
> So can we get rid of this async stuff as well?  Or were you going to do that
as
> a separate patch?

I would rather leave that to someone else since I don't really understand the
ins and outs, unless you think it's as simple as "make it synchronous." Let me
know what you think.

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:394:
void WebsiteSettingsPopupView::ButtonPressed(views::Button* button,
On 2014/08/07 03:56:57, Peter Kasting wrote:
> Nit: Keep function definition order matching declaration order

Done.

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.h:186: // bypass the SSL
error for this host.
On 2014/08/07 03:56:58, Peter Kasting wrote:
> (See comment in website_settings_ui.h )

Acknowledged.

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the SSL
error for this host.
On 2014/08/07 03:56:58, Peter Kasting wrote:
> This comment is much better now!  Maybe the variable should be named
> "[have_]bypassed_ssl_error" or "show_revoke_button" or something to be more
have_bypassed_ssl_error sounds great to me.
> directly connected to the description you give.
> 
> It seems a little odd as well that this comment sounds like this is only about
> "bypassing an error" but in the code we check if the user has allowed _or
> denied_ a cert.  Is that an actual mismatch or am I just misinterpreting the
> code?
Good point. Unfortunately, it's a little of both. We do check both because,
theoretically, there is "Allow" and "Deny," but in practice, "Deny" is never
called. I suppose there is a theoretic world in which we implement a content
setting to "always deny" or something like that, but it doesn't exist.

I'm ambivalent about how to express this in the comment. I suppose it should
probably say something additionally like, "Set when the user has explicitly
bypassed an SSL error for this host or explicitly denied it (the latter of which
is not currently possible in the Chrome UI)." What do you think?

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:25: class SSLHostStateDelegate
: NON_EXPORTED_BASE(public base::NonThreadSafe) {
On 2014/08/06 21:27:26, jam wrote:
> remove the NonThreadSafe part, we don't put that on public interface. they're
> all assumed to be used on the UI thread unless otherwise mentioned. also, the
Done.
> embedder shouldn't double check that content is calling it on the right thread
To clarify, are you saying that in, chrome_ssl_host_state_delegate.h the
implementation should *not* call DCHECK(CalledOnValidThread())?

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:27: SSLHostStateDelegate() {}
On 2014/08/06 21:27:26, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:67:
DISALLOW_COPY_AND_ASSIGN(SSLHostStateDelegate);
On 2014/08/06 21:27:26, jam wrote:
> nit:not needed on an abstract interface..

Done.

[Peter Kasting, 2014-08-08 17:15:12]

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:390:
base::Bind(&WebsiteSettingsPopupView::HandleLinkClickedAsync,
On 2014/08/08 16:48:50, jww wrote:
> On 2014/08/07 03:56:57, Peter Kasting wrote:
> > So can we get rid of this async stuff as well?  Or were you going to do that
> as
> > a separate patch?
> 
> I would rather leave that to someone else since I don't really understand the
> ins and outs, unless you think it's as simple as "make it synchronous." Let me
> know what you think.

I would say, make it synchronous, try using it under ASAN, if it seems OK land;
if it doesn't seem OK, file a bug against me to find someone to look more
closely, and don't put any further effort in.

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the SSL
error for this host.
On 2014/08/08 16:48:50, jww wrote:
> Good point. Unfortunately, it's a little of both. We do check both because,
> theoretically, there is "Allow" and "Deny," but in practice, "Deny" is never
> called. I suppose there is a theoretic world in which we implement a content
> setting to "always deny" or something like that, but it doesn't exist.
> 
> I'm ambivalent about how to express this in the comment. I suppose it should
> probably say something additionally like, "Set when the user has explicitly
> bypassed an SSL error for this host or explicitly denied it (the latter of
which
> is not currently possible in the Chrome UI)." What do you think?

If denying isn't possible in the Chrome UI, I think the underlying APIs here
should reflect that.  So a function like "AllowOrDenyX" would be named "AllowX"
instead.  The confusion you're struggling with comes from the fact that the
existing APIs you're calling claim to expose a distinction that never
practically occurs, so the bug is really with them rather than your new code. 
Once the APIs only talk about "allow", the names and comments here should be
fairly obvious.

I hope making these changes is relatively straightforward.  If there are a lot
of then maybe this would have to happen as a separate patch.

[jww, 2014-08-11 19:21:28]

jam@ and pkasting@, can you take a look at the latest to see if it addresses
your comments? Thanks!

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:390:
base::Bind(&WebsiteSettingsPopupView::HandleLinkClickedAsync,
On 2014/08/08 17:15:12, Peter Kasting wrote:
> On 2014/08/08 16:48:50, jww wrote:
> > On 2014/08/07 03:56:57, Peter Kasting wrote:
> > > So can we get rid of this async stuff as well?  Or were you going to do
that
> > as
> > > a separate patch?
> > 
> > I would rather leave that to someone else since I don't really understand
the
> > ins and outs, unless you think it's as simple as "make it synchronous." Let
me
> > know what you think.
> 
> I would say, make it synchronous, try using it under ASAN, if it seems OK
land;
> if it doesn't seem OK, file a bug against me to find someone to look more
> closely, and don't put any further effort in.

Done.

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the SSL
error for this host.
On 2014/08/08 17:15:12, Peter Kasting wrote:
> On 2014/08/08 16:48:50, jww wrote:
> > Good point. Unfortunately, it's a little of both. We do check both because,
> > theoretically, there is "Allow" and "Deny," but in practice, "Deny" is never
> > called. I suppose there is a theoretic world in which we implement a content
> > setting to "always deny" or something like that, but it doesn't exist.
> > 
> > I'm ambivalent about how to express this in the comment. I suppose it should
> > probably say something additionally like, "Set when the user has explicitly
> > bypassed an SSL error for this host or explicitly denied it (the latter of
> which
> > is not currently possible in the Chrome UI)." What do you think?
> 
> If denying isn't possible in the Chrome UI, I think the underlying APIs here
> should reflect that.  So a function like "AllowOrDenyX" would be named
"AllowX"
> instead.  The confusion you're struggling with comes from the fact that the
> existing APIs you're calling claim to expose a distinction that never
> practically occurs, so the bug is really with them rather than your new code. 
> Once the APIs only talk about "allow", the names and comments here should be
> fairly obvious.
> 
> I hope making these changes is relatively straightforward.  If there are a lot
> of then maybe this would have to happen as a separate patch.

There might be a reason to blacklist specific certs someday, so I'd like to keep
that option open in the API (additionally, a different embeder other than Chrome
might want to do this), but I agree it's definitely confusing. Addtionally, the
underlying implementation would work if "Deny" was ever called, so if anyone
ever used "Deny," they would need to go and change a renamed "HasAllowedCert"
method to be more complex.

So instead of making "HasAllowedCert," I've renamed it to be slightly more
generic ("HasUserDecision"), which I hope makes it less confusing.This also has
the advantage that if someone does start using "Deny" to blacklist certs, the UI
will still do the correct thing. Let me know if that sounds okay, or if you
think we need to change the API further (I also changed the Revoke* methods to
be clearer).

https://codereview.chromium.org/418133012/diff/180001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.h (left):

https://codereview.chromium.org/418133012/diff/180001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/08/06 21:27:26, jam wrote:
> these 2 files should stay in content/browser/ssl, and should be
> ssl_host_state_impl.{h,cc}. the public directory should only have a header
with
> the interface

Removed ssl_host_state.{h,cc} completely, as per our offline conversation, so no
longer relevant.

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
File content/public/browser/ssl_host_state.h (right):

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:32: : NON_EXPORTED_BASE(public
base::NonThreadSafe) {
On 2014/08/06 21:27:26, jam wrote:
> nit: get rid of the NonThreadSafe, if you really want it, put it on the
> implementation and not the public interface.

Removed ssl_host_state.{h,cc} completely, as per our offline conversation, so no
longer relevant.

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:44: virtual bool
HasAllowedOrDeniedCert(const std::string& host);
On 2014/08/06 21:27:26, jam wrote:
> nit: document

Removed ssl_host_state.{h,cc} completely, as per our offline conversation, so no
longer relevant.

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:46: void
set_delegate(SSLHostStateDelegate* delegate) { delegate_ = delegate; }
On 2014/08/06 21:27:26, jam wrote:
> this would be a pure method called SetDelegate

Removed ssl_host_state.{h,cc} completely, as per our offline conversation, so no
longer relevant.

https://codereview.chromium.org/418133012/diff/180001/content/public/browser/...
content/public/browser/ssl_host_state.h:54: SSLHostStateDelegate* delegate_;
On 2014/08/06 21:27:26, jam wrote:
> nit: see http://www.chromium.org/developers/content-module/content-api, this
> should only be an interface. the data members can go on the implementation

Removed ssl_host_state.{h,cc} completely, as per our offline conversation, so no
longer relevant.

[Peter Kasting, 2014-08-11 20:24:20]

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the SSL
error for this host.
On 2014/08/11 19:21:28, jww wrote:
> There might be a reason to blacklist specific certs someday,

This is dangerous reasoning.  If this doesn't come to pass, we've basically made
the code more confusing than it needs to be for no benefit.  Even if it does
come to pass, we still pay that cost in the meantime.  My argument is always: if
you have strong reason to believe this _will_ be changed within a definite
timeframe (e.g. six months), then this is OK; otherwise please make the design
reflect only the capabilities you need right now.  We can always expand the API
later if we do decide we need to implement blacklisting.

To be clear, I don't think your current design is hideously awful and the end of
the world.  I just think it could be even better, and probably should be better,
as long as the reason for having it this way is purely hypothetical.

[jww, 2014-08-11 20:27:10]

On 2014/08/11 20:24:20, Peter Kasting wrote:
>
https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings_ui.h (right):
> 
>
https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the
SSL
> error for this host.
> On 2014/08/11 19:21:28, jww wrote:
> > There might be a reason to blacklist specific certs someday,
> 
> This is dangerous reasoning.  If this doesn't come to pass, we've basically
made
> the code more confusing than it needs to be for no benefit.  Even if it does
> come to pass, we still pay that cost in the meantime.  My argument is always:
if
> you have strong reason to believe this _will_ be changed within a definite
> timeframe (e.g. six months), then this is OK; otherwise please make the design
> reflect only the capabilities you need right now.  We can always expand the
API
> later if we do decide we need to implement blacklisting.
> 
> To be clear, I don't think your current design is hideously awful and the end
of
> the world.  I just think it could be even better, and probably should be
better,
> as long as the reason for having it this way is purely hypothetical.

Okay, I'll change it. Notably, this is actually changing the API back (there's
always been a "Deny"), which is why I thought there might have been a reason to
keep it.

[jww, 2014-08-11 21:12:55]

On 2014/08/11 20:27:10, jww wrote:
> On 2014/08/11 20:24:20, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
> > File chrome/browser/ui/website_settings/website_settings_ui.h (right):
> > 
> >
>
https://codereview.chromium.org/418133012/diff/180001/chrome/browser/ui/websi...
> > chrome/browser/ui/website_settings/website_settings_ui.h:101: // bypass the
> SSL
> > error for this host.
> > On 2014/08/11 19:21:28, jww wrote:
> > > There might be a reason to blacklist specific certs someday,
> > 
> > This is dangerous reasoning.  If this doesn't come to pass, we've basically
> made
> > the code more confusing than it needs to be for no benefit.  Even if it does
> > come to pass, we still pay that cost in the meantime.  My argument is
always:
> if
> > you have strong reason to believe this _will_ be changed within a definite
> > timeframe (e.g. six months), then this is OK; otherwise please make the
design
> > reflect only the capabilities you need right now.  We can always expand the
> API
> > later if we do decide we need to implement blacklisting.
> > 
> > To be clear, I don't think your current design is hideously awful and the
end
> of
> > the world.  I just think it could be even better, and probably should be
> better,
> > as long as the reason for having it this way is purely hypothetical.
> 
> Okay, I'll change it. Notably, this is actually changing the API back (there's
> always been a "Deny"), which is why I thought there might have been a reason
to
> keep it.

I was wrong. Deny is currently used, implicitly, when a user doesn't click
"proceed" through an interstitial. There's an argument that this isn't useful
right now, but I would much prefer not to dig into it into this CL.

Any thoughts on how I can improve the comments to reflect this? In particular,
that the UI doesn't allow a user to "deny" a certificate explicitly, but that it
may be done implicitly?

[Peter Kasting, 2014-08-11 22:35:41]

On 2014/08/11 21:12:55, jww wrote:
> I was wrong. Deny is currently used, implicitly, when a user doesn't click
> "proceed" through an interstitial. There's an argument that this isn't useful
> right now, but I would much prefer not to dig into it into this CL.
> 
> Any thoughts on how I can improve the comments to reflect this? In particular,
> that the UI doesn't allow a user to "deny" a certificate explicitly, but that
it
> may be done implicitly?

So, you're saying that if a user gets an interstitial, and then (say) closes the
tab, we'll write a "deny" value somewhere, so the next time, they won't get the
interstitial, things will just auto-fail?

If so, that sounds like we should basically say that denying is possible by
closing one of these interstitials, and return the names of the APIs and members
to something that doesn't imply "allow" but not "deny".

https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:404: :
NULL;
Nit: clang-format bug: Wrap like

    gfx::NativeWindow parent = GetAnchorView() ?
        GetAnchorView()->GetWidget()->GetNativeWindow() : NULL;

https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:539: if (delegate)
Is there ever not a delegate?  If so, consider noting in a comment when that can
be the case.

[jww, 2014-08-11 23:14:41]

On 2014/08/11 22:35:41, Peter Kasting wrote:
> On 2014/08/11 21:12:55, jww wrote:
> > I was wrong. Deny is currently used, implicitly, when a user doesn't click
> > "proceed" through an interstitial. There's an argument that this isn't
useful
> > right now, but I would much prefer not to dig into it into this CL.
> > 
> > Any thoughts on how I can improve the comments to reflect this? In
particular,
> > that the UI doesn't allow a user to "deny" a certificate explicitly, but
that
> it
> > may be done implicitly?
> 
> So, you're saying that if a user gets an interstitial, and then (say) closes
the
> tab, we'll write a "deny" value somewhere, so the next time, they won't get
the
> interstitial, things will just auto-fail?
Sorry, my statement wasn't clear. No, DENY is treated like UNKOWN in
SSLBlockingPage, which is to say an interstitial will be shown again. However,
the comments throughout there imply that this might change, so I think removing
the API to do this would require more discussion. I'm not saying it shouldn't 
happen, I just think it should be a separate CL since it would be a change to
the already existing API and the comments imply it might be used soon.
> 
> If so, that sounds like we should basically say that denying is possible by
> closing one of these interstitials, and return the names of the APIs and
members
> to something that doesn't imply "allow" but not "deny".
> 
>
https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/views...
> File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> (right):
> 
>
https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/views...
> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:404: :
> NULL;
> Nit: clang-format bug: Wrap like
> 
>     gfx::NativeWindow parent = GetAnchorView() ?
>         GetAnchorView()->GetWidget()->GetNativeWindow() : NULL;
Whoops, sorry. Fixed.
> 
>
https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings.cc (right):
> 
>
https://codereview.chromium.org/418133012/diff/240001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings.cc:539: if (delegate)
> Is there ever not a delegate?  If so, consider noting in a comment when that
can
> be the case.
Good point. In general, the embedder doesn't have to provide one, but in Chrome
land, there should always be a delegate. I'll put in a DCHECK instead.

[Peter Kasting, 2014-08-12 01:03:55]

On 2014/08/11 23:14:41, jww wrote:
> I think removing
> the API to do this would require more discussion. I'm not saying it shouldn't 
> happen, I just think it should be a separate CL since it would be a change to
> the already existing API and the comments imply it might be used soon.

If your intent is to actually raise the relevant discussion here so we can
clarify the API and comments, then I'm fine with landing whatever you think is
good now and doing that cleanup as the separate CL, because that second CL will
change all this again anyway.

We only need to be picky here if the plan is to land this and then not change
the other API.

So, assuming your plan is the former, LGTM in advance.

[jam, 2014-08-12 02:24:54]

https://codereview.chromium.org/418133012/diff/260001/content/public/browser/...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/418133012/diff/260001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:52: virtual void
RevokeUserDecisionsHard(const std::string& host) = 0;
these two methods appear to be called from chrome only right? if so they should
be on the implementation there and not on the interface. what's on the interface
should only be methods that are called by content. see
http://www.chromium.org/developers/content-module/content-api

[jww, 2014-08-12 04:41:20]

jam@, I've addressed your point.

pkasting@, I think we should address this separately, so I filed
https://crbug.com/402764 to track this and ping the relevant people.

https://codereview.chromium.org/418133012/diff/260001/content/public/browser/...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/418133012/diff/260001/content/public/browser/...
content/public/browser/ssl_host_state_delegate.h:52: virtual void
RevokeUserDecisionsHard(const std::string& host) = 0;
On 2014/08/12 02:24:54, jam wrote:
> these two methods appear to be called from chrome only right? if so they
should
> be on the implementation there and not on the interface. what's on the
interface
> should only be methods that are called by content. see
> http://www.chromium.org/developers/content-module/content-api

Done. HasUserDecision is the same, so I also moved it out of content.

[jam, 2014-08-13 06:42:59]

lgtm

[jww, 2014-08-13 13:52:16]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:54:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/418133012/300001

[commit-bot: I haz the power, 2014-08-13 15:49:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-13 16:10:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 16:10:53]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[jww, 2014-08-13 16:11:39]

The CQ bit was checked by jww@chromium.org

[jww, 2014-08-13 16:12:15]

The CQ bit was unchecked by jww@chromium.org

[jww, 2014-08-13 16:12:54]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-13 16:14:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/418133012/300001

[commit-bot: I haz the power, 2014-08-13 16:20:28]

Committed patchset #16 (300001) as 289311