Extensions: Fix DocServer assert failures.

Extensions: Fix DocServer assert failures.

Currently, there are a couple of assert failures happening for the DocServer.
Two issues are involved:
  
  - r446160 updated tools/json_schema_compiler/model.py. It changed the logic to
    assign the callback for a function, so that only the last parameter function
    of a function could be recognised as a callback. This broke some DocServer
    tests and hence the extension presubmit script.    
  - Recently the API endpoint
    https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1 that
    we use to fetch the branch for a particular Chrome version, stopped
    providing info. for version 15. This led to an assert failure when
    generating the Availability info. for the Webstore API.

This CL fixes the issues so that the extension presubmit tests pass. For issue
1, a new category type is introduced to APICategorizer for the internal APIs. It
is ensured that the API data source does not generate info. for internal APIs
which are never documented. Existing tests are updated to work around the fact
that only the last parameter of a function can be recognised as a callback.

For issue 2, it is ensured that the assert failure is not reached. However, the
core issue (omahaproxy endpoint not providing details of all Chrome versions)
still needs to be tackled.

BUG=696767, 700234
Review-Url: https://codereview.chromium.org/2738623003
Cr-Commit-Position: refs/heads/master@{#456767}
Committed: https://chromium.googlesource.com/chromium/src/+/08614915649c30c0278a9b154825e370b819a6de

[karandeepb, 2017-03-07 20:00:30]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 20:01:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 20:29:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 20:29:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[karandeepb, 2017-03-09 22:41:14]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:41:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 22:56:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 22:56:12]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-03-10 03:09:42]

Description was changed from

==========
Extensions: DocServer fix.
==========

to

==========
Extensions: Fix DocServer assert failures.

Currently, there are a couple of assert failures happening for the DocServer.
Two issues are involved:
  
  - r446160 updated tools/json_schema_compiler/model.py. It changed the logic to
    assign the callback for a function, so that only the last parameter function
    of a function could be recognised as a callback. This broke some DocServer
    tests and hence the extension presubmit script.    
  - Recently the API endpoint
    https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1 that
    we use to fetch the branch for a particular Chrome version, stopped
    providing info. for version 15. This led to an assert failure when
    generating the Availability info. for the Webstore API.

This CL fixes the issues so that the extension presubmit tests pass. For issue
1, a new category type is introduced to APICategorizer for the internal APIs. It
is ensured that the API data source does not generate info. for internal APIs
which are never documented. Existing tests are updated to work around the fact
that only the last parameter of a function can be recognised as a callback.

For issue 2, it is ensured that the assert failure is not reached. However, the
core issue (omahaproxy endpoint not providing details of all Chrome versions)
still needs to be tacked.

BUG=696767, 700234
==========

[karandeepb, 2017-03-10 03:11:17]

Description was changed from

==========
Extensions: Fix DocServer assert failures.

Currently, there are a couple of assert failures happening for the DocServer.
Two issues are involved:
  
  - r446160 updated tools/json_schema_compiler/model.py. It changed the logic to
    assign the callback for a function, so that only the last parameter function
    of a function could be recognised as a callback. This broke some DocServer
    tests and hence the extension presubmit script.    
  - Recently the API endpoint
    https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1 that
    we use to fetch the branch for a particular Chrome version, stopped
    providing info. for version 15. This led to an assert failure when
    generating the Availability info. for the Webstore API.

This CL fixes the issues so that the extension presubmit tests pass. For issue
1, a new category type is introduced to APICategorizer for the internal APIs. It
is ensured that the API data source does not generate info. for internal APIs
which are never documented. Existing tests are updated to work around the fact
that only the last parameter of a function can be recognised as a callback.

For issue 2, it is ensured that the assert failure is not reached. However, the
core issue (omahaproxy endpoint not providing details of all Chrome versions)
still needs to be tacked.

BUG=696767, 700234
==========

to

==========
Extensions: Fix DocServer assert failures.

Currently, there are a couple of assert failures happening for the DocServer.
Two issues are involved:
  
  - r446160 updated tools/json_schema_compiler/model.py. It changed the logic to
    assign the callback for a function, so that only the last parameter function
    of a function could be recognised as a callback. This broke some DocServer
    tests and hence the extension presubmit script.    
  - Recently the API endpoint
    https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1 that
    we use to fetch the branch for a particular Chrome version, stopped
    providing info. for version 15. This led to an assert failure when
    generating the Availability info. for the Webstore API.

This CL fixes the issues so that the extension presubmit tests pass. For issue
1, a new category type is introduced to APICategorizer for the internal APIs. It
is ensured that the API data source does not generate info. for internal APIs
which are never documented. Existing tests are updated to work around the fact
that only the last parameter of a function can be recognised as a callback.

