Move Cronet metrics-related classes into their own files

Move Cronet metrics-related classes into their own files

UrlRequestInfo becomes RequestFinishedInfo. Both RequestFinishedInfo and
UrlRequestMetrics move out of CronetEngine. This only moves code and
does not change any behavior, although existing embedders will need to
update.

BUG=629194
Committed: https://crrev.com/c53f6e90718d72da77e8c2f9804cd53b2c511a8f
Cr-Commit-Position: refs/heads/master@{#409201}

[mgersh, 2016-08-01 20:15:18]

mgersh@chromium.org changed reviewers:
+ xunjieli@chromium.org

[mgersh, 2016-08-01 20:15:18]

PTAL

[xunjieli, 2016-08-01 20:40:05]

https://codereview.chromium.org/2204533002/diff/1/components/cronet/android/a...
File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java
(right):

https://codereview.chromium.org/2204533002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:16:
public final class RequestFinishedInfo {
I like the new name! What do you think about moving RequestFinishedListener here
as a nested class? Maybe we can do something like RequestFinishedInfo.Listener?

If UrlRequestMetrics is only used in RequestFinishedInfo, can we also nest it
here? and call it metrics?

[mgersh, 2016-08-01 21:45:19]

Rearranging done!

[xunjieli, 2016-08-01 22:14:11]

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:840: *
@param requestAnnotations Objects to pass on to {@link
RequestFinishedInfo#Listener}.
A period should be used instead of a hashtag in front of a class name.

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
(right):

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:698:
RequestFinishedInfo getRequestInfo() {
getRequestInfo() -> getRequestFinishedInfo()?

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:702:
mResponseInfo);
not in this CL, but why do we make the RequestFinishedInfo have a reference to
mResponseInfo? Shouldn't they be separate?

[mgersh, 2016-08-01 22:33:45]

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:840: *
@param requestAnnotations Objects to pass on to {@link
RequestFinishedInfo#Listener}.
On 2016/08/01 22:14:11, xunjieli wrote:
> A period should be used instead of a hashtag in front of a class name.

Done.

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
(right):

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:698:
RequestFinishedInfo getRequestInfo() {
On 2016/08/01 22:14:11, xunjieli wrote:
> getRequestInfo() -> getRequestFinishedInfo()?

Done.

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:702:
mResponseInfo);
On 2016/08/01 22:14:11, xunjieli wrote:
> not in this CL, but why do we make the RequestFinishedInfo have a reference to
> mResponseInfo? Shouldn't they be separate?

It makes it easy to access the information in UrlResponseInfo from inside a
RequestFinishedInfo.Listener callback. Without it, the embedder would get both
pieces of information but they would be in separate places and harder to link
and process together. All the embedders that currently use
RequestFinishedListener use this.

[xunjieli, 2016-08-01 22:42:49]

I wonder why the JavaDoc generation step didn't pick up the hashtag problem in
the previous patch. Could you double check the generated JavaDoc is okay? LGTM.

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
(right):

https://codereview.chromium.org/2204533002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:702:
mResponseInfo);
On 2016/08/01 22:33:45, mgersh wrote:
> On 2016/08/01 22:14:11, xunjieli wrote:
> > not in this CL, but why do we make the RequestFinishedInfo have a reference
to
> > mResponseInfo? Shouldn't they be separate?
> 
> It makes it easy to access the information in UrlResponseInfo from inside a
> RequestFinishedInfo.Listener callback. Without it, the embedder would get both
> pieces of information but they would be in separate places and harder to link
> and process together. All the embedders that currently use
> RequestFinishedListener use this.

Acknowledged. You are right. I missed the fact that this is obtained on the
CronetEngine level, and consumer doesn't know the UrlRequest that this info is
attached to.

[mgersh, 2016-08-02 14:45:02]

On 2016/08/01 22:42:49, xunjieli wrote:
> I wonder why the JavaDoc generation step didn't pick up the hashtag problem in
> the previous patch. Could you double check the generated JavaDoc is okay?
LGTM.

It didn't catch it because all of this stuff is still hidden. But I went over
the javadoc changes more carefully and fixed a few more things. Thanks!

[mgersh, 2016-08-02 14:45:07]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2016-08-02 14:45:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2204533002/#ps60001
(title: "fix javadoc")

[commit-bot: I haz the power, 2016-08-02 14:45:19]

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, 2016-08-02 15:59:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-02 16:03:19]

Description was changed from

==========
Move Cronet metrics-related classes into their own files

UrlRequestInfo becomes RequestFinishedInfo. Both RequestFinishedInfo and
UrlRequestMetrics move out of CronetEngine. This only moves code and
does not change any behavior, although existing embedders will need to
update.

BUG=629194
==========

to

==========
Move Cronet metrics-related classes into their own files

UrlRequestInfo becomes RequestFinishedInfo. Both RequestFinishedInfo and
UrlRequestMetrics move out of CronetEngine. This only moves code and
does not change any behavior, although existing embedders will need to
update.

BUG=629194

Committed: https://crrev.com/c53f6e90718d72da77e8c2f9804cd53b2c511a8f
Cr-Commit-Position: refs/heads/master@{#409201}
==========

[commit-bot: I haz the power, 2016-08-02 16:03:20]

Patchset 4 (id:??) landed as
https://crrev.com/c53f6e90718d72da77e8c2f9804cd53b2c511a8f
Cr-Commit-Position: refs/heads/master@{#409201}