Remove unnecessary |result| argument from AllowCertificateError()

Remove unnecessary |result| argument from AllowCertificateError()

ContentBrowserClient::AllowCertificateError() previously took a |result|
argument, which looks like it was basically a shortcut to save embedders
from having to call the callback. When an embedder set the |result|
argument, SSLPolicy would call the appropriate SSLCertErrorHandler
method to continue or cancel the request. If the embedder didn't set the
|result| argument, then the embedder was responsible for calling the
callback, which would in turn call the appropriate SSLCertErrorHandler
method.

This CL removes the |result| argument and changes the |callback|
argument from a bool to an enum so that the embedder has a little bit
more control over how the callback cancels the request.

In addition to being a general clean-up/simplification, this change
paves the way for making SSLErrorHandler not-refcounted. A follow-up CL
will make SSLErrorHandler live on the UI thread only, and
AllowCertificateError()'s |callback| will take ownership of the
SSLErrorHandler. (Right now, the SSLErrorHandler is shared between
SSLPolicy and the callback. Removing the |result| argument means that
SSLPolicy no longer needs to retain a reference to the SSLErrorHandler.)

BUG=558491
Committed: https://crrev.com/719dde5b353db3cf9af7e030f79eb1bc4fb9b84b
Cr-Commit-Position: refs/heads/master@{#410560}

[estark, 2016-08-05 23:26:10]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 23:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 23:52:01]

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

[commit-bot: I haz the power, 2016-08-05 23:52:01]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[estark, 2016-08-06 00:18:46]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 00:19:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-06 01:49:22]

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

[commit-bot: I haz the power, 2016-08-06 01:49:23]

Dry run: This issue passed the CQ dry run.

[estark, 2016-08-08 17:02:28]

estark@chromium.org changed reviewers:
+ nasko@chromium.org

[estark, 2016-08-08 17:02:29]

Hi Nasko, this the first of two CLs that clean up content::SSLErrorHandler and
make it non-refcounted. Could you please take a look?

This one is largely mechanical and makes the ownership/lifetime of
SSLErrorHandler a little simpler. The follow-up CL
https://codereview.chromium.org/2213193005/ combines SSLCertErrorHandler with
SSLErrorHandler into one class that lives on the UI thread only and is not
refcounted.

For this CL, I'd suggest looking first at content_browser_client.h (that's the
actual API change) and ssl_policy.cc. Then chrome_content_browser_client.cc and
aw_content_browser_client.cc, which were the users of the |result| argument that
I'm removing. The rest of the changes in this CL are mostly mechanical.

[estark, 2016-08-08 17:42:22]

On 2016/08/08 17:02:29, estark wrote:
> Hi Nasko, this the first of two CLs that clean up content::SSLErrorHandler and
> make it non-refcounted. Could you please take a look?
> 
> This one is largely mechanical and makes the ownership/lifetime of
> SSLErrorHandler a little simpler. The follow-up CL
> https://codereview.chromium.org/2213193005/ combines SSLCertErrorHandler with
> SSLErrorHandler into one class that lives on the UI thread only and is not
> refcounted.

Actually, I just split up that CL into two smaller CLs:
https://codereview.chromium.org/2222003002/ that combines SSLCertErrorHandler
with SSLErrorHandler, and https://codereview.chromium.org/2213193005/ that makes
SSLErrorHandler UI-thread-only.

> 
> For this CL, I'd suggest looking first at content_browser_client.h (that's the
> actual API change) and ssl_policy.cc. Then chrome_content_browser_client.cc
and
> aw_content_browser_client.cc, which were the users of the |result| argument
that
> I'm removing. The rest of the changes in this CL are mostly mechanical.

[nasko, 2016-08-08 18:32:04]

Thanks for working on this! LGTM

https://codereview.chromium.org/2220603003/diff/20001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2220603003/diff/20001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:366: if (cancel_request) {
nit: No need for {} on one-line if statements.

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2048: if (!callback.is_null()) {
nit: No need for {} in one-line if statements.

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:159:
base::Callback<void(content::CertificateRequestResultType)>());
Orthogonal for this CL and therefore just for my own education. Do we not pass a
meaningful callback here because we don't allow proceeding through the
interstitial?

[estark, 2016-08-08 20:00:12]

estark@chromium.org changed reviewers:
+ alokp@chromium.org, boliu@chromium.org, dbeam@chromium.org

[estark, 2016-08-08 20:00:13]

Thanks Nasko!

alokp, can you please review chromecast/?
dbeam, can you please review chrome/browser/ui/webui?
boliu, can you please review android_webview/?

https://codereview.chromium.org/2220603003/diff/20001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2220603003/diff/20001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:366: if (cancel_request) {
On 2016/08/08 18:32:03, nasko wrote:
> nit: No need for {} on one-line if statements.

