Issue 25627002: Docserver: when checking for the existence of an API on a branch, just look at

Issue 25627002: Docserver: when checking for the existence of an API on a branch, just look at (Closed)

Created:
7 years, 2 months ago by not at google - send to devlin

Modified:
7 years, 2 months ago

Reviewers:
Jeffrey Yasskin

CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Docserver: when checking for the existence of an API on a branch, just look at the filename, don't try to parse the whole JSON/IDL file unless absolutely necessary. Halves the time it takes to run a cron (it recently regressed). R=jyasskin@chromium.org NOTRY=true Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226372

Patch Set 1 #

Patch Set 2 : revert some unnecessary stuff #

Total comments: 12

Patch Set 3 : jeffrey #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+91 lines, -59 lines)			Patch
M	chrome/common/extensions/docs/server2/api_schema_graph.py	View		2 chunks	+7 lines, -2 lines	0 comments	Download
M	chrome/common/extensions/docs/server2/app.yaml	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/common/extensions/docs/server2/availability_finder.py	View	1 2	7 chunks	+52 lines, -31 lines	0 comments	Download
M	chrome/common/extensions/docs/server2/branch_utility.py	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/docs/server2/cron.yaml	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py	View		11 chunks	+24 lines, -24 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Jeffrey Yasskin

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/docs/server2/api_schema_graph.py File chrome/common/extensions/docs/server2/api_schema_graph.py (right): https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/docs/server2/api_schema_graph.py#newcode26 chrome/common/extensions/docs/server2/api_schema_graph.py:26: return '%s%s' % (type(self).__name__, repr(self.__dict__)) This is going to ...

7 years, 2 months ago (2013-10-02 00:19:16 UTC) #2

not at google - send to devlin

7 years, 2 months ago (2013-10-02 00:40:02 UTC) #3

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/api_schema_graph.py (right):

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_schema_graph.py:26: return '%s%s' %
(type(self).__name__, repr(self.__dict__))
On 2013/10/02 00:19:17, Jeffrey Yasskin wrote:
> This is going to give something like "LookupResult{'found': found,
'annotation':
> annotation}". Is that what you want?

Yep it's what I want.

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/availability_finder.py:69: # Prior to
Chrome version 18, extension_api.json contained all API schema
On 2013/10/02 00:19:17, Jeffrey Yasskin wrote:
> Do we still need this code?

Yes, this is the class responsible for stepping back through history to find out
when APIs were added, changed, etc. It specifically needs to deal with legacy
cases.

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/availability_finder.py:100: if
api['namespace'] == api_name][0]
On 2013/10/02 00:19:17, Jeffrey Yasskin wrote:
> It looks like the old code would return None if there were no matching API
name.
> This will throw an exception instead. Is that a good idea?

It still does return None; _GetApiSchemaFilename would return None if it's not
found in extension_api.json, so this method would return None. It might as well
crash here.

HOWEVER I actually think this code is kind of wrong anyway (see below), but I'll
write it in such a way to make that clearer.

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/availability_finder.py:103: return
json_parse.Parse(api_file_text)  # [0]?
On 2013/10/02 00:19:17, Jeffrey Yasskin wrote:
> Figure out the "[0]?" before checking this in.

Done.

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/availability_finder.py:156: return
_GetApiSchemaFilename(api_name, file_system) is not None
On 2013/10/02 00:19:17, Jeffrey Yasskin wrote:
> Replace these with "ApiExists(api_name, file_system)" or something similar,
> rather than encoding that in a filename is not None check.

Done.

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/25627002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:34: return '%s%s' %
(type(self).__name__, repr(self.__dict__))
On 2013/10/02 00:19:17, Jeffrey Yasskin wrote:
> Same comment as for LookupResult.

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kalman@chromium.org/25627002/10001

7 years, 2 months ago (2013-10-02 00:46:32 UTC) #5

Message was sent while issue was closed.

Change committed as 226372

Expand Messages | Collapse Messages