Report errors when parsing Manifest and expose them in developer console.

Report errors when parsing Manifest and expose them in developer console.

The ManifestParser reports a list of errors (as strings) that are then
propagated to the WebFrame's console by the ManifestManager.

BUG=437210
Committed: https://crrev.com/5bb64e626a33277374f4c23449c212cd3482e550
Cr-Commit-Position: refs/heads/master@{#305998}

[mlamouri (slow - plz ping), 2014-11-27 12:03:48]

mlamouri@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2014-11-27 13:24:32]

Whoo! I like this :-). Thanks for all the tests.

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:26:
logging::SetLogMessageHandler(nullptr);
indentation.

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:53: file && file ==
std::string("CONSOLE")) {
Mm. This feels rather fragile. Have you considered creating a 
WebContentsDelegate which overrides AddMessageToConsole? Several other tests
seem to be doing this.

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:239: GURL test_url =
GetTestUrl("manifest", "parsing-errors.html");
This file seems to be missing?

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:80: std::string GetErrorPrefix() {
This can return a "const std::string&" as |error_prefix| will have a permanent
address.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:86: } // anonymous namespace
nit: two spaces before the comment.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:122: CHECK(dictionary);
s/CHECK/DCHECK/. (We only use CHECK when something has security implications --
if that's the case, add a comment explaining why.)

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:193: "be same origin as
Document.");
I don't think "Document" makes a lot of sense when written with a capital.
Perhaps ".. as the document."?

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:216: "unknown 'display' value
ignored.");
nit: This can fit on a single line.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:309: if (!icon.src.is_valid()) {
nit: You can drop the brackets.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:26: ManifestParser(const
base::StringPiece&,
This argument should have a name.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:35: Manifest manifest() const;
This can return a "const Manifest&", leaving the copy to be done by the caller
if necessary.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:40: enum TrimType {
nit: I realize you're just moving this, but add a small one-line comment to
indicate that this is about trimming strings. E.g.:

// Used to indicate whether to strip whitespace from the value string.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:18: Manifest
ParseManifest(const base::StringPiece& json,
micro nit: you call this |data| elsewhere.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:25: }
Whilst annoying (as indicated by your stumbling across chairs here), could you
add a separate method for the arguments, rather than providing defaults?

Manifest ParseManifestWithUrls(...);

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:27: const
std::vector<std::string>& GetParseErrors() const {
nit: this should be named errors().

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:488: EXPECT_EQ(0u,
GetParseErrors().size());
nit: since you have the "GetParseErrors().size()" call 79 times, it'd make sense
to just have a "GetErrorCount()" method.

[mlamouri (slow - plz ping), 2014-11-27 13:49:09]

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:26:
logging::SetLogMessageHandler(nullptr);
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> indentation.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:53: file && file ==
std::string("CONSOLE")) {
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> Mm. This feels rather fragile. Have you considered creating a 
> WebContentsDelegate which overrides AddMessageToConsole? Several other tests
> seem to be doing this.

Do you have examples?

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:239: GURL test_url =
GetTestUrl("manifest", "parsing-errors.html");
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> This file seems to be missing?

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:80: std::string GetErrorPrefix() {
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> This can return a "const std::string&" as |error_prefix| will have a permanent
> address.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:86: } // anonymous namespace
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> nit: two spaces before the comment.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:122: CHECK(dictionary);
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> s/CHECK/DCHECK/. (We only use CHECK when something has security implications
--
> if that's the case, add a comment explaining why.)

Removed. I guess null dereference isn't really a security issue, though I prefer
to crash in a known fashion instead of randomly.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:193: "be same origin as
Document.");
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> I don't think "Document" makes a lot of sense when written with a capital.
> Perhaps ".. as the document."?

Sure...

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:216: "unknown 'display' value
ignored.");
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> nit: This can fit on a single line.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:309: if (!icon.src.is_valid()) {
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> nit: You can drop the brackets.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:26: ManifestParser(const
base::StringPiece&,
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> This argument should have a name.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:35: Manifest manifest() const;
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> This can return a "const Manifest&", leaving the copy to be done by the caller
> if necessary.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:40: enum TrimType {
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> nit: I realize you're just moving this, but add a small one-line comment to
> indicate that this is about trimming strings. E.g.:
> 
> // Used to indicate whether to strip whitespace from the value string.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:18: Manifest
ParseManifest(const base::StringPiece& json,
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> micro nit: you call this |data| elsewhere.

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:25: }
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> Whilst annoying (as indicated by your stumbling across chairs here), could you
> add a separate method for the arguments, rather than providing defaults?
> 
> Manifest ParseManifestWithUrls(...);

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:27: const
std::vector<std::string>& GetParseErrors() const {
On 2014/11/27 13:24:31, Peter Beverloo wrote:
> nit: this should be named errors().

Done.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:488: EXPECT_EQ(0u,
GetParseErrors().size());
On 2014/11/27 13:24:32, Peter Beverloo wrote:
> nit: since you have the "GetParseErrors().size()" call 79 times, it'd make
sense
> to just have a "GetErrorCount()" method.

Done.

[Peter Beverloo, 2014-11-27 13:55:15]

lgtm

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:53: file && file ==
std::string("CONSOLE")) {
On 2014/11/27 13:49:08, Mounir Lamouri wrote:
> On 2014/11/27 13:24:31, Peter Beverloo wrote:
> > Mm. This feels rather fragile. Have you considered creating a 
> > WebContentsDelegate which overrides AddMessageToConsole? Several other tests
> > seem to be doing this.
> 
> Do you have examples?

Not of this specific scenario. The tests using a similar technique to the one
you're using here haven't been disabled or marked as flaky, so I'm happy to try
this for now.

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/748373003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:122: CHECK(dictionary);
On 2014/11/27 13:49:09, Mounir Lamouri wrote:
> On 2014/11/27 13:24:31, Peter Beverloo wrote:
> > s/CHECK/DCHECK/. (We only use CHECK when something has security implications
> --
> > if that's the case, add a comment explaining why.)
> 
> Removed. I guess null dereference isn't really a security issue, though I
prefer
> to crash in a known fashion instead of randomly.

Yeah, which is why a DCHECK may still be useful?

https://codereview.chromium.org/748373003/diff/40001/content/test/data/manife...
File content/test/data/manifest/parsing-errors.html (right):

https://codereview.chromium.org/748373003/diff/40001/content/test/data/manife...
content/test/data/manifest/parsing-errors.html:4: <link rel=manifest
href='parsing-errors-manifest.json'>
Going for the icky HTML I see :-).

[Peter Beverloo, 2014-11-27 14:37:38]

I like this, thanks for trying it out!!

lgtm

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:21: class
MockWebContentsDelegate : public content::WebContentsDelegate {
nit: no need for content::

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:46: void
OverrideWebContentsDelegate() {
I wonder if we could do this in SetUp(), avoiding having to call it everywhere?

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:97: if (source !=
web_contents_)
When does this happen? Can it be a DCHECK?

https://codereview.chromium.org/748373003/diff/60001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/748373003/diff/60001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:122: CHECK(dictionary);
*D*CHECK :-)

[mlamouri (slow - plz ping), 2014-11-27 14:44:10]

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
File content/browser/manifest/manifest_browsertest.cc (right):

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:21: class
MockWebContentsDelegate : public content::WebContentsDelegate {
On 2014/11/27 14:37:37, Peter Beverloo wrote:
> nit: no need for content::

Done.

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:46: void
OverrideWebContentsDelegate() {
On 2014/11/27 14:37:37, Peter Beverloo wrote:
> I wonder if we could do this in SetUp(), avoiding having to call it
everywhere?

I've been experimenting with that but the DCHECK would fail. It seems that
SetUpOnMainThread() is what I need.

https://codereview.chromium.org/748373003/diff/60001/content/browser/manifest...
content/browser/manifest/manifest_browsertest.cc:97: if (source !=
web_contents_)
On 2014/11/27 14:37:37, Peter Beverloo wrote:
> When does this happen? Can it be a DCHECK?

Done.

https://codereview.chromium.org/748373003/diff/60001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/748373003/diff/60001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:122: CHECK(dictionary);
On 2014/11/27 14:37:37, Peter Beverloo wrote:
> *D*CHECK :-)

Done.

[mlamouri (slow - plz ping), 2014-11-27 14:53:11]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-11-27 14:53:45]

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

[commit-bot: I haz the power, 2014-11-27 15:50:47]

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

[commit-bot: I haz the power, 2014-11-27 15:50:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2014-11-27 15:54:39]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-11-27 15:55:52]

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

[commit-bot: I haz the power, 2014-11-27 16:38:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-27 16:39:25]

Patchset 5 (id:??) landed as
https://crrev.com/5bb64e626a33277374f4c23449c212cd3482e550
Cr-Commit-Position: refs/heads/master@{#305998}