Docserver: separate models for apps and extensions

Docserver: Separate models for apps and extensions.

BUG=322094
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279278

[ahernandez, 2014-06-17 17:13:45]

Looks like I had to make a separate upload, unless I can be made the owner of
Daniel's issue. I addressed most of the comments you left, so I've uploaded what
I have so far so you can review it in the meantime. 

What I didn't get to:

"that said... are we also testing something that is different in apps vs
extensions?"

Will check.

"Having the child just copy the properties from the parent, changing those that
are overridden, should work, right?"

Will look into.

[not at google - send to devlin, 2014-06-17 19:59:26]

Thanks. Ping the review when done, that last point was important.

[ahernandez, 2014-06-17 21:55:22]

On 2014/06/17 19:59:26, kalman wrote:
> Thanks. Ping the review when done, that last point was important.

I want to verify I'm understanding the situation correctly. With the example you
gave:

  "extension": {
    "channel": "stable",
    "extension_types": ["extension", "legacy_packaged_app"],
    "contexts": ["blessed_extension"]
  },
  "extension.getURL": {
    "contexts": ["blessed_extension", "unblessed_extension", "content_script"]
  },

we would expect the generated (merged?) features to look like:

  "extension": {
    "name": "extension",
    "channel": "stable",
    "contexts": ["blessed_extension"]
  },
  "extension.getURL": {
    "name": "extension",
    "channel": "stable",
    "contexts": ["blessed_extension", "unblessed_extension", "content_script"]
  },

correct?

[ahernandez, 2014-06-18 21:57:53]

https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder_test.py (right):

https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder_test.py:182: # Check
for differing availability across apps|extensions
Here's the new test. I added a new "appsFirst" API to the canned data and made
it available to apps first, then extensions.

[not at google - send to devlin, 2014-06-19 17:59:38]

On 2014/06/17 21:55:22, ahernandez.miralles wrote:
> On 2014/06/17 19:59:26, kalman wrote:
> > Thanks. Ping the review when done, that last point was important.
> 
> I want to verify I'm understanding the situation correctly. With the example
you
> gave:
> 
>   "extension": {
>     "channel": "stable",
>     "extension_types": ["extension", "legacy_packaged_app"],
>     "contexts": ["blessed_extension"]
>   },
>   "extension.getURL": {
>     "contexts": ["blessed_extension", "unblessed_extension", "content_script"]
>   },
> 
> we would expect the generated (merged?) features to look like:
> 
>   "extension": {
>     "name": "extension",
>     "channel": "stable",
>     "contexts": ["blessed_extension"]
>   },
>   "extension.getURL": {
>     "name": "extension",
>     "channel": "stable",
>     "contexts": ["blessed_extension", "unblessed_extension", "content_script"]
>   },
> 
> correct?

yes

[ahernandez, 2014-06-19 18:19:58]

On 2014/06/19 17:59:38, kalman wrote:
> On 2014/06/17 21:55:22, ahernandez.miralles wrote:
> > On 2014/06/17 19:59:26, kalman wrote:
> > > Thanks. Ping the review when done, that last point was important.
> > 
> > I want to verify I'm understanding the situation correctly. With the example
> you
> > gave:
> > 
> >   "extension": {
> >     "channel": "stable",
> >     "extension_types": ["extension", "legacy_packaged_app"],
> >     "contexts": ["blessed_extension"]
> >   },
> >   "extension.getURL": {
> >     "contexts": ["blessed_extension", "unblessed_extension",
"content_script"]
> >   },
> > 
> > we would expect the generated (merged?) features to look like:
> > 
> >   "extension": {
> >     "name": "extension",
> >     "channel": "stable",
> >     "contexts": ["blessed_extension"]
> >   },
> >   "extension.getURL": {
> >     "name": "extension",
> >     "channel": "stable",
> >     "contexts": ["blessed_extension", "unblessed_extension",
"content_script"]
> >   },
> > 
> > correct?
> 
> yes

