Docserver: Modify doc inlining so all relevant data is present in availability_finder.py

Docserver: Modify doc inlining so all relevant data is present in availability_finder.py

Not inlining schema nodes for availability finder causes data to go missing and
availability to not be found. Inlining schema nodes means that availability data
gets messed up when a node is no longer inlined, because inlined nodes are
deleted from the schema. Solution: Inline nodes, but do not remove them from the
schema when used in availability_finder.py. This provides all the necessary data,
and doesn't interfere with availability data generation when a node is no longer
inlined because it will be still be present in the schema graphs.

BUG=233982
NOTRY=True

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

[ahernandez, 2014-07-10 22:03:16]

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py
(right):

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py:482:
'    // $ref:WasImplicitlyInlinedType',
I need a bit of help with this IDL file test. I want this ref to be detected as
inlineable, but it's not. I modified a line in schema_util.py to get some
working tests in the tabs.json data, but I don't know how to reference a node in
an IDL file so that it gets detected as inlineable.

[not at google - send to devlin, 2014-07-15 21:02:38]

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py
(right):

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py:482:
'    // $ref:WasImplicitlyInlinedType',
On 2014/07/10 22:03:15, ahernandez wrote:
> I need a bit of help with this IDL file test. I want this ref to be detected
as
> inlineable, but it's not. I modified a line in schema_util.py to get some
> working tests in the tabs.json data, but I don't know how to reference a node
in
> an IDL file so that it gets detected as inlineable.

here's an example:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

basically you have a type, then a method which takes it as a parameter.

[not at google - send to devlin, 2014-07-15 21:12:05]

nice approach, just a nit really, and awaiting result of your testing.

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_data_source.py:220:
logging.warning('No availability found for: %s' % self)
hooray. these logs definitely go away then?

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/schema_util.py (right):

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/schema_util.py:95: def ProcessSchema(path,
file_data, full_inline=False):
can you make this actually be explicit? "full_inline" is rather vague, perhaps
"remove_inlined_nodes" or "retain_inlined_nodes" (whichever makes more sense as
a default configuration; making it "retain" would be a backwards-compatible
change).

[ahernandez, 2014-07-15 22:02:31]

What should the BUG be/does there need to be a BUG for this?

[not at google - send to devlin, 2014-07-15 22:30:51]

On 2014/07/15 22:02:31, ahernandez wrote:
> What should the BUG be/does there need to be a BUG for this?

use that original bug with the object-level availability, if any.

[ahernandez, 2014-07-15 22:35:37]

On 2014/07/15 22:30:51, kalman wrote:
> On 2014/07/15 22:02:31, ahernandez wrote:
> > What should the BUG be/does there need to be a BUG for this?
> 
> use that original bug with the object-level availability, if any.

I've updated it accordingly. The new patchset is up, when you get a chance PTAL.

[not at google - send to devlin, 2014-07-15 22:43:21]

lgtm but please address those nits.

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/schema_util.py (right):

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/schema_util.py:95: def ProcessSchema(path,
file_data, full_inline=False):
On 2014/07/15 21:12:04, kalman wrote:
> can you make this actually be explicit? "full_inline" is rather vague, perhaps
> "remove_inlined_nodes" or "retain_inlined_nodes" (whichever makes more sense
as
> a default configuration; making it "retain" would be a backwards-compatible
> change).

I would prefer retain_inlined_nodes actually (reverse the condition), since
typically inlining does imply not leaving the original value around.

that also gives you a chance to update the call site to explain why you're
retaining those inlined types.

come to think about it, this specifically deals with types, so
"retain_inlined_types" would be more accurate.

https://codereview.chromium.org/385703005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/schema_util.py (right):

https://codereview.chromium.org/385703005/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/schema_util.py:57: def InlineDocs(schema,
remove_nodes):
please update this variable as well

https://codereview.chromium.org/385703005/diff/40001/chrome/common/extensions...
File
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py
(right):

https://codereview.chromium.org/385703005/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py:458:
'fake_tabs_with_inlining.idl'),
having a separate file for these is kinda overkill, can you inline the content?
perhaps as a private variable and using ''' ... '''?

