Issue 1148263004: [Cronet] Generate Javadoc

Issue 1148263004: [Cronet] Generate Javadoc (Closed)

Created:
5 years, 6 months ago by xunjieli

Modified:
5 years, 6 months ago

Reviewers:
mef

CC:
chromium-reviews, cbentzel+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Cronet] Generate Javadoc This CL generate Javadoc as a part of cronet_package. BUG=428935 Committed: https://crrev.com/66a41e6227869ef7f6f89a707cfd810850e0b9f4 Cr-Commit-Position: refs/heads/master@{#333162}

Patch Set 1 : #

Total comments: 12

Patch Set 2 : Address comments #

Total comments: 2

Patch Set 3 : Added a blank line #

Created: 5 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+74 lines, -0 lines)			Patch
M	components/cronet.gypi	View	1	1 chunk	+13 lines, -0 lines	0 comments	Download
A	components/cronet/android/java/build.xml	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	components/cronet/tools/generate_javadoc.py	View	1 2	1 chunk	+43 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

xunjieli

Misha, what do you think about this? We can add overview pages as follow up ...

5 years, 6 months ago (2015-06-02 22:12:36 UTC) #4

mef

nice! https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml File components/cronet/android/java/build.xml (right): https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml#newcode6 components/cronet/android/java/build.xml:6: noindex="yes" Not sure what noindex does, but it ...

5 years, 6 months ago (2015-06-03 18:25:01 UTC) #5

xunjieli

Thanks for the feedback! PTAL. https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml File components/cronet/android/java/build.xml (right): https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml#newcode6 components/cronet/android/java/build.xml:6: noindex="yes" On 2015/06/03 18:25:01, ...

5 years, 6 months ago (2015-06-03 20:19:34 UTC) #6

Thanks for the feedback! PTAL.

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
File components/cronet/android/java/build.xml (right):

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
components/cronet/android/java/build.xml:6: noindex="yes"
On 2015/06/03 18:25:01, mef wrote:
> Not sure what noindex does, but it would make sense to have a TOC or
something.

Done. Removed.

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
components/cronet/android/java/build.xml:11: <include name="**/*.java"/>
On 2015/06/03 18:25:01, mef wrote:
> Should we generate javadoc just for interface files and not implementation?
This
> would reduce noise / clutter.