I had added in that example to a test and the code handling parents as
dependencies correctly handled this example. Isn't copying parent properties and
using only the most restrictive ones effectively the same as adding the parent
as a dependency? Or am I missing something subtle?

[not at google - send to devlin, 2014-06-19 19:50:40]

On 2014/06/19 18:19:58, ahernandez.miralles wrote:
> On 2014/06/19 17:59:38, kalman wrote:
> > On 2014/06/17 21:55:22, ahernandez.miralles wrote:
> > > On 2014/06/17 19:59:26, kalman wrote:
> > > > Thanks. Ping the review when done, that last point was important.
> > > 
> > > I want to verify I'm understanding the situation correctly. With the
example
> > you
> > > gave:
> > > 
> > >   "extension": {
> > >     "channel": "stable",
> > >     "extension_types": ["extension", "legacy_packaged_app"],
> > >     "contexts": ["blessed_extension"]
> > >   },
> > >   "extension.getURL": {
> > >     "contexts": ["blessed_extension", "unblessed_extension",
> "content_script"]
> > >   },
> > > 
> > > we would expect the generated (merged?) features to look like:
> > > 
> > >   "extension": {
> > >     "name": "extension",
> > >     "channel": "stable",
> > >     "contexts": ["blessed_extension"]
> > >   },
> > >   "extension.getURL": {
> > >     "name": "extension",
> > >     "channel": "stable",
> > >     "contexts": ["blessed_extension", "unblessed_extension",
> "content_script"]
> > >   },
> > > 
> > > correct?
> > 
> > yes
> 
> I had added in that example to a test and the code handling parents as
> dependencies correctly handled this example. Isn't copying parent properties
and
> using only the most restrictive ones effectively the same as adding the parent
> as a dependency? Or am I missing something subtle?

The difference is as I explained in the other patch. With that example,
declaring parent as a dependency is equivalent to this:

  "extension": {
    "channel": "stable",
    "extension_types": ["extension", "legacy_packaged_app"],
    "contexts": ["blessed_extension"]
  },
  "extension.getURL": {
    "dependencies": ["api:extension"],
    "contexts": ["blessed_extension", "unblessed_extension", "content_script"]
  },

which mean that in order for extension.getURL to be available in say a content
script, it must be one of those contexts (check) AND the parent must be
available. However, the parent isn't available because it says the only context
is "blessed_extension".

This is an example with contexts, but the same *may* apply elsewhere, now or in
the future.

I'm not sure when I should have another final round of this CL. Ping me when.

[ahernandez, 2014-06-19 20:13:48]

On 2014/06/19 19:50:40, kalman wrote:
> On 2014/06/19 18:19:58, ahernandez.miralles wrote:
> > On 2014/06/19 17:59:38, kalman wrote:
> > > On 2014/06/17 21:55:22, ahernandez.miralles wrote:
> > > > On 2014/06/17 19:59:26, kalman wrote:
> > > > > Thanks. Ping the review when done, that last point was important.
> > > > 
> > > > I want to verify I'm understanding the situation correctly. With the
> example
> > > you
> > > > gave:
> > > > 
> > > >   "extension": {
> > > >     "channel": "stable",
> > > >     "extension_types": ["extension", "legacy_packaged_app"],
> > > >     "contexts": ["blessed_extension"]
> > > >   },
> > > >   "extension.getURL": {
> > > >     "contexts": ["blessed_extension", "unblessed_extension",
> > "content_script"]
> > > >   },
> > > > 
> > > > we would expect the generated (merged?) features to look like:
> > > > 
> > > >   "extension": {
> > > >     "name": "extension",
> > > >     "channel": "stable",
> > > >     "contexts": ["blessed_extension"]
> > > >   },
> > > >   "extension.getURL": {
> > > >     "name": "extension",
> > > >     "channel": "stable",
> > > >     "contexts": ["blessed_extension", "unblessed_extension",
> > "content_script"]
> > > >   },
> > > > 
> > > > correct?
> > > 
> > > yes
> > 
> > I had added in that example to a test and the code handling parents as
> > dependencies correctly handled this example. Isn't copying parent properties
> and
> > using only the most restrictive ones effectively the same as adding the
parent
> > as a dependency? Or am I missing something subtle?
> 
> The difference is as I explained in the other patch. With that example,
> declaring parent as a dependency is equivalent to this:
> 
>   "extension": {
>     "channel": "stable",
>     "extension_types": ["extension", "legacy_packaged_app"],
>     "contexts": ["blessed_extension"]
>   },
>   "extension.getURL": {
>     "dependencies": ["api:extension"],
>     "contexts": ["blessed_extension", "unblessed_extension", "content_script"]
>   },
> 
> which mean that in order for extension.getURL to be available in say a content
> script, it must be one of those contexts (check) AND the parent must be
> available. However, the parent isn't available because it says the only
context
> is "blessed_extension".
> 
> This is an example with contexts, but the same *may* apply elsewhere, now or
in
> the future.
> 
> I'm not sure when I should have another final round of this CL. Ping me when.