Done.

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2048: if (!callback.is_null()) {
On 2016/08/08 18:32:03, nasko wrote:
> nit: No need for {} in one-line if statements.

Done.

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/2220603003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:159:
base::Callback<void(content::CertificateRequestResultType)>());
On 2016/08/08 18:32:03, nasko wrote:
> Orthogonal for this CL and therefore just for my own education. Do we not pass
a
> meaningful callback here because we don't allow proceeding through the
> interstitial?

Yep, that's right. Clicking "Back to Safety" or "Proceed" on
chrome://interstitials is a no-op.

[boliu, 2016-08-08 20:15:49]

lgtm % include

suggest making base::Callback<void(CertificateRequestResultType)> an alias in
content, so all the clients don't have to type that thing over and over again

https://codereview.chromium.org/2220603003/diff/40001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/2220603003/diff/40001/android_webview/browser...
android_webview/browser/aw_content_browser_client.h:15: #include
"content/public/browser/certificate_request_result_type.h"
this include should be in content/public/browser/content_browser_client.h ?

[alokp, 2016-08-08 21:33:00]

chromecast/ lgtm

[Dan Beam, 2016-08-09 01:37:58]

rs lgtm

[estark, 2016-08-09 02:16:49]

Thanks all.

boliu I like your suggestion to alias the callback and filed a bug for it
(https://crbug.com/635718) -- I'd like to do it as a follow-up because I have a
number of CLs in flight that depend on this one or touch similar code, so I'm
eager to unblock them.

https://codereview.chromium.org/2220603003/diff/40001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/2220603003/diff/40001/android_webview/browser...
android_webview/browser/aw_content_browser_client.h:15: #include
"content/public/browser/certificate_request_result_type.h"
On 2016/08/08 20:15:48, boliu wrote:
> this include should be in content/public/browser/content_browser_client.h ?

Done.

[estark, 2016-08-09 02:17:31]

The CQ bit was checked by estark@chromium.org

[estark, 2016-08-09 02:17:31]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
alokp@chromium.org, boliu@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2220603003/#ps60001
(title: "remove unnecessary include")

[commit-bot: I haz the power, 2016-08-09 02:17:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 03:17:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-09 03:20:24]

Description was changed from

==========
Remove unnecessary |result| argument from AllowCertificateError()

ContentBrowserClient::AllowCertificateError() previously took a |result|
argument, which looks like it was basically a shortcut to save embedders
from having to call the callback. When an embedder set the |result|
argument, SSLPolicy would call the appropriate SSLCertErrorHandler
method to continue or cancel the request. If the embedder didn't set the
|result| argument, then the embedder was responsible for calling the
callback, which would in turn call the appropriate SSLCertErrorHandler
method.

This CL removes the |result| argument and changes the |callback|
argument from a bool to an enum so that the embedder has a little bit
more control over how the callback cancels the request.

In addition to being a general clean-up/simplification, this change
paves the way for making SSLErrorHandler not-refcounted. A follow-up CL
will make SSLErrorHandler live on the UI thread only, and
AllowCertificateError()'s |callback| will take ownership of the
SSLErrorHandler. (Right now, the SSLErrorHandler is shared between
SSLPolicy and the callback. Removing the |result| argument means that
SSLPolicy no longer needs to retain a reference to the SSLErrorHandler.)

BUG=558491
==========

to

==========
Remove unnecessary |result| argument from AllowCertificateError()

ContentBrowserClient::AllowCertificateError() previously took a |result|
argument, which looks like it was basically a shortcut to save embedders
from having to call the callback. When an embedder set the |result|
argument, SSLPolicy would call the appropriate SSLCertErrorHandler
method to continue or cancel the request. If the embedder didn't set the
|result| argument, then the embedder was responsible for calling the
callback, which would in turn call the appropriate SSLCertErrorHandler
method.

This CL removes the |result| argument and changes the |callback|
argument from a bool to an enum so that the embedder has a little bit
more control over how the callback cancels the request.

In addition to being a general clean-up/simplification, this change
paves the way for making SSLErrorHandler not-refcounted. A follow-up CL
will make SSLErrorHandler live on the UI thread only, and
AllowCertificateError()'s |callback| will take ownership of the
SSLErrorHandler. (Right now, the SSLErrorHandler is shared between
SSLPolicy and the callback. Removing the |result| argument means that
SSLPolicy no longer needs to retain a reference to the SSLErrorHandler.)

BUG=558491

Committed: https://crrev.com/719dde5b353db3cf9af7e030f79eb1bc4fb9b84b
Cr-Commit-Position: refs/heads/master@{#410560}
==========

[commit-bot: I haz the power, 2016-08-09 03:20:26]

Patchset 4 (id:??) landed as
https://crrev.com/719dde5b353db3cf9af7e030f79eb1bc4fb9b84b
Cr-Commit-Position: refs/heads/master@{#410560}