Implement ManifestParser in content/.

content/public/common/manifest.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_COMMON_MANIFEST_H_
+#define CONTENT_PUBLIC_COMMON_MANIFEST_H_
+
+#include "base/strings/nullable_string16.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+// The Manifest structure is an internal representation of the Manifest file
+// described in the "Manifest for Web Application" document:
+// http://w3c.github.io/manifest/
+struct CONTENT_EXPORT Manifest {

[Michael van Ouwerkerk, 2014/09/08 17:07:42]

This struct just represents a pair of strings, then? I think jam would say "just
pass the two strings, no need to define a new type in the public content API if
it can be so easily avoided".

[mlamouri (slow - plz ping), 2014/09/08 17:13:16]

It's only a pair of strings for the moment. It is meant to be way more than that
but I don't want to overweight the initial CL so I've only implemented the two
very trivial Manifest properties here.

[Michael van Ouwerkerk, 2014/09/08 17:36:27]

If it's going to be more complex, that might be a problem as well. Please share
a temporary patch or a detailed design indicating what this will do when
complete.

[mlamouri (slow - plz ping), 2014/09/08 17:39:48]

It will never be complete. The manifest spec (see link on top of |struct
CONTENT_EXPORT Manifest {| is listing some properties that we might or might not
support in the future. I'm planning to implement a few features in a near
future, including start_url and icons.

This structure will be bigger but not really more complex. It's going to stay a
property bag.

[Michael van Ouwerkerk, 2014/09/09 12:41:53]

For the record, I'm not opposed to adding types to the public content API the
way jam is. Your current approach looks reasonable to me. But, I'm not an owner
here. Mind you, with the addition of icons structures, Manifest will no longer
be a simple (flat) property bag.

[mlamouri (slow - plz ping), 2014/09/09 12:45:47]

What would be a reasonable alternative?

[Michael van Ouwerkerk, 2014/09/09 13:11:58]

Reasonable is subjective. Like I said, your current approach looks reasonable to
me.

+  bool IsEmpty() const;
+
+  // Null if the parsing failed or the field was not present.
+  base::NullableString16 name;

[Michael van Ouwerkerk, 2014/09/09 12:41:53]

From nullable_string16.h: "This should be used only where the difference between
null and empty is meaningful."

How is it meaningful? Having empty strings as values for name and short_name
seems undesirable as well, and could be considered 'empty'.

[mlamouri (slow - plz ping), 2014/09/09 12:45:47]

There actually is a difference. I want to be able to distinguish those
situations:
{
}
{
  "name": { }
}
{
  "name": ""
}

In the first two cases, the string will be null. In the last, it will be empty.
I think it is better to have a clear distinction (for all the properties)
between parsed or not parsed.

[Michael van Ouwerkerk, 2014/09/09 13:11:58]

Perhaps a note for IsEmpty then, indicating that it returns true when none of
the fields have been set, as opposed to (not set || falsy).

[mlamouri (slow - plz ping), 2014/09/09 13:23:15]

Done.

+
+  // Null if the parsing failed or the field was not present.
+  base::NullableString16 short_name;
+};
+
+} // namespace content
+
+#endif // CONTENT_PUBLIC_COMMON_MANIFEST_H_