But the parent dependency is temporary and doesn't appear in the generated
features, so it shouldn't affect the child, right? As far as I can tell, the
dependencies are only used to determine which channel the feature should have,
and any additions to the dependency list (such as adding the parent) do not
carry over to the final output.

[not at google - send to devlin, 2014-06-19 21:06:55]

https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:110: # If this feature
has a parent, add the parent as a dependency.
(regarding your question)

I don't know the answer to that, but my read of this code is that |dependencies|
contains both the items from the "dependencies" key /and/ any parent. They are
different. Perhaps this method needs to be called something more specific? It
probably does the right thing given only the channel appears to be merged, and
it's not a list.

that makes me think though, how are the items like extension types being
inherited from the parent? dependencies? I looked at the test and it does seem
to be not quite right.

The easiest model I think would be to do multiple passes over these features,
one of which, and perhaps the first, to copy down all properties from parents to
their children.

https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle_test.py (right):

https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle_test.py:469: 'channel':
'dev'
i.e. this should include extension_types

[ahernandez, 2014-06-20 01:01:39]

On 2014/06/19 21:06:55, kalman wrote:
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> File chrome/common/extensions/docs/server2/features_bundle.py (right):
> 
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> chrome/common/extensions/docs/server2/features_bundle.py:110: # If this
feature
> has a parent, add the parent as a dependency.
> (regarding your question)
> 
> I don't know the answer to that, but my read of this code is that
|dependencies|
> contains both the items from the "dependencies" key /and/ any parent. They are
> different. Perhaps this method needs to be called something more specific? It
> probably does the right thing given only the channel appears to be merged, and
> it's not a list.
> 
> that makes me think though, how are the items like extension types being
> inherited from the parent? dependencies? I looked at the test and it does seem
> to be not quite right.
> 
> The easiest model I think would be to do multiple passes over these features,
> one of which, and perhaps the first, to copy down all properties from parents
to
> their children.
> 
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> File chrome/common/extensions/docs/server2/features_bundle_test.py (right):
> 
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> chrome/common/extensions/docs/server2/features_bundle_test.py:469: 'channel':
> 'dev'
> i.e. this should include extension_types

When do we want to keep items such as extension_types in the generated features?
In the test data a lot of the expected features don't retain the extension_type
key, but they retain other keys such as "value".

[not at google - send to devlin, 2014-06-20 02:51:38]

