Issue 159607: Extension docs build script, gyp target and PRESUBMIT.PY check

Issue 159607: Extension docs build script, gyp target and PRESUBMIT.PY check (Closed)

Created:
11 years, 4 months ago by rafaelw

Modified:
9 years, 7 months ago

Reviewers:
Aaron Boodman, asargent_no_longer_on_chrome, M-A Ruel, Daniel Berlin, sgk

CC:
chromium-reviews_googlegroups.com

Visibility:
Public.

Description

Extension docs build script, gyp target and PRESUBMIT.PY check This addeds a mechanism for extension docs to "render" from the api/extension_api.json api "idl", the docs/static file content and the template/ html templates into the static extension docs. It retains the property that extension docs loaded in chrome with file:/// scheme will regenerate if dependent data has changed, but otherwise is static. The build.py script runs test_shell to render the static files. A extension_docs gyp target is defined with appropriate inputs & outputs that will run the build.py A PRESUBMIT.PY check will warn on upload and commit if any files have changed that would require a docs rebuild. Note that the PRESUBMIT.PY checks are not currently being run for git cl upload or git cl dcommit. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=22300

Patch Set 1 #

Patch Set 2 : pre cr #

Patch Set 3 : more pre cr #

Total comments: 17

Patch Set 4 : cr changes. added simplejson #

Patch Set 5 : build paths for linux & mac #

Total comments: 18

Patch Set 6 : cr changes #

Total comments: 1

Patch Set 7 : rebase #

Patch Set 8 : last cr change #

Patch Set 9 : remove build step from mac #

Patch Set 10 : remove build step on mac #

Created: 11 years, 4 months ago

(Patch set is too large to download)

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+10141 lines, -974 lines)			Patch
M	chrome/chrome.gyp	View	2 3 4 5 6 7 8	1 chunk	+19 lines, -0 lines	0 comments	Download
M	chrome/common/common_resources.grd	View		1 chunk	+1 line, -1 line	0 comments	Download
A	chrome/common/extensions/PRESUBMIT.PY	View	2 3	1 chunk	+62 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/api/extension_api.json	View		1 chunk	+400 lines, -487 lines	0 comments	Download
M	chrome/common/extensions/docs/api_index.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/bookmarks.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
A	chrome/common/extensions/docs/build/build.py	View	1 2 3 4 5 6 7 8	1 chunk	+164 lines, -0 lines	0 comments	Download
A	chrome/common/extensions/docs/build/generator.html	View	1 2	1 chunk	+38 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/docs/devguide.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/getstarted.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/index.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/js/api_page_generator.js	View	1 2 3 4 5 6 7 8	12 chunks	+31 lines, -18 lines	0 comments	Download
A	chrome/common/extensions/docs/js/bootstrap.js	View		1 chunk	+11 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/docs/overview.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/pageActions.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/self.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/tabs.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
M	chrome/common/extensions/docs/template/page_shell.html	View	1 2 3	1 chunk	+1 line, -13 lines	0 comments	Download
M	chrome/common/extensions/docs/windows.html	View	1 2 3	1 chunk	+1 line, -46 lines	0 comments	Download
A	third_party/python_24/Lib/site-packages/simplejson.pth	View		1 chunk	+4 lines, -0 lines	0 comments	Download
A	third_party/simplejson/LICENSE.txt	View		1 chunk	+19 lines, -0 lines	0 comments	Download
A	third_party/simplejson/PKG-INFO	View		1 chunk	+29 lines, -0 lines	0 comments	Download
A	third_party/simplejson/conf.py	View		1 chunk	+179 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_sources/index.txt	View		1 chunk	+428 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/contents.png	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A	third_party/simplejson/docs/_static/default.css	View		1 chunk	+833 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/doctools.js	View		1 chunk	+352 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/file.png	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A	third_party/simplejson/docs/_static/interface.js	View		1 chunk	+8 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/jquery.js	View		1 chunk	+32 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/minus.png	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A	third_party/simplejson/docs/_static/navigation.png	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A	third_party/simplejson/docs/_static/plus.png	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A	third_party/simplejson/docs/_static/pygments.css	View		1 chunk	+59 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/rightsidebar.css	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/searchtools.js	View		1 chunk	+404 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/sphinxdoc.css	View		1 chunk	+504 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/stickysidebar.css	View		1 chunk	+19 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/_static/traditional.css	View		1 chunk	+700 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/genindex.html	View		1 chunk	+135 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/index.html	View	7	1 chunk	+503 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/search.html	View		1 chunk	+81 lines, -0 lines	0 comments	Download
A	third_party/simplejson/docs/searchindex.json	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/simplejson/ez_setup.py	View		1 chunk	+233 lines, -0 lines	0 comments	Download
A	third_party/simplejson/index.rst	View		1 chunk	+428 lines, -0 lines	0 comments	Download
A	third_party/simplejson/scripts/make_docs.py	View		1 chunk	+17 lines, -0 lines	0 comments	Download
A	third_party/simplejson/setup.cfg	View		1 chunk	+5 lines, -0 lines	0 comments	Download
A	third_party/simplejson/setup.py	View		1 chunk	+122 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson.egg-info/PKG-INFO	View		1 chunk	+29 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson.egg-info/SOURCES.txt	View		1 chunk	+54 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson.egg-info/dependency_links.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson.egg-info/top_level.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson.egg-info/zip-safe	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/__init__.py	View		1 chunk	+318 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/_speedups.c	View		1 chunk	+2329 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/decoder.py	View		1 chunk	+354 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/encoder.py	View		1 chunk	+440 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/scanner.py	View		1 chunk	+65 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/__init__.py	View		1 chunk	+23 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_check_circular.py	View		1 chunk	+30 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_decode.py	View		1 chunk	+22 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_default.py	View		1 chunk	+9 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_dump.py	View		1 chunk	+21 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_encode_basestring_ascii.py	View		1 chunk	+38 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_fail.py	View		1 chunk	+76 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_float.py	View		1 chunk	+15 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_indent.py	View		1 chunk	+41 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_pass1.py	View		1 chunk	+76 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_pass2.py	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_pass3.py	View		1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_recursion.py	View		1 chunk	+67 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_scanstring.py	View		1 chunk	+111 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_separators.py	View		1 chunk	+42 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tests/test_unicode.py	View		1 chunk	+64 lines, -0 lines	0 comments	Download
A	third_party/simplejson/simplejson/tool.py	View		1 chunk	+37 lines, -0 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Aaron Boodman

