Extend the TestRunner so that the manifest-src CSP directive can be tested from layout tests.

Extend the TestRunner so that the manifest-src CSP directive can be tested from layout tests.

BUG=409996
Committed: https://crrev.com/1a4946d176030030e7938807f047b28ebaf8213f
Cr-Commit-Position: refs/heads/master@{#296716}

[kenneth.r.christiansen, 2014-09-18 13:48:44]

kenneth.r.christiansen@intel.com changed reviewers:
+ mkwst@chromium.org, moumouri@chromium.org

[kenneth.r.christiansen, 2014-09-18 13:50:27]

I am not allowed to use content here so I had to skip the upload hooks. I am not
really allowed to use core/ either so I cannot even emulate the fetch as there
is no public API.

How should we proceed?

This actually makes this following test work:

 1 <meta http-equiv="Content-Security-Policy" content="manifest-src 'self'">
 2 <link rel="manifest" href="http://www.evil.com/manifest.json">
 3 <script>
 4     if (window.testRunner) {
 5         testRunner.dumpAsText();
 6         testRunner.waitUntilDone();
 7         testRunner.getManifestThen(function() {
 8             alert("Pass");
 9             testRunner.notifyDone();
10         });
11     }
12 </script>

With the result:

Blocked access to external URL http://www.evil.com/manifest.json
ALERT: Pass

[kenneth.christiansen, 2014-09-18 16:05:44]

New patch up which passes the upload hooks

[mlamouri (slow - plz ping), 2014-09-18 17:36:04]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org
- moumouri@chromium.org

[mlamouri (slow - plz ping), 2014-09-18 17:36:05]

(nit: moumouri@chromium.org is not me :))

https://codereview.chromium.org/584553002/diff/20001/content/public/test/layo...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/584553002/diff/20001/content/public/test/layo...
content/public/test/layouttest_support.h:53: const std::string& data)>
callback);
nit: I think it would be better to have a type for that callback.
FetchManifestCallback?

Actually, it would have been clearer that you are not using |const Callback&|
which you should.

https://codereview.chromium.org/584553002/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/584553002/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/test_runner.cc:2823: base::Unretained(this),
Why not passing a weakptr? TestRunner has a weak_factory_ so you could easily
use it.

https://codereview.chromium.org/584553002/diff/20001/content/test/layouttest_...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/584553002/diff/20001/content/test/layouttest_...
content/test/layouttest_support.cc:87: void
FetchManifestDoneCallback(ManifestFetcher* fetcher,
nit: I think you should align parameters.

https://codereview.chromium.org/584553002/diff/20001/content/test/layouttest_...
content/test/layouttest_support.cc:88: ManifestFetcher::Callback callback,
const ManifestFetcher::Callback& callback

https://codereview.chromium.org/584553002/diff/20001/content/test/layouttest_...
content/test/layouttest_support.cc:96: ManifestFetcher::Callback callback) {
It's a bit odd to have a non typed callback in the header and a typed callback
here. I would add a type specific to this file.

https://codereview.chromium.org/584553002/diff/20001/content/test/layouttest_...
content/test/layouttest_support.cc:101: base::Unretained(fetcher),
Alternatively, you could make |fetcher| be a scoped_ptr and Pass it to the
callback so it would be clearer that you make the callback owner of the object.
You wouldn't even have to explicitely |delete| it anymore.

[kenneth.christiansen, 2014-09-24 13:12:04]

On 2014/09/18 at 17:36:05, mlamouri wrote:
> (nit: moumouri@chromium.org is not me :))

For some reason the autocomplete suggest you (Mounir Lamouri (malamouri@.. or
similar) but then it actually adds moumouri@. 


> nit: I think it would be better to have a type for that callback.
FetchManifestCallback?
> 
> Actually, it would have been clearer that you are not using |const Callback&|
which you should.

I agree, will fix.

>
https://codereview.chromium.org/584553002/diff/20001/content/shell/renderer/t...
> content/shell/renderer/test_runner/test_runner.cc:2823:
base::Unretained(this),
> Why not passing a weakptr? TestRunner has a weak_factory_ so you could easily
use it.

Existing code (which this is based upon) does it like this. It would be better
to be consistent now and then change them all in one follow up patch.

>
https://codereview.chromium.org/584553002/diff/20001/content/test/layouttest_...
> content/test/layouttest_support.cc:101: base::Unretained(fetcher),
> Alternatively, you could make |fetcher| be a scoped_ptr and Pass it to the
callback so it would be clearer that you make the callback owner of the object.
You wouldn't even have to explicitely |delete| it anymore.

Right

[Mike West, 2014-09-24 15:15:47]

LGTM with a few nits. You'll need a //content/shell OWNER to take a look to
verify that I'm not nuts.

https://codereview.chromium.org/584553002/diff/40001/content/public/test/layo...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/584553002/diff/40001/content/public/test/layo...
content/public/test/layouttest_support.h:14: #include "url/gurl.h"
Can you forward-declare this?

https://codereview.chromium.org/584553002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/584553002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/test_runner.cc:2819: GURL
url(web_view_->mainFrame()->document().manifestURL());
Nit: Do you need the temp variable? I'd just put it into the FetchManifest call.

https://codereview.chromium.org/584553002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/584553002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/test_runner.h:554: void
GetManifestThen(v8::Handle<v8::Function> callback);
Nit: I'd suggest moving this down under the pixel stuff, as that's conceptually
related to DisplayAsync. Or above DisplayAsync. Whatever you like.

https://codereview.chromium.org/584553002/diff/40001/content/test/layouttest_...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/584553002/diff/40001/content/test/layouttest_...
content/test/layouttest_support.cc:95: void FetchManifest(blink::WebView* view,
GURL url,
Nit: Can this be `const GURL&`? It looks like that's what ManifestFetcher
accepts.

[mlamouri (slow - plz ping), 2014-09-24 15:52:10]

lgtm too

https://codereview.chromium.org/584553002/diff/40001/content/public/test/layo...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/584553002/diff/40001/content/public/test/layo...
content/public/test/layouttest_support.h:53: void FetchManifest(blink::WebView*
view, GURL url,
You probably want to pass a |const GURL&| here.

https://codereview.chromium.org/584553002/diff/40001/content/test/layouttest_...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/584553002/diff/40001/content/test/layouttest_...
content/test/layouttest_support.cc:92: callback.Run(response, data);
nit: add a comment saying that |fetcher| will be deleted, maybe?

[jochen (gone - plz use gerrit), 2014-09-24 20:17:46]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2014-09-24 20:17:47]

lgtm with nits addressed

using weak ptrs instead of unretained sounds like a great follow-up patch :)

[kenneth.r.christiansen, 2014-09-25 13:03:26]

The CQ bit was checked by kenneth.r.christiansen@intel.com

[commit-bot: I haz the power, 2014-09-25 13:04:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584553002/60001

[commit-bot: I haz the power, 2014-09-25 13:21:41]

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

[commit-bot: I haz the power, 2014-09-25 13:21:42]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kenneth.r.christiansen, 2014-09-25 15:01:03]

The CQ bit was checked by kenneth.r.christiansen@intel.com

[commit-bot: I haz the power, 2014-09-25 15:01:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584553002/100001

[commit-bot: I haz the power, 2014-09-25 16:11:15]

Committed patchset #6 (id:100001) as eed8af21a941885ec98d60618eb32b7b4b18d5ae

[commit-bot: I haz the power, 2014-09-25 16:11:42]

Patchset 6 (id:??) landed as
https://crrev.com/1a4946d176030030e7938807f047b28ebaf8213f
Cr-Commit-Position: refs/heads/master@{#296716}