On 2014/06/20 01:01:39, ahernandez.miralles wrote:
> On 2014/06/19 21:06:55, kalman wrote:
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > File chrome/common/extensions/docs/server2/features_bundle.py (right):
> > 
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > chrome/common/extensions/docs/server2/features_bundle.py:110: # If this
> feature
> > has a parent, add the parent as a dependency.
> > (regarding your question)
> > 
> > I don't know the answer to that, but my read of this code is that
> |dependencies|
> > contains both the items from the "dependencies" key /and/ any parent. They
are
> > different. Perhaps this method needs to be called something more specific?
It
> > probably does the right thing given only the channel appears to be merged,
and
> > it's not a list.
> > 
> > that makes me think though, how are the items like extension types being
> > inherited from the parent? dependencies? I looked at the test and it does
seem
> > to be not quite right.
> > 
> > The easiest model I think would be to do multiple passes over these
features,
> > one of which, and perhaps the first, to copy down all properties from
parents
> to
> > their children.
> > 
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > File chrome/common/extensions/docs/server2/features_bundle_test.py (right):
> > 
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > chrome/common/extensions/docs/server2/features_bundle_test.py:469:
'channel':
> > 'dev'
> > i.e. this should include extension_types
> 
> When do we want to keep items such as extension_types in the generated
features?
> In the test data a lot of the expected features don't retain the
extension_type
> key, but they retain other keys such as "value".

I'm not sure. We do need to know the extension types in theory though that is
indeed unlikely to change.

I think my concern still stands though. This doesn't quite seem like the *right*
thing to do and the "right" thing is pretty easy to implement.

[ahernandez, 2014-06-20 03:09:09]

On 2014/06/20 02:51:38, kalman wrote:
> On 2014/06/20 01:01:39, ahernandez.miralles wrote:
> > On 2014/06/19 21:06:55, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > File chrome/common/extensions/docs/server2/features_bundle.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > chrome/common/extensions/docs/server2/features_bundle.py:110: # If this
> > feature
> > > has a parent, add the parent as a dependency.
> > > (regarding your question)
> > > 
> > > I don't know the answer to that, but my read of this code is that
> > |dependencies|
> > > contains both the items from the "dependencies" key /and/ any parent. They
> are
> > > different. Perhaps this method needs to be called something more specific?
> It
> > > probably does the right thing given only the channel appears to be merged,
> and
> > > it's not a list.
> > > 
> > > that makes me think though, how are the items like extension types being
> > > inherited from the parent? dependencies? I looked at the test and it does
> seem
> > > to be not quite right.
> > > 
> > > The easiest model I think would be to do multiple passes over these
> features,
> > > one of which, and perhaps the first, to copy down all properties from
> parents
> > to
> > > their children.
> > > 
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > File chrome/common/extensions/docs/server2/features_bundle_test.py
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > chrome/common/extensions/docs/server2/features_bundle_test.py:469:
> 'channel':
> > > 'dev'
> > > i.e. this should include extension_types
> > 
> > When do we want to keep items such as extension_types in the generated
> features?
> > In the test data a lot of the expected features don't retain the
> extension_type
> > key, but they retain other keys such as "value".
> 
> I'm not sure. We do need to know the extension types in theory though that is
> indeed unlikely to change.
> 
> I think my concern still stands though. This doesn't quite seem like the
*right*
> thing to do and the "right" thing is pretty easy to implement.

That's why I'm asking which features should be retained. I'm copying the parent
properties to the child, but that adds more than the test data is looking for,
usually because it adds an extension_types key that isn't present in the
expected output. And since you mentioned that one of the tests was slightly off,
I'm not as confident that the other data is 100% correct. Looking at the data I
didn't see a pattern but maybe I need to look harder.

[not at google - send to devlin, 2014-06-20 16:13:36]

