Change RequestFinishedListener to provide executor

Change RequestFinishedListener to provide executor

RequestFinishedListener originally depended on NetworkQualityEstimator's
executor. These are separate features which should not depend on each
other, and as of recently the NQE listeners also provide their own
executors.

Also move RequestFinishedListener out of CronetEngine into its own file,
and move its tests into their own file.

BUG=618034
Committed: https://crrev.com/5387379ab19b85f9e2335d6e931d63116f449d66
Cr-Commit-Position: refs/heads/master@{#408711}

[mgersh, 2016-07-25 19:02:20]

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

[mgersh, 2016-07-25 19:02:20]

Helen, PTAL. The main purpose of this is adding an executor to
RequestFinishedListener, but I also cleaned up the rest of the old NQE API and
moved some things around while I was doing it.

I talked to Misha and he said he's fine with doing this update all at once
instead of leaving the old API since there aren't a lot of callers.

[xunjieli, 2016-07-26 17:11:24]

On 2016/07/25 19:02:20, mgersh wrote:
> Helen, PTAL. The main purpose of this is adding an executor to
> RequestFinishedListener, but I also cleaned up the rest of the old NQE API and
> moved some things around while I was doing it.
> 
> I talked to Misha and he said he's fine with doing this update all at once
> instead of leaving the old API since there aren't a lot of callers.

Could you do a separate CL to remove the old NQE API and add tbansal@ as a
reviewer too? We should let him know that you are removing it since the current
bug (crbug.com/618034) is assigned to him.
It is a bit hard to review both the RequestFinishedListener change as well as
the NQE API removal in one CL.

[mgersh, 2016-07-26 20:13:06]

Okay, done. This CL is just changing and moving RequestFinishedListener now.

[tbansal1, 2016-07-26 20:37:26]

tbansal@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2016-07-26 20:37:27]

fwiw, lgtm.

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:10:
* Interface to listen for finished requests for the purpose of collecting
metrics.
s/Interface/Something_Else/

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:72:
private final Object mFinishedListenerLock = new Object();
May be add a separate comment block for this.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java
(right):

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:21:
* Test RequestFinishedListener and the metrics information it provides
Missing period at the end.

[tbansal1, 2016-07-26 20:37:50]

On 2016/07/26 20:37:27, tbansal1 wrote:
> fwiw, lgtm.
> 
>
https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
> File
>
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java
> (right):
> 
>
https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
>
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:10:
> * Interface to listen for finished requests for the purpose of collecting
> metrics.
> s/Interface/Something_Else/
> 
>
https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
> File
>
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
> (right):
> 
>
https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
>
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:72:
> private final Object mFinishedListenerLock = new Object();
> May be add a separate comment block for this.
> 
>
https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
> File
>
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java
> (right):
> 
>
https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
>
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:21:
> * Test RequestFinishedListener and the metrics information it provides
> Missing period at the end.

And, thank you for doing this.

