Allow extensions of the ManifestParser outside of the content/ layer.

Allow extensions of the ManifestParser outside of the content/ layer.

This is solving part of the issues raised in:
https://codereview.chromium.org/919293002/

BUG=457403

[mlamouri (slow - plz ping), 2015-02-19 18:36:37]

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

[mlamouri (slow - plz ping), 2015-02-19 18:36:38]

Avi, this is only solving part of the problem you raised but I guess it's a step
in the right direction. PTAL.

[Avi (use Gerrit), 2015-02-20 20:07:58]

I briefly looked at what you're doing with the ManifestParser, and it's not
along the lines of how we've done the Content API boundary.

What we do in the content API is put an interface in the public directory, and
an Impl class in the guts of it. We've never split up the header to be in the
public directory and the implementation in the private directory. And we try not
to do extensions via subclassing.

Can you leave the manifest parser entirely private, and have it call out through
a function if the key is unknown?

The general content api guidelines are at
http://www.chromium.org/developers/content-module/content-api but feel free to
ask me or John (jam@chromium) any questions.

https://codereview.chromium.org/936413003/diff/1/content/public/renderer/mani...
File content/public/renderer/manifest_parser.h (right):

https://codereview.chromium.org/936413003/diff/1/content/public/renderer/mani...
content/public/renderer/manifest_parser.h:48: // allow it to be overridden with
ContentRendererClient.
I can't parse this sentence. Rather than "it is not possible" say "disallow" if
that's what you mean.

And what's going on here? How does not allowing a direct instantiation interact
with the ContentRendererClient?

[mlamouri (slow - plz ping), 2015-02-23 14:50:58]

I did some update that follows your recommendations. The patch isn't entirely
updated because there are some bits of the API that we could improve I think and
I prefer to get feedback before writing some code in the wrong direction.

This CL is only delegating only key/value pairs of the parsing to the chrome/
layer but there are a few issues:
- as you can see, there is no result coming back because the content/ layer
doesn't know about the key/value so it can't really know what's the type going
to be;
- having the delegation be entirely stateless will make some things painful: no
way for the delegated parsing to make decision based on the rest of the Manifest
for example;
- the chrome/ layer parser will need to re-implement a lot of basic parsing
methods like ParseString() or ParseUrl().

[Avi (use Gerrit), 2015-02-23 17:30:25]

https://codereview.chromium.org/936413003/diff/20001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/936413003/diff/20001/content/content_renderer...
content/content_renderer.gypi:56: 'public/renderer/manifest_parser.h',
This file doesn't exist in this CL.

https://codereview.chromium.org/936413003/diff/20001/content/public/renderer/...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/936413003/diff/20001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:302: /* TODO: errors */);
The ContentRendererClient is a singleton, and thus stateless. Any context that a
function here would need has to be passed in, but I'm not sure how you're
intending this to work. The implementer of this function is getting a key and a
value, but what is it supposed to do with it? It wouldn't even know what
document it belonged to.

I thought that perhaps it would be parsed and the function would return some
chunk of data that content would hold onto, but it returns just a bool. And the
implementation you have in the ChromeContentRenderClient:

//*parser = new ChromeManifestParser();

leads me to think that you're not entirely sure how this is going to work,
either.

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

https://codereview.chromium.org/936413003/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:143: private:
Why this? We're already in a private section from line 40.

[mlamouri (slow - plz ping), 2015-02-23 17:38:34]

Please, see my comment. I have on purpose not fully implement that CL.

[mlamouri (slow - plz ping), 2015-02-24 12:30:17]

+CC benwells@

[Avi (use Gerrit), 2015-02-24 16:49:07]

Wow, I completely missed this comment. :( Apologies.

On 2015/02/23 14:50:58, Mounir Lamouri wrote:
> - the chrome/ layer parser will need to re-implement a lot of basic parsing
> methods like ParseString() or ParseUrl().

Can you export those as utility methods on the ManifestParser? The usual pattern
is the interface in public/ and impl in renderer/, but you could have all the
utility methods you need.

> - having the delegation be entirely stateless will make some things painful:
no
> way for the delegated parsing to make decision based on the rest of the
Manifest
> for example;

I don't know enough about manifests to comment here, and that's frustrating to
me because I want to give you suggestions and I don't know enough about them to
have any clever ideas. Is there some kind of common data structure that all
items in the manifest parse into, or is each item completely different?