On 2014/06/20 03:09:09, ahernandez.miralles wrote:
> On 2014/06/20 02:51:38, kalman wrote:
> > On 2014/06/20 01:01:39, ahernandez.miralles wrote:
> > > On 2014/06/19 21:06:55, kalman wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > > File chrome/common/extensions/docs/server2/features_bundle.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > > chrome/common/extensions/docs/server2/features_bundle.py:110: # If this
> > > feature
> > > > has a parent, add the parent as a dependency.
> > > > (regarding your question)
> > > > 
> > > > I don't know the answer to that, but my read of this code is that
> > > |dependencies|
> > > > contains both the items from the "dependencies" key /and/ any parent.
They
> > are
> > > > different. Perhaps this method needs to be called something more
specific?
> > It
> > > > probably does the right thing given only the channel appears to be
merged,
> > and
> > > > it's not a list.
> > > > 
> > > > that makes me think though, how are the items like extension types being
> > > > inherited from the parent? dependencies? I looked at the test and it
does
> > seem
> > > > to be not quite right.
> > > > 
> > > > The easiest model I think would be to do multiple passes over these
> > features,
> > > > one of which, and perhaps the first, to copy down all properties from
> > parents
> > > to
> > > > their children.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > > File chrome/common/extensions/docs/server2/features_bundle_test.py
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/344453003/diff/20001/chrome/common/extensions...
> > > > chrome/common/extensions/docs/server2/features_bundle_test.py:469:
> > 'channel':
> > > > 'dev'
> > > > i.e. this should include extension_types
> > > 
> > > When do we want to keep items such as extension_types in the generated
> > features?
> > > In the test data a lot of the expected features don't retain the
> > extension_type
> > > key, but they retain other keys such as "value".
> > 
> > I'm not sure. We do need to know the extension types in theory though that
is
> > indeed unlikely to change.
> > 
> > I think my concern still stands though. This doesn't quite seem like the
> *right*
> > thing to do and the "right" thing is pretty easy to implement.
> 
> That's why I'm asking which features should be retained. I'm copying the
parent
> properties to the child, but that adds more than the test data is looking for,
> usually because it adds an extension_types key that isn't present in the
> expected output. And since you mentioned that one of the tests was slightly
off,
> I'm not as confident that the other data is 100% correct. Looking at the data
I
> didn't see a pattern but maybe I need to look harder.

Retaining everything seems simplest.

[ahernandez, 2014-06-20 20:52:57]

This is now ready for review, PTAL.

https://codereview.chromium.org/344453003/diff/60001/native_client_sdk/doc_ge...
File native_client_sdk/doc_generated/community/application-gallery.html (right):

https://codereview.chromium.org/344453003/diff/60001/native_client_sdk/doc_ge...
native_client_sdk/doc_generated/community/application-gallery.html:15: Pepper
{{pepperversion}}, and may correspondingly require a newer version of
Chrome.</p>
This is a dirty change, but the source file for this seemed to be missing. The
previous {{peppernum}} was causing handlebar to complain.

[not at google - send to devlin, 2014-06-20 22:40:28]

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:79: if parent is not
None:
this should really be an assert I think, since you're only executing this code
if the feature exists, and you're not executing it if it's unresolved... so it
must have been resolved.

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:82: if key not in child
and key != 'name' and key != 'noparent':
"if key not in child and key not in ('name', 'noparent')"

though (a) this doesn't really apply if this method looks more like I said
below, and (b) why do you need to explicitly exclude name, do children not have
a 'name' property?

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:86: # overwrite the
child's value. However, list values should be merged.
actually list values shouldn't be merged. any list merging currently going on I
think is a hack to work around our lack of separating models?

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:89: 
anyway, I'd actually rather we return a new value here rather than mutating the
input.

I think this method can be more or less:

merged = copy(parent)
merged.pop('noparent', None)
merged.pop('name', None)
merged.update(child)
return merged

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:98: features_map):
indent += 2 for these to align with the (

https://codereview.chromium.org/344453003/diff/60001/native_client_sdk/doc_ge...
File native_client_sdk/doc_generated/community/application-gallery.html (right):

https://codereview.chromium.org/344453003/diff/60001/native_client_sdk/doc_ge...
native_client_sdk/doc_generated/community/application-gallery.html:15: Pepper
{{pepperversion}}, and may correspondingly require a newer version of
Chrome.</p>
On 2014/06/20 20:52:57, ahernandez.miralles wrote:
> This is a dirty change, but the source file for this seemed to be missing. The
> previous {{peppernum}} was causing handlebar to complain.

I'm told it shouldn't even be here actually, so you can delete it.

[not at google - send to devlin, 2014-06-20 22:40:49]

fwiw other than the above comments lg but just want to resolve them

[ahernandez, 2014-06-21 00:08:20]

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:79: if parent is not
None:
On 2014/06/20 22:40:28, kalman wrote:
> this should really be an assert I think, since you're only executing this code
> if the feature exists, and you're not executing it if it's unresolved... so it
> must have been resolved.

I was thinking the same thing, but None still manages to get passed in. I can
investigate that further if you think it might be bug of some sort.

[ahernandez, 2014-06-21 00:18:00]

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:82: if key not in child
and key != 'name' and key != 'noparent':
On 2014/06/20 22:40:28, kalman wrote:
> "if key not in child and key not in ('name', 'noparent')"
> 
> though (a) this doesn't really apply if this method looks more like I said
> below, and (b) why do you need to explicitly exclude name, do children not
have
> a 'name' property?

Sometimes the children don't have a name explicitly defined, so at line 190 we
check for that and add the correct name. I think that only really occurs in the
test data though, and there is some weird name overriding that occurs in
permission_data_source_test.py.

[ahernandez, 2014-06-21 01:10:17]

I have made the requested changes. PTAL

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:79: if parent is not
None:
On 2014/06/21 00:08:20, ahernandez.miralles wrote:
> On 2014/06/20 22:40:28, kalman wrote:
> > this should really be an assert I think, since you're only executing this
code
> > if the feature exists, and you're not executing it if it's unresolved... so
it
> > must have been resolved.
> 
> I was thinking the same thing, but None still manages to get passed in. I can
> investigate that further if you think it might be bug of some sort.
> 

I looked into this a bit more, and the reason that parent is sometimes None is
two passes are done over the features, and on the first pass features are not
resolved across FeaturesCaches, so it must be that the parent sometimes
(always?) resides in a different cache, so during the first pass None gets
passed in.

[not at google - send to devlin, 2014-06-23 22:45:02]

lgtm

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_bundle.py:79: if parent is not
None:
On 2014/06/21 01:10:17, ahernandez.miralles wrote:
> On 2014/06/21 00:08:20, ahernandez.miralles wrote:
> > On 2014/06/20 22:40:28, kalman wrote:
> > > this should really be an assert I think, since you're only executing this
> code
> > > if the feature exists, and you're not executing it if it's unresolved...
so
> it
> > > must have been resolved.
> > 
> > I was thinking the same thing, but None still manages to get passed in. I
can
> > investigate that further if you think it might be bug of some sort.
> > 
> 
> I looked into this a bit more, and the reason that parent is sometimes None is
> two passes are done over the features, and on the first pass features are not
> resolved across FeaturesCaches, so it must be that the parent sometimes
> (always?) resides in a different cache, so during the first pass None gets
> passed in.

ok, add comment

https://codereview.chromium.org/344453003/diff/100001/chrome/common/extension...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/344453003/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/features_bundle.py:79: if parent is not
None:
invert this condition so the majority of the body is not the exception

https://codereview.chromium.org/344453003/diff/100001/native_client_sdk/doc_g...
File native_client_sdk/doc_generated/community/application-gallery.html (left):

https://codereview.chromium.org/344453003/diff/100001/native_client_sdk/doc_g...
native_client_sdk/doc_generated/community/application-gallery.html:5: 
why did this file get removed? I don't think it should be..

(and if you rebase the original issue will be fixed)

[ahernandez, 2014-06-24 00:27:34]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-06-24 00:30:36]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/34445300...

[commit-bot: I haz the power, 2014-06-24 02:58:33]

Change committed as 279278