[xunjieli, 2016-07-26 21:11:11]

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:25:
* Invoked with request info.
Can you add a comment to say that is method will be called on the executor's
thread?

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:30:
public Executor getExecutor() {
Why does this need to be public?
Can you also add a comment to say that this method can be called on any thread?

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:495:
* Called before following redirects. The redirect will only be followed if
followRedirect() is
This line seems a bit too long compare to the rest. could you do a line wrap?
Also use {@link } for followRedirect.

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:72:
private final Object mFinishedListenerLock = new Object();
Can you add a comment here?

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:530:
postObservationTaskToExecutor(listener.getExecutor(), task);
We shouldn't call into the executor, which might happen to be a direct executor.
The caller might spend considerable time doing things and we don't want to block
other requests which are trying to acquire this listener lock.

Can you make a copy of the list of listeners, and do the rest out of the
synchronized block?

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java
(right):

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:54:
} catch (NoSuchElementException e) {
Maybe rethrow this exception as a runtime exception.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:60:
private UrlRequestInfo mRequestInfo = null;
nit: omit initialization to null and rely on default initialization.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:91:
testExecutor.runAllTasks();
Maybe add a test where the executor runs the task directly, and one test where
the executor runs the task on a different thread than the test thread.

[mgersh, 2016-07-27 20:54:13]

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:10:
* Interface to listen for finished requests for the purpose of collecting
metrics.
On 2016/07/26 20:37:26, tbansal1 wrote:
> s/Interface/Something_Else/

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:25:
* Invoked with request info.
On 2016/07/26 21:11:08, xunjieli wrote:
> Can you add a comment to say that is method will be called on the executor's
> thread?

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/RequestFinishedListener.java:30:
public Executor getExecutor() {
On 2016/07/26 21:11:08, xunjieli wrote:
> Why does this need to be public?
> Can you also add a comment to say that this method can be called on any
thread?

Done. It needs to be public because this is in the API package and it's called
in the impl package, which are now separate.

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:495:
* Called before following redirects. The redirect will only be followed if
followRedirect() is
On 2016/07/26 21:11:08, xunjieli wrote:
> This line seems a bit too long compare to the rest. could you do a line wrap?
> Also use {@link } for followRedirect.

Done.

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:72:
private final Object mFinishedListenerLock = new Object();
On 2016/07/26 20:37:26, tbansal1 wrote:
> May be add a separate comment block for this.

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:72:
private final Object mFinishedListenerLock = new Object();
On 2016/07/26 21:11:09, xunjieli wrote:
> Can you add a comment here?

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:530:
postObservationTaskToExecutor(listener.getExecutor(), task);
On 2016/07/26 21:11:09, xunjieli wrote:
> We shouldn't call into the executor, which might happen to be a direct
executor.
> The caller might spend considerable time doing things and we don't want to
block
> other requests which are trying to acquire this listener lock.
> 
> Can you make a copy of the list of listeners, and do the rest out of the
> synchronized block?

Done. I also changed mFinishedListenerList from an ObserverList to a normal
ArrayList, since if we're making a copy we're not using the ObserverList
functionality at all, and I don't think we need it.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java
(right):

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:21:
* Test RequestFinishedListener and the metrics information it provides
On 2016/07/26 20:37:27, tbansal1 wrote:
> Missing period at the end.

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:54:
} catch (NoSuchElementException e) {
On 2016/07/26 21:11:11, xunjieli wrote:
> Maybe rethrow this exception as a runtime exception. 

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:60:
private UrlRequestInfo mRequestInfo = null;
On 2016/07/26 21:11:09, xunjieli wrote:
> nit: omit initialization to null and rely on default initialization.

Done.

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedListenerTest.java:91:
testExecutor.runAllTasks();
On 2016/07/26 21:11:10, xunjieli wrote:
> Maybe add a test where the executor runs the task directly, and one test where
> the executor runs the task on a different thread than the test thread.

Done.

[xunjieli, 2016-07-29 16:35:25]

Looks great! LGTM

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

https://codereview.chromium.org/2178053002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:530:
postObservationTaskToExecutor(listener.getExecutor(), task);
On 2016/07/27 20:54:12, mgersh wrote:
> On 2016/07/26 21:11:09, xunjieli wrote:
> > We shouldn't call into the executor, which might happen to be a direct
> executor.
> > The caller might spend considerable time doing things and we don't want to
> block
> > other requests which are trying to acquire this listener lock.
> > 
> > Can you make a copy of the list of listeners, and do the rest out of the
> > synchronized block?
> 
> Done. I also changed mFinishedListenerList from an ObserverList to a normal
> ArrayList, since if we're making a copy we're not using the ObserverList
> functionality at all, and I don't think we need it.

Acknowledged. I agree with you.

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

https://codereview.chromium.org/2178053002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:502:
* @param headers an array of response headers with keys at the even indices
followed by the
Why is this line changed? Could you revert?

https://codereview.chromium.org/2178053002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2178053002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:74:
/** Locks operations on the list of RequestFinishedListeners, because operations
can happen on
The previous example is a bad example :( 
The first line needs be to moved one line down.
/**
 * first line...
 * second line.
 */

[mgersh, 2016-07-29 18:21:27]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2016-07-29 18:21:27]

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

[commit-bot: I haz the power, 2016-07-29 18:21:48]

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

[mgersh, 2016-07-29 18:21:52]

Thanks! I also rebased the CL that depends on this one.

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

https://codereview.chromium.org/2178053002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:502:
* @param headers an array of response headers with keys at the even indices
followed by the
On 2016/07/29 16:35:24, xunjieli wrote:
> Why is this line changed? Could you revert?

Done.

https://codereview.chromium.org/2178053002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2178053002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:74:
/** Locks operations on the list of RequestFinishedListeners, because operations
can happen on
On 2016/07/29 16:35:25, xunjieli wrote:
> The previous example is a bad example :( 
> The first line needs be to moved one line down.
> /**
>  * first line...
>  * second line.
>  */

Done.

[commit-bot: I haz the power, 2016-07-29 19:17:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 19:20:16]

Description was changed from

==========
Change RequestFinishedListener to provide executor

RequestFinishedListener originally depended on NetworkQualityEstimator's
executor. These are separate features which should not depend on each
other, and as of recently the NQE listeners also provide their own
executors.

Also move RequestFinishedListener out of CronetEngine into its own file,
and move its tests into their own file.

BUG=618034
==========

to

==========
Change RequestFinishedListener to provide executor

RequestFinishedListener originally depended on NetworkQualityEstimator's
executor. These are separate features which should not depend on each
other, and as of recently the NQE listeners also provide their own
executors.

Also move RequestFinishedListener out of CronetEngine into its own file,
and move its tests into their own file.

BUG=618034

Committed: https://crrev.com/5387379ab19b85f9e2335d6e931d63116f449d66
Cr-Commit-Position: refs/heads/master@{#408711}
==========

[commit-bot: I haz the power, 2016-07-29 19:20:18]

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