For issue 2, it is ensured that the assert failure is not reached. However, the
core issue (omahaproxy endpoint not providing details of all Chrome versions)
still needs to be tackled.

BUG=696767, 700234
==========

[karandeepb, 2017-03-10 03:13:37]

karandeepb@chromium.org changed reviewers:
+ lazyboy@chromium.org, rdevlin.cronin@chromium.org

[karandeepb, 2017-03-10 03:13:38]

PTAL Devlin.

https://codereview.chromium.org/2738623003/diff/20001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_categorizer.py (right):

https://codereview.chromium.org/2738623003/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_categorizer.py:46: assert api_name not
in documented_apis, \
Do we have a common tool we use to format python files. Couldn't find one.

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

https://codereview.chromium.org/2738623003/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:150: else
self._branch_utility.GetChannelInfo(node_info['channel'])
This is required for issue 700234. I haven't dived much into this code, but it
seems that for some older APIs, if the info is not available through
https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1, our logic
shows them as available from the least version for which info. is available.

Hence for Webstore API, it'll show as available from Version 16 (Actual should
be 15). This also seems to be the case for multiple other APIs. See
https://developer.chrome.com/extensions/api_index where many APIs are available
since Chrome 15, which is probably incorrect.

https://codereview.chromium.org/2738623003/diff/20001/chrome/common/extension...
File
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py
(left):

https://codereview.chromium.org/2738623003/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py:387:
},
This is for https://bugs.chromium.org/p/chromium/issues/detail?id=696767#c4

[karandeepb, 2017-03-10 03:14:07]

karandeepb@chromium.org changed reviewers:
- lazyboy@chromium.org

[Devlin, 2017-03-13 20:38:18]

lgtm % nits

Before you land, can you double check that someone's ready to bump the app
engine instance?

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_categorizer.py (right):

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_categorizer.py:39: "private":     
APIs which are undocumented or are available to
Maybe "APIs which are available to whitelisted apps/extensions. Can be either
documented or undocumented."

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_categorizer.py:45: if
api_name.endswith('Internal'):
Let's hope we never have an fooInternal that's supposed to be public. ;)

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_schema_graph.py (right):

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_schema_graph.py:83: assert
self._lookup_path[-3] == 'events' , 'Invalid lookup path- %s' \
nitty nit: Let's separate the lines so the error string is together:
assert self._lookup_path[-3] == 'events' , \
    'Invalid lookup path: %s' % (self._lookup_path)

Also, let's replace the '-' with a ':' (which is a little more common).

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

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:148: channel_info =
self._branch_utility.GetStableChannelInfo(
nit: I think this is quite a bit more readable as:
if node_info['channel'] == 'stable':
  channel_info = self._branch_utility.GetStableChannelInfo(node_info['version'])
else
  channel_info = self._branch_utility.GetChannelInfo(node_info['channel'])

It's one more line of code, but python's ternaries are weird enough and the line
breaking is awkward enough that I find it very difficult to parse this in its
current form. :)

[karandeepb, 2017-03-14 00:59:04]

Patchset #4 (id:60001) has been deleted

[karandeepb, 2017-03-14 01:02:02]

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_categorizer.py (right):

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_categorizer.py:39: "private":     
APIs which are undocumented or are available to
On 2017/03/13 20:38:18, Devlin wrote:
> Maybe "APIs which are available to whitelisted apps/extensions. Can be either
> documented or undocumented."

But the code wouldn't be consistent with the comment then. We return 'private'
if it's actually Private (whitelisted) or if it's undocumented.

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_categorizer.py:45: if
api_name.endswith('Internal'):
On 2017/03/13 20:38:18, Devlin wrote:
> Let's hope we never have an fooInternal that's supposed to be public. ;)

Hopefully not!

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_schema_graph.py (right):

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_schema_graph.py:83: assert
self._lookup_path[-3] == 'events' , 'Invalid lookup path- %s' \
On 2017/03/13 20:38:18, Devlin wrote:
> nitty nit: Let's separate the lines so the error string is together:
> assert self._lookup_path[-3] == 'events' , \
>     'Invalid lookup path: %s' % (self._lookup_path)
> 
> Also, let's replace the '-' with a ':' (which is a little more common).

Done. Again is there a formatter commonly used for chromium python code?

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

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:148: channel_info =
self._branch_utility.GetStableChannelInfo(
On 2017/03/13 20:38:18, Devlin wrote:
> nit: I think this is quite a bit more readable as:
> if node_info['channel'] == 'stable':
>   channel_info =
self._branch_utility.GetStableChannelInfo(node_info['version'])
> else
>   channel_info = self._branch_utility.GetChannelInfo(node_info['channel'])
> 
> It's one more line of code, but python's ternaries are weird enough and the
line
> breaking is awkward enough that I find it very difficult to parse this in its
> current form. :)

