Add support for gcm_sender_id in Manifest.

Add support for gcm_sender_id in Manifest.

It is a proprietary extension of the Web Manifest.

BUG=414873
Committed: https://crrev.com/9291444d2aa5e882e5a287d33f944e1c98f4c67c
Cr-Commit-Position: refs/heads/master@{#297197}

[mlamouri (slow - plz ping), 2014-09-16 19:58:04]

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

[mlamouri (slow - plz ping), 2014-09-16 19:58:50]

palmer@, can you look at the IPC changes again?

avi@, can you look at the content/ changes?

Thanks :)

[mlamouri (slow - plz ping), 2014-09-16 19:59:47]

+mvanouwerkerk@, +johnme@: FYI

[Avi (use Gerrit), 2014-09-16 20:02:18]

This looks reasonable to me. I'm not an expert in the manifest area; can you get
some more eyes on it?

(owner LGTM)

[mlamouri (slow - plz ping), 2014-09-16 20:09:20]

On 2014/09/16 20:02:18, Avi wrote:
> This looks reasonable to me. I'm not an expert in the manifest area; can you
get
> some more eyes on it?

I own manifest so I'm not sure who could look into that. Maybe jochen@ could
have a look but he is super busy.

[palmer, 2014-09-16 20:58:45]

lgtm

[Mike West, 2014-09-17 04:20:51]

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

[Mike West, 2014-09-17 04:20:52]

Drive-by LGTM with one comment. I'm not a //content OWNER, but I think I've
looked at enough of the Manifest stuff to help out here.

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

https://codereview.chromium.org/580513002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:84: const base::DictionaryValue&
dictionary)  {
Can you add an ASSERT that the runtime flag isn't enabled?

[mlamouri (slow - plz ping), 2014-09-17 08:35:41]

This is waiting for the required WebKit changes to land in order to go in.

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

https://codereview.chromium.org/580513002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:84: const base::DictionaryValue&
dictionary)  {
On 2014/09/17 04:20:52, Mike West wrote:
> Can you add an ASSERT that the runtime flag isn't enabled?

Done.

[jochen (gone - plz use gerrit), 2014-09-18 18:19:47]

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

[jochen (gone - plz use gerrit), 2014-09-18 18:19:47]

as mentioned on the webkit side, i wouldn't make the features interdepend on
each other. you're just parsing a string here, i don't see why you shouldn't do
that in any case.

[mlamouri (slow - plz ping), 2014-09-18 19:54:42]

On 2014/09/18 18:19:47, jochen wrote:
> as mentioned on the webkit side, i wouldn't make the features interdepend on
> each other. you're just parsing a string here, i don't see why you shouldn't
do
> that in any case.

The reason why I wouldn't want to do that in every case is mostly to control
what's happening with that string. Any consumer of the Manifest can read the
entire object and use whatever is present. There is currently one consumer in
the code but there are at least two others being planned. If you scale this to a
lot of consumers and as many "experimental" properties in the manifest, it will
be easy to end up exposing something by mistake.

Basically, my plan was to make the manifest parser the guard to prevent
properties to be exposed by mistake. It is human to make a mistake and find that
the Manifest exposes that thing that you think would be cool without realizing
it is actually not meant to be used because the code path using it is only
triggered when a specific flag is set in Blink.

All of that boils down to ensuring best practices with automated tools. We have
linters to check coding style, PRESUBMIT hooks to check more complex rules. The
reason for those is that we can't expect people to catch every single problems.
For the same reasons, I want to make sure that if someone reads an experimental
property of the Manifest by mistake, they will not be able to use it unless it's
okay to do so.

[jochen (gone - plz use gerrit), 2014-09-19 13:26:37]

On 2014/09/18 at 19:54:42, mlamouri wrote:
> On 2014/09/18 18:19:47, jochen wrote:
> > as mentioned on the webkit side, i wouldn't make the features interdepend on
> > each other. you're just parsing a string here, i don't see why you shouldn't
do
> > that in any case.
> 
> The reason why I wouldn't want to do that in every case is mostly to control
what's happening with that string. Any consumer of the Manifest can read the
entire object and use whatever is present. There is currently one consumer in
the code but there are at least two others being planned. If you scale this to a
lot of consumers and as many "experimental" properties in the manifest, it will
be easy to end up exposing something by mistake.
> 
> Basically, my plan was to make the manifest parser the guard to prevent
properties to be exposed by mistake. It is human to make a mistake and find that
the Manifest exposes that thing that you think would be cool without realizing
it is actually not meant to be used because the code path using it is only
triggered when a specific flag is set in Blink.

that's not what I would expect a parser to do.

> 
> All of that boils down to ensuring best practices with automated tools. We
have linters to check coding style, PRESUBMIT hooks to check more complex rules.
The reason for those is that we can't expect people to catch every single
problems. For the same reasons, I want to make sure that if someone reads an
experimental property of the Manifest by mistake, they will not be able to use
it unless it's okay to do so.

if a certain runtime enabled feature is off, you can't use it anyway. I don't
share your point of view here.

[mlamouri (slow - plz ping), 2014-09-22 09:53:34]

On 2014/09/19 13:26:37, jochen (slow for reviews) wrote:
> On 2014/09/18 at 19:54:42, mlamouri wrote:
> > All of that boils down to ensuring best practices with automated tools. We
> have linters to check coding style, PRESUBMIT hooks to check more complex
rules.
> The reason for those is that we can't expect people to catch every single
> problems. For the same reasons, I want to make sure that if someone reads an
> experimental property of the Manifest by mistake, they will not be able to use
> it unless it's okay to do so.
> 
> if a certain runtime enabled feature is off, you can't use it anyway. I don't
> share your point of view here.

My point isn't that the feature would still work even though it was disabled but
that another feature might use a manifest property created by a feature behind a
flag without realizing it is actually an experimental manifest feature.

[jochen (gone - plz use gerrit), 2014-09-24 19:55:12]

On 2014/09/22 at 09:53:34, mlamouri wrote:
> On 2014/09/19 13:26:37, jochen (slow for reviews) wrote:
> > On 2014/09/18 at 19:54:42, mlamouri wrote:
> > > All of that boils down to ensuring best practices with automated tools. We
> > have linters to check coding style, PRESUBMIT hooks to check more complex
rules.
> > The reason for those is that we can't expect people to catch every single
> > problems. For the same reasons, I want to make sure that if someone reads an
> > experimental property of the Manifest by mistake, they will not be able to
use
> > it unless it's okay to do so.
> > 
> > if a certain runtime enabled feature is off, you can't use it anyway. I
don't
> > share your point of view here.
> 
> My point isn't that the feature would still work even though it was disabled
but that another feature might use a manifest property created by a feature
behind a flag without realizing it is actually an experimental manifest feature.

still not convinced, sorry

[mlamouri (slow - plz ping), 2014-09-29 15:22:38]

Jochen and I discussed this today. I wasn't able to convinced him that having a
safe guard at the parser level was a good idea. In order to not block this
further, I made the property always available regardless of the Push API
availability in Blink.

[mlamouri (slow - plz ping), 2014-09-29 16:27:42]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-29 16:28:21]

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

[commit-bot: I haz the power, 2014-09-29 17:21:07]

Committed patchset #4 (id:60001) as 915dda49b12a79c769530233f837673ce9da0151

[commit-bot: I haz the power, 2014-09-29 17:21:39]

Patchset 4 (id:??) landed as
https://crrev.com/9291444d2aa5e882e5a287d33f944e1c98f4c67c
Cr-Commit-Position: refs/heads/master@{#297197}