lgtm http://codereview.chromium.org/159607/diff/1022/36 File chrome/common/extensions/docs/api_index.html (right): http://codereview.chromium.org/159607/diff/1022/36#newcode1 Line 1: <!DOCTYPE html><!-- This page is a placeholder ...

11 years, 4 months ago (2009-07-30 23:14:45 UTC) #1

lgtm

http://codereview.chromium.org/159607/diff/1022/36
File chrome/common/extensions/docs/api_index.html (right):

http://codereview.chromium.org/159607/diff/1022/36#newcode1
Line 1: <!DOCTYPE html><!-- This page is a placeholder for generated extensions
api doc. Note: 1) The <head> information in this page is significant, should be
uniform across api docs and should be edited only with knowledge of the
templating mechanism. 2) The <body> tag *must* retain id="body" 3) All
<body>.innerHTML is genereated as an rendering step. If viewed in a browser, it
will be re-generated from the template, json schema and authored overview
content. 4) The <body>.innerHTML is also generated by an offline step so that
this page may easily be indexed by search engines. TODO(rafaelw): Abstract this
into a "pageshell" that becomes the single version of page template shell and
the "instance" pages (bookmarks.html, etc...) can be generated with a build
step. --><!-- <html> must retain id="template --><html
xmlns="http://www.w3.org/1999/xhtml" jstcache="0"><!-- <head> data is
significant and loads the needed libraries and styles --><head jstcache="0">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" jstcache="0">
<title jscontent="pageTitle" jstcache="1">api_index</title> <link
href="css/ApiRefStyles.css" rel="stylesheet" type="text/css" jstcache="0">
<script type="text/javascript"
src="../../../third_party/jstemplate/jstemplate_compiled.js" jstcache="0">
</script> <script type="text/javascript" src="js/api_page_generator.js"
jstcache="0"></script> <script type="text/javascript" src="js/boostrap.js"
jstcache="0"></script> </head><!-- <body> content is completely generated. Do
not edit, as it will be and rewritten. --><body class="" jstcache="0"> <div
id="container" jstcache="0"> <a name="top" jstcache="0"> </a> <!-- API HEADER
--> <div id="pageHeader" jstcache="0"> <!-- BREADCRUMB --> <!-- TODO: Fix these
hrefs --> <div id="breadcrumbs" jstcache="0"> <a href="index.html"
jstcache="0">Google Chrome Extensions</a> &gt; <a href="reference_index.html"
jstcache="0">Reference</a> &gt; <a href="api_index.html" jstcache="0">chrome.*
APIs</a> &gt; <span jscontent="namespace" jstcache="2">null</span> </div> <div
id="searchbox" jstcache="0"> <form action="http://www.google.com/cse"
id="cse-search-box" jstcache="0"> <div jstcache="0"> <input type="hidden"
name="cx" value="002967670403910741006:61_cvzfqtno" jstcache="0"> <input
type="hidden" name="ie" value="UTF-8" jstcache="0"> <input type="text" name="q"
size="31" jstcache="0"> <input type="submit" name="sa" value="Search"
jstcache="0"> </div> </form> <script type="text/javascript"
src="http://www.google.com/jsapi" jstcache="0"></script> <script
type="text/javascript" jstcache="0">google.load("elements", "1", {packages:
"transliteration"});</script> <script type="text/javascript"
src="http://www.google.com/coop/cse/t13n?form=cse-search-box&amp;t13n_langs=en"
jstcache="0"></script> <script type="text/javascript"
src="http://www.google.com/coop/cse/brand?form=cse-search-box&amp;lang=en"
jstcache="0"></script> </div> <div id="pageTitle" jstcache="0"> <h1
jscontent="h1Header" jstcache="3">api_index</h1> </div> </div> <!-- /pageHeader
--> <div id="pageContent" jstcache="0"> <!-- SIDENAV --> <div id="leftNav"
jstcache="0"> <ul jstcache="0"> <li jstcache="0"> <a href="overview.html"
jstcache="0">Overview</a></li> <li jstcache="0"> <a href="getstarted.html"
jstcache="0">Get Started</a></li> <li jstcache="0"> <a href="devguide.html"
jstcache="0">Developer's Guide</a></li> <li jstcache="0"> Reference <ul
jstcache="0"> <li jstcache="0"> <a href="api_index.html" jstcache="0">Extension
APIs</a> <ul jstcache="0"> <li jsselect="apiModules" jstcache="18"
jsinstance="0"> <a jscontent="name" jsvalues=".href:module + '.html'"
jstcache="25" href="bookmarks.html">Bookmarks</a> </li><li jsselect="apiModules"
jstcache="18" jsinstance="1"> <a jscontent="name" jsvalues=".href:module +
'.html'" jstcache="25" href="pageActions.html">PageActions</a> </li><li
jsselect="apiModules" jstcache="18" jsinstance="2"> <a jscontent="name"
jsvalues=".href:module + '.html'" jstcache="25" href="windows.html">Windows</a>
</li><li jsselect="apiModules" jstcache="18" jsinstance="3"> <a jscontent="name"
jsvalues=".href:module + '.html'" jstcache="25" href="tabs.html">Tabs</a>
</li><li jsselect="apiModules" jstcache="18" jsinstance="*4"> <a
jscontent="name" jsvalues=".href:module + '.html'" jstcache="25"
href="self.html">Self</a> </li> </ul> </li> </ul> </li> </ul> </div> <div
id="mainColumn" jstcache="0"> <!-- TABLE OF CONTENTS --> <div id="toc"
jsselect="apiDefinition" jstcache="4" style="display: none; "> <p
jstcache="0">Contents</p> <ol jstcache="0"> <li jstcache="0"> <a
href="#overview" jstcache="0">Description</a> <ol jstcache="0"> <li
jstcache="0"><a href="#overview-properties" jstcache="0">Properties</a></li> <li
jstcache="0"><a href="#overview-examples" jstcache="0">Examples</a></li> </ol>
</li> <li jstcache="0"> <a href="#methods" jstcache="0">Methods</a> <ol
jstcache="0"> <li jsselect="functions" jstcache="5"> <a jscontent="name"
jsvalues=".href:'#method-' + name" href="#method-anchor"
jstcache="19">methodName</a> </li> </ol> </li> <li jstcache="0"> <a
href="#events" jstcache="0">Events</a> <ol jstcache="0"> <li jsselect="events"
jstcache="6"> <a jscontent="name" jsvalues=".href:'#event-' + name"
href="#event-anchor" jstcache="20">eventName</a> </li> </ol> </li> <!-- TODO:
What do we do about structs/types? --> <li jstcache="0"> <a href="#structs"
jstcache="0">TODO: Structs</a> <ol jstcache="0"> <li jstcache="0"><a
href="#struct-BookmarkTreeNode" jstcache="0"></a></li> </ol> </li> </ol>
[PENDING: links to all h2s and h3s should go here -- would it be possible to
link to overview h3s, as well? if so, how should we create their anchor/id
values?] </div> <!-- /TABLE OF CONTENTS --> <!-- STATIC CONTENT PLACEHOLDER -->
<div id="static" jstcache="0"><p jstcache="0"> [PENDING: links to reference doc
— for example, <a href="chrome-api-index.html" jstcache="0">chrome.* APIs</a> —
go here.] </p> <p jstcache="0"> Lorem ipsum dolor sit amet, consectetur
adipiscing elit. Fusce sed elit felis. Vestibulum porta mauris eget mi placerat
a condimentum leo semper. Ut scelerisque, ipsum in cursus semper, lectus enim
molestie ante, a porttitor dolor neque sit amet lorem. Integer dignissim gravida
eros ac rhoncus. Curabitur porttitor hendrerit dolor nec aliquam. Ut vulputate
ultrices eros eu ultrices. Pellentesque tempor mattis odio in blandit. Proin
nulla metus, tristique eget iaculis gravida, varius at leo. Ut dolor mi, auctor
et scelerisque vitae, volutpat eleifend magna. Pellentesque habitant morbi
tristique senectus et netus et malesuada fames ac turpis egestas. Morbi ut risus
nec massa consectetur accumsan sed non est. Fusce ornare eros et felis dignissim
convallis. Maecenas condimentum purus eget nisl condimentum vitae ornare diam
sollicitudin. Nulla et dictum ante. In lacus odio, dapibus vel faucibus at,
posuere id enim. Nulla tincidunt felis id lectus convallis vulputate. Phasellus
libero dui, posuere sollicitudin egestas eget, blandit in urna. Sed cursus
tellus sed diam hendrerit pulvinar. </p> <p jstcache="0"> Nam tincidunt mollis
aliquam. Suspendisse bibendum dignissim dui, ac mollis ligula fringilla sit
amet. Pellentesque dapibus lobortis dignissim. Cras eu lorem a ligula imperdiet
ultricies. Phasellus ipsum mi, accumsan quis dignissim eu, commodo ut mi. Nulla
sagittis aliquet malesuada. Ut lobortis tellus vitae dolor venenatis eu
ullamcorper lorem gravida. Quisque non pharetra velit. Maecenas elit risus,
ultricies in sagittis eget, facilisis sed neque. Quisque feugiat porta pharetra.
Vestibulum lorem magna, pellentesque et mattis sit amet, euismod et dui.
</p></div> <!-- API PAGE --> <div class="apiPage" jsselect="apiDefinition"
jstcache="4" style="display: none; "> <!-- METHODS --> <div class="apiGroup"
id="methods" jstcache="0"> <a name="#methods" jstcache="0"></a> <h2
jstcache="0">Methods</h2> <!-- iterates over all functions --> <div
class="apiItem" jsselect="functions" jstcache="5"> <a jsvalues=".name:'method-'
+ name" jstcache="7"></a> <!-- method-anchor --> <h3 jscontent="name"
jstcache="8">method name</h3> <div class="summary" jstcache="0"><span
jsdisplay="returns" jscontent="returns.typeName" jstcache="10">void</span> <!--
Note: intentionally longer 80 columns --> <span jscontent="fullName"
jstcache="11">chrome.module.methodName</span>(<span jsselect="parameters"
jsvalues="class:optional ? 'optional' : ''" jstcache="12"><span
jsdisplay="$index" jstcache="21">, </span><span jscontent="typeName"
jstcache="22"></span> <var jstcache="0"><span jscontent="name"
jstcache="8"></span></var></span>)</div> <div class="description" jstcache="0">
<p class="todo" jsdisplay="!description" jstcache="13">Undocumented.</p> <p
jsdisplay="description" jsvalues=".innerHTML:description" jstcache="14"> A
description from the json schema def of the function goes here. </p> <!--
PARAMETERS --> <h4 jstcache="0">Parameters</h4> <dl jstcache="0"> <div
jsselect="parameters" jstcache="23"> <dt jstcache="0"> <!-- Note: intentionally
longer 80 columns --> <var jscontent="name" jstcache="8">paramName</var><em
jstcache="0"> (<span class="optional" jsdisplay="optional"
jstcache="31">optional </span><span jscontent="typeName"
jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> <!--
OBJECT PROPERTIES --> <dd jsdisplay="_propertyList" jstcache="28"> <dl
jstcache="0"> <div jsselect="_propertyList" jstcache="32"> <dt jstcache="0">
<!-- Note: intentionally longer 80 columns --> <var jscontent="name"
jstcache="8">paramName</var><em jstcache="0"> (<span class="optional"
jsdisplay="optional" jstcache="31">optional </span><span jscontent="typeName"
jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> </div>
</dl> </dd> </div> </dl> <!-- RETURNS --> <h4 jsdisplay="returns"
jstcache="15">Returns</h4> <dl jstcache="0"> <div jsselect="returns"
jstcache="24"> <dt jstcache="0"> <!-- Note: intentionally longer 80 columns -->
<var jscontent="name" jstcache="8">paramName</var> <em jstcache="0">(<span
jscontent="typeName" jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> <!--
OBJECT PROPERTIES --> <dd jsdisplay="_propertyList" jstcache="28"> <dl
jstcache="0"> <div jsselect="_propertyList" jstcache="32"> <dt jstcache="0">
<!-- Note: intentionally longer 80 columns --> <var jscontent="name"
jstcache="8">paramName</var><em jstcache="0"> (<span class="optional"
jsdisplay="optional" jstcache="31">optional </span><span jscontent="typeName"
jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> </div>
</dl> </dd> </div> </dl> <!-- CALLBACK -->	 <div jsdisplay="callbackParameters"
jstcache="16"> <h4 jstcache="0">Callback function</h4> <p jstcache="0"> If you
specify the <em jstcache="0">callback</em> parameter, it should specify a
function that looks like this: </p> <!-- Note: intentionally longer 80 columns
--> <pre jstcache="0">function(<span jscontent="callbackSignature"
jstcache="29">Type param1, Type param2</span>) <span class="subdued"
jstcache="0">{...}</span>);</pre> <dl jstcache="0"> <div
jsselect="callbackParameters" jstcache="30"> <dt jstcache="0"> <!-- Note:
intentionally longer 80 columns --> <var jscontent="name"
jstcache="8">paramName</var><em jstcache="0"> (<span jscontent="typeName"
jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> <!--
OBJECT PROPERTIES --> <dd jsdisplay="_propertyList" jstcache="28"> <dl
jstcache="0"> <div jsselect="_propertyList" jstcache="32"> <dt jstcache="0">
<!-- Note: intentionally longer 80 columns --> <var jscontent="name"
jstcache="8">paramName</var><em jstcache="0"> (<span class="optional"
jsdisplay="optional" jstcache="31">optional </span><span jscontent="typeName"
jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> </div>
</dl> </dd> </div> </dl> </div> </div> <!-- /description --> </div> <!--
/apiItem --> </div> <!-- /apiGroup --> <!-- EVENTS --> <div class="apiGroup"
id="events" jstcache="0"> <a name="#events" jstcache="0"></a> <h2 id="events"
jstcache="0">Events</h2> <!-- iterates over all events --> <div
jsselect="events" class="apiItem" jstcache="6"> <a jsvalues=".name:'event-' +
name" jstcache="9"></a> <h3 jscontent="name" jstcache="8">event name</h3> <div
class="summary" jstcache="0"> <!-- Note: intentionally longer 80 columns -->
<span class="subdued" jstcache="0">chrome.bookmarks.</span><span
jscontent="name" jstcache="8">onEvent</span><span class="subdued"
jstcache="0">.addListener</span>(function(<span jscontent="callSignature"
jstcache="17">Type param1, Type param2</span>) <span class="subdued"
jstcache="0">{...}</span>); </div> <div class="description" jstcache="0"> <p
class="todo" jsdisplay="!description" jstcache="13">Undocumented.</p> <p
jsdisplay="description" jsvalues=".innerHTML:description" jstcache="14"> A
description from the json schema def of the event goes here. </p> <!--
PARAMETERS --> <h4 jstcache="0">Parameters</h4> <dl jstcache="0"> <div
jsselect="parameters" jstcache="23"> <dt jstcache="0"> <!-- Note: intentionally
longer 80 columns --> <var jscontent="name" jstcache="8">paramName</var><em
jstcache="0"> (<span jscontent="typeName" jstcache="22">paramType</span>)</em>
</dt> <dd class="todo" jsdisplay="!$this.description"
jstcache="26">Undocumented.</dd> <dd jsdisplay="$this.description"
jsvalues=".innerHTML:$this.description" jstcache="27"> Description of this
parameter from the json schema. </dd> <!-- OBJECT PROPERTIES --> <dd
jsdisplay="_propertyList" jstcache="28"> <dl jstcache="0"> <div
jsselect="_propertyList" jstcache="32"> <dt jstcache="0"> <!-- Note:
intentionally longer 80 columns --> <var jscontent="name"
jstcache="8">paramName</var><em jstcache="0"> (<span class="optional"
jsdisplay="optional" jstcache="31">optional </span><span jscontent="typeName"
jstcache="22">paramType</span>)</em> </dt> <dd class="todo"
jsdisplay="!$this.description" jstcache="26"> Undocumented. </dd> <dd
jsdisplay="$this.description" jsvalues=".innerHTML:$this.description"
jstcache="27"> Description of this parameter from the json schema. </dd> </div>
</dl> </dd> </div> </dl> </div> <!-- /decription --> </div> <!-- /apiItem -->
</div> <!-- /apiGroup --> </div> <!-- /apiPage --> </div> <!-- /mainColumn -->
</div> <!-- /pageContent --> <div id="pageFooter" --="" jstcache="0"> Copyright
2009 <br jstcache="0"> TBD: copyright/license should be automatically included
here </div> <!-- /pageFooter --> </div> <!-- /container --> </body></html>
Whoa. It's probably worth putting some newlines and basic formatting in here if
possible. You never know when you're going to want to debug it.

http://codereview.chromium.org/159607/diff/1022/38
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/159607/diff/1022/38#newcode101
Line 101: print "Failed to find test_shell executable in: \n" +
"\n".join(search_locations)
line length - multiple places

http://codereview.chromium.org/159607/diff/1022/44
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/159607/diff/1022/44#newcode167
Line 167: return s.serializeToString(document);
Oh, that worked. Great!

Doesn't look like there's anyway to get prettier formatting. Oh well.

http://codereview.chromium.org/159607/diff/1022/45
File chrome/common/extensions/docs/js/boostrap.js (right):

http://codereview.chromium.org/159607/diff/1022/45#newcode4
Line 4: // from the file:/// scheme.
Why are you only doing this for the file:// scheme? It seems like it would be
useful to always do the regeneration? That way you always for sure browse the
very latest.

asargent_no_longer_on_chrome

http://codereview.chromium.org/159607/diff/1022/35 File chrome/common/extensions/PRESUBMIT.PY (right): http://codereview.chromium.org/159607/diff/1022/35#newcode1 Line 1: import os missing copyright http://codereview.chromium.org/159607/diff/1022/35#newcode21 Line 21: def ...

11 years, 4 months ago (2009-07-31 01:14:05 UTC) #2

rafaelw

Aaron & Antony jumped the gun, but this CL is now ready for review. (I've ...

11 years, 4 months ago (2009-07-31 03:00:30 UTC) #3

Aaron & Antony jumped the gun, but this CL is now ready for review. (I've
addressed aa & asargent's comments with the current state).

sgk, please review new gyp target.

dannyb, please review addition of simplejson to thirdparty. there's already a
copy in trunk/tools/ which claims it's MIT license. I just unpacked the latest
from the web and included it here. let me know if this is ok or if i need to do
anything else.

http://codereview.chromium.org/159607/diff/1022/35
File chrome/common/extensions/PRESUBMIT.PY (right):

http://codereview.chromium.org/159607/diff/1022/35#newcode1
Line 1: import os
On 2009/07/31 01:14:06, Antony Sargent wrote:
> missing copyright

Done.

http://codereview.chromium.org/159607/diff/1022/35#newcode21
Line 21: def CheckChange(input_api, output_api, warn=True):
On 2009/07/31 01:14:06, Antony Sargent wrote:
> Looks like you aren't using the warn parameter in the body of this method - do
> you need it?
> 

Nope. thanks.

http://codereview.chromium.org/159607/diff/1022/38
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/159607/diff/1022/38#newcode42
Line 42: _script_path = os.path.realpath(__file__)
On 2009/07/31 01:14:06, Antony Sargent wrote:
> Looks like this makes line 40 unnecessary?

Done.

http://codereview.chromium.org/159607/diff/1022/38#newcode101
Line 101: print "Failed to find test_shell executable in: \n" +
"\n".join(search_locations)
On 2009/07/30 23:14:45, Aaron Boodman wrote:
> line length - multiple places

Done.

http://codereview.chromium.org/159607/diff/1022/38#newcode111
Line 111: static_names = set(map(rootname, os.listdir(_static_dir)))
On 2009/07/31 01:14:06, Antony Sargent wrote:
> You could avoid needing the "rootname" function with a list comprehension, eg.
> 
> files = os.listdir(_static_dir)
> static_names = set([os.path.splitext(x)[0] for x in files])
> 

Done.

http://codereview.chromium.org/159607/diff/1022/38#newcode119
Line 119: module_names = set(map(modname, extension_api))
On 2009/07/31 01:14:06, Antony Sargent wrote:
> same thing here

Done.

http://codereview.chromium.org/159607/diff/1022/44
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/159607/diff/1022/44#newcode167
Line 167: return s.serializeToString(document);
On 2009/07/30 23:14:45, Aaron Boodman wrote:
> Oh, that worked. Great!
> 
> Doesn't look like there's anyway to get prettier formatting. Oh well.

Right. I briefly considered using some html library in python for this reason,
but it didn't seem worth it.

http://codereview.chromium.org/159607/diff/1022/45
File chrome/common/extensions/docs/js/boostrap.js (right):

http://codereview.chromium.org/159607/diff/1022/45#newcode4
Line 4: // from the file:/// scheme.
On 2009/07/30 23:14:45, Aaron Boodman wrote:
> Why are you only doing this for the file:// scheme? It seems like it would be
> useful to always do the regeneration? That way you always for sure browse the
> very latest.

Done.

sgk

Needs refactoring between the .gyp file and build.py. There's too much hard-coded information that would ...

11 years, 4 months ago (2009-07-31 16:40:58 UTC) #4

Needs refactoring between the .gyp file and build.py.  There's too much
hard-coded information that would need to be kept in sync between the two and
which would break when people (inevitably) fail to do so.

In general, the .gyp file should contain the knowledge about the overall
structure of the project (where the extensions actually live, where the
test_shell can be found) and pass that info in to build.py.  build.py should
then be responsible and finding/passing back the "internal" knowledge about the
actual input and output files it uses/generates.

Come see me if you want to talk over the details FTF.

http://codereview.chromium.org/159607/diff/130/1101
File chrome/chrome.gyp (right):

http://codereview.chromium.org/159607/diff/130/1101#newcode4294
Line 4294: 'action': [ 'python', 'common/extensions/docs/build/build.py'],
The fact that you list the hard-coded files in 'inputs' and 'outputs' above but
don't have a way to either pass them in (or pass them back) is going to make it
hard to keep in sync.  People will, for example, add a new input file to static/
and it will "work" because the build.py script will find it, but they'll forget
to add it to the list here and the dependencies will be wrong.

We really want to have the authoritative lists be generated from a single source
of input.  Here, you want build.py to pick up automatically the new files that
people add, which makes live easy for developers.  So the best technique would
probably be to give build.py options that have it return lists of input and
output files that we can then interpolate in the .gyp file as follows:

'inputs': [ '<!@(python common/extensions/docs/build/build.py --list-inputs)'],
'outputs': ['<!@(python common/extensions/docs/build/build.py --list-outputs)'],

This would keep all the logic in build.py.

http://codereview.chromium.org/159607/diff/130/135
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/159607/diff/130/135#newcode1
Line 1: #!/usr/bin/python2.4
Chromium preference is to use:

#!/usr/bin/python

The code should be written so it will run under Python 2.4, but outside
developers should still be able to build if they only have 2.[56] installed.

http://codereview.chromium.org/159607/diff/130/135#newcode2
Line 2: # Copyright 2009, Google Inc.
Unless the extensions are being treated special, this is the wrong copyright
notice.  DannyB is the authority, of course, but it should probably be (see
almost any other source file):

# Copyright (c) 2009 The Chromium Authors.  All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

http://codereview.chromium.org/159607/diff/130/135#newcode44
Line 44: _static_dir = os.path.normpath(_base_dir + "/static")
os.path.normpath() is unnecessary here (and on the next line) since _base_dir
has already been normalized.

http://codereview.chromium.org/159607/diff/130/135#newcode47
Line 47: _chrome_dir = os.path.normpath(_base_dir + "/../../../")
Hard-coding knowledge of how deep the script lives in the tree is fragile. 
Since you're only using this to set _src_dir and then find the test_shell, let
the .gyp file pass in <(PRODUCT_DIR) so this script doesn't have to care about
where it actually lives.

http://codereview.chromium.org/159607/diff/130/135#newcode86
Line 86: fd = open(output_file, 'w')
More compact and readable, all three lines can be replaced by:

open(output_file, 'w').write(result)

http://codereview.chromium.org/159607/diff/130/135#newcode95
Line 95: search_locations.append(_chrome_dir + "/Release/test_shell.exe")
This is better done by having the gyp file pass in the location of the
test_shell for the generated configuration.  If someone generates the file in a
different configuration output directory (which they can do), this search won't
find it.  Also, the test_shell should be one of the inputs in the .gyp file,
since presumably a changed test_shell could affect the output and therefore
require the documentation to be re-generated.  This can probably best be done by
passing in '<(PRODUCT_DIR)' as an option to build.py:

'action': 'python --product_dir=<(PRODUCT_DIR) ...'

http://codereview.chromium.org/159607/diff/130/135#newcode109
Line 109: pass
Remove left-over pass statement.

http://codereview.chromium.org/159607/diff/130/135#newcode128
Line 128: fd = open(_extension_api_json, 'r')
Again, more compact (readability):

contents = open(_extension_api_json, 'r').read()
extension_api = json.loads(contents, encoding="ASCII")

Daniel Berlin

LGTM for the simplejson stuff. On Thu, Jul 30, 2009 at 8:00 PM, <rafaelw@chromium.org> wrote: ...

11 years, 4 months ago (2009-07-31 17:29:37 UTC) #5

LGTM for the simplejson stuff.

On Thu, Jul 30, 2009 at 8:00 PM, <rafaelw@chromium.org> wrote:
> Reviewers: Aaron Boodman, sgk, Antony Sargent, Daniel Berlin,
>
> Message:
> Aaron & Antony jumped the gun, but this CL is now ready for review.
> (I've addressed aa & asargent's comments with the current state).
>
> sgk, please review new gyp target.
>
> dannyb, please review addition of simplejson to thirdparty. there's
> already a copy in trunk/tools/ which claims it's MIT license. I just
> unpacked the latest from the web and included it here. let me know if
> this is ok or if i need to do anything else.
>
>
> http://codereview.chromium.org/159607/diff/1022/35
> File chrome/common/extensions/PRESUBMIT.PY (right):
>
> http://codereview.chromium.org/159607/diff/1022/35#newcode1
> Line 1: import os
> On 2009/07/31 01:14:06, Antony Sargent wrote:
>>
>> missing copyright
>
> Done.
>
> http://codereview.chromium.org/159607/diff/1022/35#newcode21
> Line 21: def CheckChange(input_api, output_api, warn=3DTrue):
> On 2009/07/31 01:14:06, Antony Sargent wrote:
>>
>> Looks like you aren't using the warn parameter in the body of this
>
> method - do
>>
>> you need it?
>
>
> Nope. thanks.
>
> http://codereview.chromium.org/159607/diff/1022/38
> File chrome/common/extensions/docs/build/build.py (right):
>
> http://codereview.chromium.org/159607/diff/1022/38#newcode42
> Line 42: _script_path =3D os.path.realpath(__file__)
> On 2009/07/31 01:14:06, Antony Sargent wrote:
>>
>> Looks like this makes line 40 unnecessary?
>
> Done.
>
> http://codereview.chromium.org/159607/diff/1022/38#newcode101
> Line 101: print "Failed to find test_shell executable in: \n" +
> "\n".join(search_locations)
> On 2009/07/30 23:14:45, Aaron Boodman wrote:
>>
>> line length - multiple places
>
> Done.
>
> http://codereview.chromium.org/159607/diff/1022/38#newcode111
> Line 111: static_names =3D set(map(rootname, os.listdir(_static_dir)))
> On 2009/07/31 01:14:06, Antony Sargent wrote:
>>
>> You could avoid needing the "rootname" function with a list
>
> comprehension, eg.
>
>> files =3D os.listdir(_static_dir)
>> static_names =3D set([os.path.splitext(x)[0] for x in files])
>
>
> Done.
>
> http://codereview.chromium.org/159607/diff/1022/38#newcode119
> Line 119: module_names =3D set(map(modname, extension_api))
> On 2009/07/31 01:14:06, Antony Sargent wrote:
>>
>> same thing here
>
> Done.
>
> http://codereview.chromium.org/159607/diff/1022/44
> File chrome/common/extensions/docs/js/api_page_generator.js (right):
>
> http://codereview.chromium.org/159607/diff/1022/44#newcode167
> Line 167: return s.serializeToString(document);
> On 2009/07/30 23:14:45, Aaron Boodman wrote:
>>
>> Oh, that worked. Great!
>
>> Doesn't look like there's anyway to get prettier formatting. Oh well.
>
> Right. I briefly considered using some html library in python for this
> reason, but it didn't seem worth it.
>
> http://codereview.chromium.org/159607/diff/1022/45
> File chrome/common/extensions/docs/js/boostrap.js (right):
>
> http://codereview.chromium.org/159607/diff/1022/45#newcode4
> Line 4: // from the file:/// scheme.
> On 2009/07/30 23:14:45, Aaron Boodman wrote:
>>
>> Why are you only doing this for the file:// scheme? It seems like it
>
> would be
>>
>> useful to always do the regeneration? That way you always for sure
>
> browse the
>>
>> very latest.
>
> Done.
>
> Description:
> Extension docs build script, gyp target and PRESUBMIT.PY check
>
> This addeds a mechanism for extension docs to "render" from the
> api/extension_api.json api "idl", the docs/static file content and the
> template/ html templates into the static extension docs.
>
> It retains the property that extension docs loaded in chrome with
> file:/// scheme will regenerate if dependent data has changed, but
> otherwise is static.
>
> The build.py script runs test_shell to render the static files.
>
> A extension_docs gyp target is defined with appropriate inputs & outputs
> that will run the build.py
>
> A PRESUBMIT.PY check will warn on upload and commit if any files have
> changed that would require a docs rebuild.
>
> Note that the PRESUBMIT.PY checks are not currently being run for git cl
> upload or git cl dcommit.
>
>
> Please review this at http://codereview.chromium.org/159607
>
> Affected files:
> =A0M chrome/chrome.gyp
> =A0A chrome/common/extensions/PRESUBMIT.PY
> =A0M chrome/common/extensions/api/extension_api.json
> =A0M chrome/common/extensions/docs/api_index.html
> =A0M chrome/common/extensions/docs/bookmarks.html
> =A0A chrome/common/extensions/docs/build/build.py
> =A0A chrome/common/extensions/docs/build/generator.html
> =A0M chrome/common/extensions/docs/devguide.html
> =A0M chrome/common/extensions/docs/getstarted.html
> =A0M chrome/common/extensions/docs/index.html
> =A0M chrome/common/extensions/docs/js/api_page_generator.js
> =A0A chrome/common/extensions/docs/js/bootstrap.js
> =A0M chrome/common/extensions/docs/overview.html
> =A0M chrome/common/extensions/docs/pageActions.html
> =A0M chrome/common/extensions/docs/self.html
> =A0M chrome/common/extensions/docs/tabs.html
> =A0M chrome/common/extensions/docs/template/page_shell.html
> =A0M chrome/common/extensions/docs/windows.html
> =A0A third_party/python_24/Lib/site-packages/simplejson.pth
> =A0A third_party/simplejson/LICENSE.txt
> =A0A third_party/simplejson/PKG-INFO
> =A0A third_party/simplejson/conf.py
> =A0A third_party/simplejson/docs/_sources/index.txt
> =A0A third_party/simplejson/docs/_static/contents.png
> =A0A third_party/simplejson/docs/_static/default.css
> =A0A third_party/simplejson/docs/_static/doctools.js
> =A0A third_party/simplejson/docs/_static/file.png
> =A0A third_party/simplejson/docs/_static/interface.js
> =A0A third_party/simplejson/docs/_static/jquery.js
> =A0A third_party/simplejson/docs/_static/minus.png
> =A0A third_party/simplejson/docs/_static/navigation.png
> =A0A third_party/simplejson/docs/_static/plus.png
> =A0A third_party/simplejson/docs/_static/pygments.css
> =A0A third_party/simplejson/docs/_static/rightsidebar.css
> =A0A third_party/simplejson/docs/_static/searchtools.js
> =A0A third_party/simplejson/docs/_static/sphinxdoc.css
> =A0A third_party/simplejson/docs/_static/stickysidebar.css
> =A0A third_party/simplejson/docs/_static/traditional.css
> =A0A third_party/simplejson/docs/genindex.html
> =A0A third_party/simplejson/docs/index.html
> =A0A third_party/simplejson/docs/search.html
> =A0A third_party/simplejson/docs/searchindex.json
> =A0A third_party/simplejson/ez_setup.py
> =A0A third_party/simplejson/index.rst
> =A0A third_party/simplejson/scripts/make_docs.py
> =A0A third_party/simplejson/setup.cfg
> =A0A third_party/simplejson/setup.py
> =A0A third_party/simplejson/simplejson.egg-info/PKG-INFO
> =A0A third_party/simplejson/simplejson.egg-info/SOURCES.txt
> =A0A third_party/simplejson/simplejson.egg-info/dependency_links.txt
> =A0A third_party/simplejson/simplejson.egg-info/top_level.txt
> =A0A third_party/simplejson/simplejson.egg-info/zip-safe
> =A0A third_party/simplejson/simplejson/__init__.py
> =A0A third_party/simplejson/simplejson/_speedups.c
> =A0A third_party/simplejson/simplejson/decoder.py
> =A0A third_party/simplejson/simplejson/encoder.py
> =A0A third_party/simplejson/simplejson/scanner.py
> =A0A third_party/simplejson/simplejson/tests/__init__.py
> =A0A third_party/simplejson/simplejson/tests/test_check_circular.py
> =A0A third_party/simplejson/simplejson/tests/test_decode.py
> =A0A third_party/simplejson/simplejson/tests/test_default.py
> =A0A third_party/simplejson/simplejson/tests/test_dump.py
> =A0A third_party/simplejson/simplejson/tests/test_encode_basestring_ascii=
.py
> =A0A third_party/simplejson/simplejson/tests/test_fail.py
> =A0A third_party/simplejson/simplejson/tests/test_float.py
> =A0A third_party/simplejson/simplejson/tests/test_indent.py
> =A0A third_party/simplejson/simplejson/tests/test_pass1.py
> =A0A third_party/simplejson/simplejson/tests/test_pass2.py
> =A0A third_party/simplejson/simplejson/tests/test_pass3.py
> =A0A third_party/simplejson/simplejson/tests/test_recursion.py
> =A0A third_party/simplejson/simplejson/tests/test_scanstring.py
> =A0A third_party/simplejson/simplejson/tests/test_separators.py
> =A0A third_party/simplejson/simplejson/tests/test_unicode.py
> =A0A third_party/simplejson/simplejson/tool.py
>
>
>

rafaelw

Ok. Hopefully, I've addressed all of sgk's comments with this revision. asargent, do I have ...

11 years, 4 months ago (2009-07-31 22:50:58 UTC) #6

Ok. Hopefully, I've addressed all of sgk's comments with this revision.

asargent, do I have an LGTM from you?

http://codereview.chromium.org/159607/diff/130/1101
File chrome/chrome.gyp (right):

http://codereview.chromium.org/159607/diff/130/1101#newcode4294
Line 4294: 'action': [ 'python', 'common/extensions/docs/build/build.py'],
On 2009/07/31 16:40:58, sgk wrote:
> The fact that you list the hard-coded files in 'inputs' and 'outputs' above
but
> don't have a way to either pass them in (or pass them back) is going to make
it
> hard to keep in sync.  People will, for example, add a new input file to
static/
> and it will "work" because the build.py script will find it, but they'll
forget
> to add it to the list here and the dependencies will be wrong.
> 
> We really want to have the authoritative lists be generated from a single
source
> of input.  Here, you want build.py to pick up automatically the new files that
> people add, which makes live easy for developers.  So the best technique would
> probably be to give build.py options that have it return lists of input and
> output files that we can then interpolate in the .gyp file as follows:
> 
> 'inputs': [ '<!@(python common/extensions/docs/build/build.py
--list-inputs)'],
> 'outputs': ['<!@(python common/extensions/docs/build/build.py
--list-outputs)'],
> 
> This would keep all the logic in build.py.

Done.

http://codereview.chromium.org/159607/diff/130/135
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/159607/diff/130/135#newcode1
Line 1: #!/usr/bin/python2.4
On 2009/07/31 16:40:58, sgk wrote:
> Chromium preference is to use:
> 
> #!/usr/bin/python
> 
> The code should be written so it will run under Python 2.4, but outside
> developers should still be able to build if they only have 2.[56] installed.

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode2
Line 2: # Copyright 2009, Google Inc.
On 2009/07/31 16:40:58, sgk wrote:
> Unless the extensions are being treated special, this is the wrong copyright
> notice.  DannyB is the authority, of course, but it should probably be (see
> almost any other source file):
> 
> # Copyright (c) 2009 The Chromium Authors.  All rights reserved.
> # Use of this source code is governed by a BSD-style license that can be
> # found in the LICENSE file.
> 

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode44
Line 44: _static_dir = os.path.normpath(_base_dir + "/static")
On 2009/07/31 16:40:58, sgk wrote:
> os.path.normpath() is unnecessary here (and on the next line) since _base_dir
> has already been normalized.

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode47
Line 47: _chrome_dir = os.path.normpath(_base_dir + "/../../../")
On 2009/07/31 16:40:58, sgk wrote:
> Hard-coding knowledge of how deep the script lives in the tree is fragile. 
> Since you're only using this to set _src_dir and then find the test_shell, let
> the .gyp file pass in <(PRODUCT_DIR) so this script doesn't have to care about
> where it actually lives.

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode86
Line 86: fd = open(output_file, 'w')
On 2009/07/31 16:40:58, sgk wrote:
> More compact and readable, all three lines can be replaced by:
> 
> open(output_file, 'w').write(result)

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode95
Line 95: search_locations.append(_chrome_dir + "/Release/test_shell.exe")
On 2009/07/31 16:40:58, sgk wrote:
> This is better done by having the gyp file pass in the location of the
> test_shell for the generated configuration.  If someone generates the file in
a
> different configuration output directory (which they can do), this search
won't
> find it.  Also, the test_shell should be one of the inputs in the .gyp file,
> since presumably a changed test_shell could affect the output and therefore
> require the documentation to be re-generated.  This can probably best be done
by
> passing in '<(PRODUCT_DIR)' as an option to build.py:
> 
> 'action': 'python --product_dir=<(PRODUCT_DIR) ...'

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode109
Line 109: pass
On 2009/07/31 16:40:58, sgk wrote:
> Remove left-over pass statement.

Done.

http://codereview.chromium.org/159607/diff/130/135#newcode128
Line 128: fd = open(_extension_api_json, 'r')
On 2009/07/31 16:40:58, sgk wrote:
> Again, more compact (readability):
> 
> contents = open(_extension_api_json, 'r').read()
> extension_api = json.loads(contents, encoding="ASCII")

Done.

sgk

lgtm http://codereview.chromium.org/159607/diff/1178/212 File chrome/common/extensions/docs/build/build.py (right): http://codereview.chromium.org/159607/diff/1178/212#newcode114 Line 114: input_files += [GetRelativePath(_js_dir + "/" + x) ...

11 years, 4 months ago (2009-08-01 00:12:17 UTC) #7

asargent_no_longer_on_chrome

LGTM. FYI, there's nothing special about 'x' in list comprehensions; you can make up any ...

11 years, 4 months ago (2009-08-01 00:31:43 UTC) #8

Crap, that should never have got in.

Expand Messages | Collapse Messages