Done.

[Devlin, 2017-03-14 01:12:05]

(slgtm)

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_categorizer.py (right):

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_categorizer.py:39: "private":     
APIs which are undocumented or are available to
On 2017/03/14 01:02:01, karandeepb wrote:
> On 2017/03/13 20:38:18, Devlin wrote:
> > Maybe "APIs which are available to whitelisted apps/extensions. Can be
either
> > documented or undocumented."
> 
> But the code wouldn't be consistent with the comment then. We return 'private'
> if it's actually Private (whitelisted) or if it's undocumented.

Ah, right you are.  Nevermind.

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_schema_graph.py (right):

https://codereview.chromium.org/2738623003/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_schema_graph.py:83: assert
self._lookup_path[-3] == 'events' , 'Invalid lookup path- %s' \
On 2017/03/14 01:02:02, karandeepb wrote:
> On 2017/03/13 20:38:18, Devlin wrote:
> > nitty nit: Let's separate the lines so the error string is together:
> > assert self._lookup_path[-3] == 'events' , \
> >     'Invalid lookup path: %s' % (self._lookup_path)
> > 
> > Also, let's replace the '-' with a ':' (which is a little more common).
> 
> Done. Again is there a formatter commonly used for chromium python code?

No formatter I'm aware of.  I think there are some tools that probably do "a
decent job", but nothing canonical.

[karandeepb, 2017-03-14 01:21:40]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-03-14 01:21:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2738623003/#ps80001
(title: "Address review.")

[commit-bot: I haz the power, 2017-03-14 01:22:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2017-03-14 02:36:56]

The CQ bit was unchecked by karandeepb@chromium.org

[karandeepb, 2017-03-14 18:14:35]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-03-14 18:15:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 18:28:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489515275383990,
"parent_rev": "cca03377f7a4e4357e6933be00ba8c205c765099", "commit_rev":
"08614915649c30c0278a9b154825e370b819a6de"}

[commit-bot: I haz the power, 2017-03-14 18:28:53]

Description was changed from

==========
Extensions: Fix DocServer assert failures.

Currently, there are a couple of assert failures happening for the DocServer.
Two issues are involved:
  
  - r446160 updated tools/json_schema_compiler/model.py. It changed the logic to
    assign the callback for a function, so that only the last parameter function
    of a function could be recognised as a callback. This broke some DocServer
    tests and hence the extension presubmit script.    
  - Recently the API endpoint
    https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1 that
    we use to fetch the branch for a particular Chrome version, stopped
    providing info. for version 15. This led to an assert failure when
    generating the Availability info. for the Webstore API.

This CL fixes the issues so that the extension presubmit tests pass. For issue
1, a new category type is introduced to APICategorizer for the internal APIs. It
is ensured that the API data source does not generate info. for internal APIs
which are never documented. Existing tests are updated to work around the fact
that only the last parameter of a function can be recognised as a callback.

For issue 2, it is ensured that the assert failure is not reached. However, the
core issue (omahaproxy endpoint not providing details of all Chrome versions)
still needs to be tackled.

BUG=696767, 700234
==========

to

==========
Extensions: Fix DocServer assert failures.

Currently, there are a couple of assert failures happening for the DocServer.
Two issues are involved:
  
  - r446160 updated tools/json_schema_compiler/model.py. It changed the logic to
    assign the callback for a function, so that only the last parameter function
    of a function could be recognised as a callback. This broke some DocServer
    tests and hence the extension presubmit script.    
  - Recently the API endpoint
    https://omahaproxy.appspot.com/history.json?channel=dev&os=win&json=1 that
    we use to fetch the branch for a particular Chrome version, stopped
    providing info. for version 15. This led to an assert failure when
    generating the Availability info. for the Webstore API.

This CL fixes the issues so that the extension presubmit tests pass. For issue
1, a new category type is introduced to APICategorizer for the internal APIs. It
is ensured that the API data source does not generate info. for internal APIs
which are never documented. Existing tests are updated to work around the fact
that only the last parameter of a function can be recognised as a callback.

For issue 2, it is ensured that the assert failure is not reached. However, the
core issue (omahaproxy endpoint not providing details of all Chrome versions)
still needs to be tackled.

BUG=696767, 700234

Review-Url: https://codereview.chromium.org/2738623003
Cr-Commit-Position: refs/heads/master@{#456767}
Committed:
https://chromium.googlesource.com/chromium/src/+/08614915649c30c0278a9b154825...
==========

[commit-bot: I haz the power, 2017-03-14 18:28:54]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/08614915649c30c0278a9b154825...