Watch for filtering pref changes in ManagedModeInterstitial and unblock requests if they become…

Watch for filtering pref changes in ManagedModeInterstitial and unblock requests if they become allowed.

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

[Bernhard Bauer, 2013-09-03 15:54:59]

Please review.

[Pam (message me for reviews), 2013-09-04 09:56:59]

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_interstitial.cc (right):

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_interstitial.cc:42: // request without
showing the interstitial
missing .

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_interstitial.cc:152:
DCHECK(ShouldProceed());
Should we just crash? I could imagine some situations where showing a blocked
page would be worse than crashing.

[Bernhard Bauer, 2013-09-04 11:48:35]

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_interstitial.cc (right):

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_interstitial.cc:42: // request without
showing the interstitial
On 2013/09/04 09:56:59, Pam (also PM for reviews) wrote:
> missing .

Done.

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_interstitial.cc:152:
DCHECK(ShouldProceed());
On 2013/09/04 09:56:59, Pam (also PM for reviews) wrote:
> Should we just crash? I could imagine some situations where showing a blocked
> page would be worse than crashing.

Well, this is really one of these "this cannot happen" situations. I feel like
purposely crashing would be trying to handle it, which you're not supposed to
do.

[Pam (message me for reviews), 2013-09-05 10:13:52]

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_interstitial.cc (right):

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_interstitial.cc:152:
DCHECK(ShouldProceed());
On 2013/09/04 11:48:35, Bernhard Bauer wrote:
> On 2013/09/04 09:56:59, Pam (also PM for reviews) wrote:
> > Should we just crash? I could imagine some situations where showing a
blocked
> > page would be worse than crashing.
> 
> Well, this is really one of these "this cannot happen" situations. I feel like
> purposely crashing would be trying to handle it, which you're not supposed to
> do. 

Keeping going somehow even though something unexpected has happened is
"handling" it. Crashing would be "not handling". I'm proposing changing the
DCHECK to a CHECK, on the grounds that showing a blocked site is kinda-sorta a
"security" problem.

[Bernhard Bauer, 2013-09-05 12:49:10]

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_interstitial.cc (right):

https://codereview.chromium.org/23533014/diff/8001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_interstitial.cc:152:
DCHECK(ShouldProceed());
On 2013/09/05 10:13:52, Pam (also PM for reviews) wrote:
> On 2013/09/04 11:48:35, Bernhard Bauer wrote:
> > On 2013/09/04 09:56:59, Pam (also PM for reviews) wrote:
> > > Should we just crash? I could imagine some situations where showing a
> blocked
> > > page would be worse than crashing.
> > 
> > Well, this is really one of these "this cannot happen" situations. I feel
like
> > purposely crashing would be trying to handle it, which you're not supposed
to
> > do. 
> 
> Keeping going somehow even though something unexpected has happened is
> "handling" it. Crashing would be "not handling".

What I meant by handling was that a CHECK results in production code that checks
this condition.

> I'm proposing changing the
> DCHECK to a CHECK, on the grounds that showing a blocked site is kinda-sorta a
> "security" problem.

Fair enough. I changed it to a CHECK, and added a comment.

[Pam (message me for reviews), 2013-09-05 12:56:06]

lgtm

[commit-bot: I haz the power, 2013-09-05 13:17:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23533014/22001

[commit-bot: I haz the power, 2013-09-05 15:52:15]

Change committed as 221442