[ahernandez, 2014-07-15 22:49:52]

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/schema_util.py (right):

https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/schema_util.py:95: def ProcessSchema(path,
file_data, full_inline=False):
On 2014/07/15 22:43:20, kalman wrote:
> On 2014/07/15 21:12:04, kalman wrote:
> > can you make this actually be explicit? "full_inline" is rather vague,
perhaps
> > "remove_inlined_nodes" or "retain_inlined_nodes" (whichever makes more sense
> as
> > a default configuration; making it "retain" would be a backwards-compatible
> > change).
> 
> I would prefer retain_inlined_nodes actually (reverse the condition), since
> typically inlining does imply not leaving the original value around.
> 
> that also gives you a chance to update the call site to explain why you're
> retaining those inlined types.
> 
> come to think about it, this specifically deals with types, so
> "retain_inlined_types" would be more accurate.

I believe this line:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

prevents me from reversing the condition, unless there's an easy way to get
around that.

[not at google - send to devlin, 2014-07-15 23:01:08]

On 2014/07/15 22:49:52, ahernandez wrote:
>
https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
> File chrome/common/extensions/docs/server2/schema_util.py (right):
> 
>
https://codereview.chromium.org/385703005/diff/1/chrome/common/extensions/doc...
> chrome/common/extensions/docs/server2/schema_util.py:95: def
ProcessSchema(path,
> file_data, full_inline=False):
> On 2014/07/15 22:43:20, kalman wrote:
> > On 2014/07/15 21:12:04, kalman wrote:
> > > can you make this actually be explicit? "full_inline" is rather vague,
> perhaps
> > > "remove_inlined_nodes" or "retain_inlined_nodes" (whichever makes more
sense
> > as
> > > a default configuration; making it "retain" would be a
backwards-compatible
> > > change).
> > 
> > I would prefer retain_inlined_nodes actually (reverse the condition), since
> > typically inlining does imply not leaving the original value around.
> > 
> > that also gives you a chance to update the call site to explain why you're
> > retaining those inlined types.
> > 
> > come to think about it, this specifically deals with types, so
> > "retain_inlined_types" would be more accurate.
> 
> I believe this line:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> prevents me from reversing the condition, unless there's an easy way to get
> around that.

Why is that? Do you need that method to not inline types? I was expecting that
you *would*.

but you can make that flag passable to ForAPISchema easily enough, and include
it in the category.

[ahernandez, 2014-07-16 02:00:57]

I had confused myself earlier, that's why my comments probably made no sense.
Could you give these changes another once over, in case I'm still confused.

[not at google - send to devlin, 2014-07-16 16:17:15]

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

https://codereview.chromium.org/385703005/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_models.py:42: # finding purposes; they
should be removed when creating Namespace objects.
See comment in schema_util. This comment shouldn't be necessary as this type
inlining is (was) supposed to be transparent to callers, the exception to that
rule is in AvailabilityFinder. So move this comment into there, but reverse it.

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

https://codereview.chromium.org/385703005/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/schema_util.py:96: def ProcessSchema(path,
file_data, retain_inlined_types=True):
Can this be default-False?

It looks like you'd just need to change the compiled_file_system instance to set
True and it'd work exactly the same... but is this really what you want? Like I
said I'd expect the naive caller to assume that a type doesn't appear
(effectively) twice, so *really* what you want is to add the
retain_inlined_types method to ForAPISchema.

*However* ForAPISchema is actually only used in 1 place, from
AvailabilityFinder, so perhaps you can just move that method in there and forget
about it on CompiledFileSystem entirely. Then you can explain whats going on in
AvailabliltyFinder.

[ahernandez, 2014-07-16 19:09:56]

I this what you meant? PTAL.

[not at google - send to devlin, 2014-07-16 20:03:50]

yep, lgtm.

https://codereview.chromium.org/385703005/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/385703005/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:155: def
comp_func(path, data):
s/comp_func/process_schema/

[ahernandez, 2014-07-16 20:54:17]

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

[commit-bot: I haz the power, 2014-07-16 20:59:11]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-07-16 23:38:34]

Change committed as 283579