Cleanup: Rename is_hsts_host to fatal.

Rename is_hsts_host to fatal.

Per wtc: the old name was too specific, and became confusing because we now
also have certificate errors that users cannot override, but for reasons
other than (although still closely related to) HSTS.

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

[palmer, 2012-01-11 23:48:36]

Reviewers are all OWNERs of the relevant subtrees. Thanks!

[tony, 2012-01-11 23:57:40]

src/webkit/ LGTM if wtc agrees.

[michaeln, 2012-01-12 20:39:33]

http://codereview.chromium.org/9188019/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9188019/diff/1/net/url_request/url_request.h#n...
net/url_request/url_request.h:268: // must not be bypassable by the user.
Is the polarity of the new argument reversed?

if (overridable)
   "must not be bypassable by the user"

[palmer, 2012-01-12 23:53:25]

http://codereview.chromium.org/9188019/diff/1/net/url_request/url_request.h#n...
> net/url_request/url_request.h:268: // must not be bypassable by the user.
> Is the polarity of the new argument reversed?
> 
> if (overridable)
>    "must not be bypassable by the user"

Sigh. :) The code would have behaved the same, but the word would be exactly
backwards. The real irony is that I fixed a regression introduced by someone
else in exactly the same way a few weeks back.

So, now it's is_hsts_host -> fatal instead of is_hsts_host -> overridable.
"true" means "true" again, and we can all breathe easier...

[wtc, 2012-01-13 01:39:57]

Patch Set 2 LGTM.

Please note my comments on url_request_test_util.h.

http://codereview.chromium.org/9188019/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9188019/diff/1/net/url_request/url_request.h#n...
net/url_request/url_request.h:268: // must not be bypassable by the user.

Yes, 'fatal' is more appropriate than 'overridable'.

http://codereview.chromium.org/9188019/diff/8001/content/browser/ssl/ssl_cert...
File content/browser/ssl/ssl_cert_error_handler.h (right):

http://codereview.chromium.org/9188019/diff/8001/content/browser/ssl/ssl_cert...
content/browser/ssl/ssl_cert_error_handler.h:44: const bool fatal_;  // true if
the error is from an HSTS host.

Update this comment.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request....
net/url_request/url_request.h:709: bool fatal);

Nit: this should fit on one line now.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
net/url_request/url_request_job.h:207: bool fatal);

Nit: this should fit on one line now.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
File net/url_request/url_request_test_util.h (right):

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
net/url_request/url_request_test_util.h:123: bool fatal() const { return fatal_;
}

This getter method and the data member should be named
certificate_errors_are_fatal to clarify what is fatal.
(This class, TestDelegate, is not specific to certificate
errors.)

Note the existing members allow_certificate_errors_ and
have_certificate_errors_.  I think it is confusing to have
both allow_certificate_errors_ and certificate_errors_are_fatal.
Please see if we can somehow combine the two, or document
their relation.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
net/url_request/url_request_test_util.h:133: bool fatal) OVERRIDE;

In contrast, this input parameter to a function handling
SSL certificate error can be named simply 'fatal' because
the context makes it clear what is fatal.

[palmer, 2012-01-13 23:03:47]

http://codereview.chromium.org/9188019/diff/8001/content/browser/ssl/ssl_cert...
File content/browser/ssl/ssl_cert_error_handler.h (right):

http://codereview.chromium.org/9188019/diff/8001/content/browser/ssl/ssl_cert...
content/browser/ssl/ssl_cert_error_handler.h:44: const bool fatal_;  // true if
the error is from an HSTS host.
On 2012/01/13 01:39:57, wtc wrote:
> 
> Update this comment.

Done.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request....
net/url_request/url_request.h:709: bool fatal);
On 2012/01/13 01:39:57, wtc wrote:
> 
> Nit: this should fit on one line now.

Done.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
net/url_request/url_request_job.h:207: bool fatal);
On 2012/01/13 01:39:57, wtc wrote:
> 
> Nit: this should fit on one line now.

Done.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
File net/url_request/url_request_test_util.h (right):

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
net/url_request/url_request_test_util.h:123: bool fatal() const { return fatal_;
}
On 2012/01/13 01:39:57, wtc wrote:
> 
> This getter method and the data member should be named
> certificate_errors_are_fatal to clarify what is fatal.
> (This class, TestDelegate, is not specific to certificate
> errors.)
> 
> Note the existing members allow_certificate_errors_ and
> have_certificate_errors_.  I think it is confusing to have
> both allow_certificate_errors_ and certificate_errors_are_fatal.
> Please see if we can somehow combine the two, or document
> their relation.

Done.

http://codereview.chromium.org/9188019/diff/8001/net/url_request/url_request_...
net/url_request/url_request_test_util.h:133: bool fatal) OVERRIDE;
On 2012/01/13 01:39:57, wtc wrote:
> 
> In contrast, this input parameter to a function handling
> SSL certificate error can be named simply 'fatal' because
> the context makes it clear what is fatal.

Done.

[commit-bot: I haz the power, 2012-01-17 21:33:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/9188019/10002

[commit-bot: I haz the power, 2012-01-17 21:34:02]

Presubmit check for 9188019-10002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/ssl/ssl_manager.cc,content/browser/ssl/ssl_manager.h,content/browser/renderer_host/resource_dispatcher_host.h,content/browser/ssl/ssl_cert_error_handler.cc,content/browser/ssl/ssl_cert_error_handler.h,content/browser/ssl/ssl_policy.cc

Presubmit checks took 1.8s to calculate.

[abarth-chromium, 2012-01-17 21:39:49]

LGTM

I would have called it |is_fatal|, but whatever.

[commit-bot: I haz the power, 2012-01-17 21:43:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/9188019/10002

[commit-bot: I haz the power, 2012-01-17 21:43:46]

Presubmit check for 9188019-10002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/resource_dispatcher_host.h

Presubmit checks took 1.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!