Disable TLS channel id if cookies are disabled or third-party cookies are blocked.

Disable TLS channel id if cookies are disabled or third-party cookies are blocked.

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

[battre, 2012-09-11 08:55:17]

LGTM, thanks a lot!

[wtc, 2012-09-11 22:21:03]

Patch set 2 LGTM.

I reviewed everything, but
ssl_config_service_manager_pref_unittest.cc I only skimmed
through.

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:175: void
OnDefaultContentSettingsChange(PrefService* prefs);

I guess this argument is named "prefs" because it can be either
local state or user prefs?

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:178: PrefChangeRegistrar
user_pref_change_registrar_;

Nit: user_pref => user_prefs ?

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:224: this);

Nit: align |this| with the first argument on the previous
line?  I know this is the old style, but it is followed by
this file.

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:48: const
scoped_refptr<SSLConfigService>& config_service) {

It seems that this parameter can be declared more simply as
  SSLConfigService* config_service

Are you trying to make it clear that the caller is not passing
a reference to this function?

[mattm, 2012-09-12 03:37:43]

+sky for chrome/ OWNERS

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:175: void
OnDefaultContentSettingsChange(PrefService* prefs);
On 2012/09/11 22:21:03, wtc wrote:
> 
> I guess this argument is named "prefs" because it can be either
> local state or user prefs?

Ah, I guess these could be named more specifically too.  The 
Observe function will get called with either kind of pref service, but it should
always be the one kind for each of the prefs.

done

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:178: PrefChangeRegistrar
user_pref_change_registrar_;
On 2012/09/11 22:21:03, wtc wrote:
> 
> Nit: user_pref => user_prefs ?

Done.

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:224: this);
On 2012/09/11 22:21:03, wtc wrote:
> 
> Nit: align |this| with the first argument on the previous
> line?  I know this is the old style, but it is followed by
> this file.

Done.

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

http://codereview.chromium.org/10917182/diff/4004/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:48: const
scoped_refptr<SSLConfigService>& config_service) {
On 2012/09/11 22:21:03, wtc wrote:
> 
> It seems that this parameter can be declared more simply as
>   SSLConfigService* config_service

done

> Are you trying to make it clear that the caller is not passing
> a reference to this function?

[sky, 2012-09-12 04:35:53]

LGTM

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager.h (right):

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager.h:22: PrefService* local_state,
PrefService* user_prefs);
nit: when you wrap, each param on its own line.

[battre, 2012-09-12 12:11:31]

one more nit :-)

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:240:
default_config.rev_checking_enabled);
nit: can you fix the indentation? also below

[wtc, 2012-09-12 18:53:11]

Patch set 4 LGTM.

[mattm, 2012-09-12 19:07:17]

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager.h (right):

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager.h:22: PrefService* local_state,
PrefService* user_prefs);
On 2012/09/12 04:35:53, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/10917182/diff/7003/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:240:
default_config.rev_checking_enabled);
On 2012/09/12 12:11:31, battre wrote:
> nit: can you fix the indentation? also below

done

[commit-bot: I haz the power, 2012-09-12 19:07:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/10917182/6005

[commit-bot: I haz the power, 2012-09-12 21:51:43]

Change committed as 156387