Relanding https://chromiumcodereview.appspot.com/10694037 after a revert. The original CL failed on…

Relanding https://chromiumcodereview.appspot.com/10694037 after a revert. The original CL failed on Mac Debug.

Add a policy to disable proceeding through the Safe Browsing interstitials.

BUG=119184
TEST=browser_tests:SafeBrowsingBlockingPageTest.*, Safe Browsing warning pages don't have a "proceed" link when opening malware sites (e.g. ianfette.org) and the policy is set


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

[Joao da Silva, 2012-07-08 16:21:20]

Please have a look at this again. Patch set 1 is the original CL
(https://chromiumcodereview.appspot.com/10694037), and patch set 2 includes the
fix for the failing test. Thanks!

[James Hawkins, 2012-07-08 17:19:30]

lgtm

[Mattias Nissler (ping if slow), 2012-07-09 08:32:25]

chrome/browser/policy still LGTM, but I'm unsure about the busy wait loop you
put in.

http://codereview.chromium.org/10690102/diff/4001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/10690102/diff/4001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:392: } while
(ready_state == "loading");
I guess this loop spins pretty fast and thus eats CPU. Is this a common thing to
do and/or do we have better alternatives? (I'm not an expert on this though,
maybe you want somebody who knows this stuff better to take a look).

[Miranda Callahan, 2012-07-09 13:20:37]

profiles/* still LGTM.

On 2012/07/09 08:32:25, Mattias Nissler wrote:
> chrome/browser/policy still LGTM, but I'm unsure about the busy wait loop you
> put in.
> 
>
http://codereview.chromium.org/10690102/diff/4001/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):
> 
>
http://codereview.chromium.org/10690102/diff/4001/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:392: } while
> (ready_state == "loading");
> I guess this loop spins pretty fast and thus eats CPU. Is this a common thing
to
> do and/or do we have better alternatives? (I'm not an expert on this though,
> maybe you want somebody who knows this stuff better to take a look).

[Joao da Silva, 2012-07-10 09:52:20]

@Mattias: please have a look at the current fix for the test. Still lgty?

http://codereview.chromium.org/10690102/diff/4001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/10690102/diff/4001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:392: } while
(ready_state == "loading");
On 2012/07/09 08:32:26, Mattias Nissler wrote:
> I guess this loop spins pretty fast and thus eats CPU. Is this a common thing
to
> do and/or do we have better alternatives? (I'm not an expert on this though,
> maybe you want somebody who knows this stuff better to take a look).

I've had a deeper look into this. document.readyState goes from 'loading' to
'interactive' or 'complete' quite fast, and this loops once or twice before
proceeding. I couldn't find a nicer way to make sure that the page has finished
loading.

The generated page consists of the malware_block.html file, with a couple of
<script> tags appended: the i18n-template code, jstemplate code, the
templateData as a global object, and a last <script> that invokes jstProcess on
the template_root element (using templateData). So the test fails because the
resource is loaded and the page is parsed, but these scripts haven't executed
yet.

A simpler fix is to execute jstProcess again (this is supported by jstemplate)
before inspecting, to make sure it has processed the page. That fixes the test,
but I still wonder if other tests may be racing against scripts that should
execute immediately. I'll ping chromium-dev to see if someone has any hints on
this.

[Mattias Nissler (ping if slow), 2012-07-10 11:28:45]

http://codereview.chromium.org/10690102/diff/11002/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/10690102/diff/11002/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:388:
"jstProcess(new JsEvalContext(templateData), root);\n"
If I were in your shoes, I'd just wait a day or two to see whether chromium-dev
comes up with a better idea. If not, I'm fine with this.

[Joao da Silva, 2012-07-12 14:27:21]

http://codereview.chromium.org/10690102/diff/11002/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/10690102/diff/11002/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:388:
"jstProcess(new JsEvalContext(templateData), root);\n"
On 2012/07/10 11:28:45, Mattias Nissler wrote:
> If I were in your shoes, I'd just wait a day or two to see whether
chromium-dev
> comes up with a better idea. If not, I'm fine with this.

Haven't found a better way to do this. John's suggestion to wait for 2
navigations with NavigateToURLBlockUntilNavigationsComplete() doesn't work (it
blocks waiting for the 2nd navigation; the interstitial doesn't count as a
navigation). Ok to land as-is for now?

[Mattias Nissler (ping if slow), 2012-07-12 14:29:34]

On 2012/07/12 14:27:21, Joao da Silva wrote:
>
http://codereview.chromium.org/10690102/diff/11002/chrome/browser/safe_browsi...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):
> 
>
http://codereview.chromium.org/10690102/diff/11002/chrome/browser/safe_browsi...
> chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:388:
> "jstProcess(new JsEvalContext(templateData), root);\n"
> On 2012/07/10 11:28:45, Mattias Nissler wrote:
> > If I were in your shoes, I'd just wait a day or two to see whether
> chromium-dev
> > comes up with a better idea. If not, I'm fine with this.
> 
> Haven't found a better way to do this. John's suggestion to wait for 2
> navigations with NavigateToURLBlockUntilNavigationsComplete() doesn't work (it
> blocks waiting for the 2nd navigation; the interstitial doesn't count as a
> navigation). Ok to land as-is for now?

Yeah, put a TODO and go ahead :)

[commit-bot: I haz the power, 2012-07-12 14:40:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10690102/4002

[commit-bot: I haz the power, 2012-07-12 16:04:08]

Change committed as 146359