Looking at the internal code base, our current embedders are using the concrete
ones directly (eg. CronetUrlRequestContext), so I think we should still need to
keep them there.

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
File components/cronet/tools/generate_javadoc.py (right):

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
components/cronet/tools/generate_javadoc.py:26: jar_cmd = ['ant',
'-Dsource.dir=' + source_dir,
On 2015/06/03 18:25:01, mef wrote:
> javadoc_cmd = ...
> 
> Why do you need ant? Can we run javadoc directly?

javadoc is horrendously hard to configure. For instance it doesn't easily allow
file inclusion/exclusion. I battled with it for a while and decided to use ant
task which is much cleaner and easy to configure/customize.

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
components/cronet/tools/generate_javadoc.py:34:
parser.add_option('--javadoc-cwd', help='Working directory')
On 2015/06/03 18:25:01, mef wrote:
> maybe temp-dir or working-dir? 
Done.
> Can it be created dynamically and deleted at the end?
This is the path that the command needs to be run with. It is not a newly
created directly. Sorry for my poor naming.

mef

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml File components/cronet/android/java/build.xml (right): https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml#newcode11 components/cronet/android/java/build.xml:11: <include name="**/*.java"/> On 2015/06/03 20:19:34, xunjieli wrote: > On ...

5 years, 6 months ago (2015-06-03 22:00:54 UTC) #7

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
File components/cronet/android/java/build.xml (right):

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
components/cronet/android/java/build.xml:11: <include name="**/*.java"/>
On 2015/06/03 20:19:34, xunjieli wrote:
> On 2015/06/03 18:25:01, mef wrote:
> > Should we generate javadoc just for interface files and not implementation?
> This
> > would reduce noise / clutter.
> 
> Looking at the internal code base, our current embedders are using the
concrete
> ones directly (eg. CronetUrlRequestContext), so I think we should still need
to
> keep them there.

Sounds good, as long as only 'public' classes get javadoc'ed. Could you confirm
that non-public classes are not excluded?

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
File components/cronet/tools/generate_javadoc.py (right):

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
components/cronet/tools/generate_javadoc.py:26: jar_cmd = ['ant',
'-Dsource.dir=' + source_dir,
On 2015/06/03 20:19:34, xunjieli wrote:
> On 2015/06/03 18:25:01, mef wrote:
> > javadoc_cmd = ...
> > 
> > Why do you need ant? Can we run javadoc directly?
> 
> javadoc is horrendously hard to configure. For instance it doesn't easily
allow
> file inclusion/exclusion. I battled with it for a while and decided to use ant
> task which is much cleaner and easy to configure/customize.

I see. That's unfortunate, but I don't see any examples of running javadoc in
chromium builds, so this could be our best option. :(

https://codereview.chromium.org/1148263004/diff/60001/components/cronet/tools...
File components/cronet/tools/generate_javadoc.py (right):

https://codereview.chromium.org/1148263004/diff/60001/components/cronet/tools...
components/cronet/tools/generate_javadoc.py:29: 
nit: 2 empty lines between methods.

xunjieli

PTAL. Thanks! https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml File components/cronet/android/java/build.xml (right): https://codereview.chromium.org/1148263004/diff/40001/components/cronet/android/java/build.xml#newcode11 components/cronet/android/java/build.xml:11: <include name="**/*.java"/> On 2015/06/03 22:00:54, mef wrote: ...

5 years, 6 months ago (2015-06-04 13:37:35 UTC) #8

PTAL. Thanks!

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
File components/cronet/android/java/build.xml (right):

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/andro...
components/cronet/android/java/build.xml:11: <include name="**/*.java"/>
On 2015/06/03 22:00:54, mef wrote:
> On 2015/06/03 20:19:34, xunjieli wrote:
> > On 2015/06/03 18:25:01, mef wrote:
> > > Should we generate javadoc just for interface files and not
implementation?
> > This
> > > would reduce noise / clutter.
> > 
> > Looking at the internal code base, our current embedders are using the
> concrete
> > ones directly (eg. CronetUrlRequestContext), so I think we should still need
> to
> > keep them there.
> 
> Sounds good, as long as only 'public' classes get javadoc'ed. Could you
confirm
> that non-public classes are not excluded?

Confirmed that non-public classes are not included :)

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
File components/cronet/tools/generate_javadoc.py (right):

https://codereview.chromium.org/1148263004/diff/40001/components/cronet/tools...
components/cronet/tools/generate_javadoc.py:26: jar_cmd = ['ant',
'-Dsource.dir=' + source_dir,
On 2015/06/03 22:00:54, mef wrote:
> On 2015/06/03 20:19:34, xunjieli wrote:
> > On 2015/06/03 18:25:01, mef wrote:
> > > javadoc_cmd = ...
> > > 
> > > Why do you need ant? Can we run javadoc directly?
> > 
> > javadoc is horrendously hard to configure. For instance it doesn't easily
> allow
> > file inclusion/exclusion. I battled with it for a while and decided to use
ant
> > task which is much cleaner and easy to configure/customize.
> 
> I see. That's unfortunate, but I don't see any examples of running javadoc in
> chromium builds, so this could be our best option. :(

Acknowledged.

https://codereview.chromium.org/1148263004/diff/60001/components/cronet/tools...
File components/cronet/tools/generate_javadoc.py (right):

https://codereview.chromium.org/1148263004/diff/60001/components/cronet/tools...
components/cronet/tools/generate_javadoc.py:29: 
On 2015/06/03 22:00:54, mef wrote:
> nit: 2 empty lines between methods.

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1148263004/80001

5 years, 6 months ago (2015-06-05 20:52:16 UTC) #11

commit-bot: I haz the power

5 years, 6 months ago (2015-06-05 22:43:18 UTC) #13

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/66a41e6227869ef7f6f89a707cfd810850e0b9f4
Cr-Commit-Position: refs/heads/master@{#333162}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages