[Offline Pages] Clear remaining requests after test harness finishes.

[Offline Pages] Clear remaining requests after test harness finishes.

Fixed a bug where requests are not removed when test harness finished with
incomplete requests.

BUG=662247
Committed: https://crrev.com/3a50787136f81047838c1ce3a24e38133c7d13cc
Cr-Commit-Position: refs/heads/master@{#430494}

[romax, 2016-11-05 01:28:41]

romax@chromium.org changed reviewers:
+ petewil@chromium.org

[romax, 2016-11-05 01:28:42]

PTAL, thanks!

[romax, 2016-11-05 01:28:52]

The CQ bit was checked by romax@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 01:29:11]

Dry run: 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-11-05 01:59:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 02:00:00]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-11-07 23:45:26]

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:85:
private static final int REMOVE_REQUESTS_TIMEOUT_MS = 5000;
Maybe allow a longer timeout.  A svelte device might be busy, and we don't
really want to fail the test if the timeout happens (or stop trying to clean
up).  30s might be a better timeout than 5s.

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:118:
final Semaphore mClearingSemaphore = new Semaphore(0);
I see where we decrement the count and release the semaphore, where do we
increment the count?

https://codereview.chromium.org/2482623002/diff/1/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:83:
ScopedJavaLocalRef<jobjectArray> CreateJavaSavePageRequests(
I didn't see a call to this.  What does it do, and who calls it?

[romax, 2016-11-08 00:18:36]

Addressed comments and replied to 2 of them, PTAL!
Thanks!

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:85:
private static final int REMOVE_REQUESTS_TIMEOUT_MS = 5000;
On 2016/11/07 23:45:26, Pete Williamson wrote:
> Maybe allow a longer timeout.  A svelte device might be busy, and we don't
> really want to fail the test if the timeout happens (or stop trying to clean
> up).  30s might be a better timeout than 5s.

Done.
I've also updated the timeouts above.

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:118:
final Semaphore mClearingSemaphore = new Semaphore(0);
On 2016/11/07 23:45:26, Pete Williamson wrote:
> I see where we decrement the count and release the semaphore, where do we
> increment the count?

This is used as a way to block the thread until timeout or the callback has been
called. So seems there's no need to increment the count. I might be wrong here
but this is how I understand this usage...

https://codereview.chromium.org/2482623002/diff/1/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:83:
ScopedJavaLocalRef<jobjectArray> CreateJavaSavePageRequests(
On 2016/11/07 23:45:26, Pete Williamson wrote:
> I didn't see a call to this.  What does it do, and who calls it?

This is used in OnGetAllRequestsDone in the same file at line 122.

[romax, 2016-11-08 00:18:36]

Addressed comments and replied to 2 of them, PTAL!
Thanks!

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:85:
private static final int REMOVE_REQUESTS_TIMEOUT_MS = 5000;
On 2016/11/07 23:45:26, Pete Williamson wrote:
> Maybe allow a longer timeout.  A svelte device might be busy, and we don't
> really want to fail the test if the timeout happens (or stop trying to clean
> up).  30s might be a better timeout than 5s.

Done.
I've also updated the timeouts above.

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:118:
final Semaphore mClearingSemaphore = new Semaphore(0);
On 2016/11/07 23:45:26, Pete Williamson wrote:
> I see where we decrement the count and release the semaphore, where do we
> increment the count?

This is used as a way to block the thread until timeout or the callback has been
called. So seems there's no need to increment the count. I might be wrong here
but this is how I understand this usage...

https://codereview.chromium.org/2482623002/diff/1/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:83:
ScopedJavaLocalRef<jobjectArray> CreateJavaSavePageRequests(
On 2016/11/07 23:45:26, Pete Williamson wrote:
> I didn't see a call to this.  What does it do, and who calls it?

This is used in OnGetAllRequestsDone in the same file at line 122.

[Pete Williamson, 2016-11-08 00:51:35]

lgtm

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java
(right):

https://codereview.chromium.org/2482623002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:118:
final Semaphore mClearingSemaphore = new Semaphore(0);
On 2016/11/08 00:18:35, romax wrote:
> On 2016/11/07 23:45:26, Pete Williamson wrote:
> > I see where we decrement the count and release the semaphore, where do we
> > increment the count?
> 
> This is used as a way to block the thread until timeout or the callback has
been
> called. So seems there's no need to increment the count. I might be wrong here
> but this is how I understand this usage...

Ah, so it is only called once, not once per request.

[romax, 2016-11-08 00:55:55]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-11-08 00:56:31]

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-11-08 03:05:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-08 03:27:40]

Description was changed from

==========
[Offline Pages] Clear remaining requests after test harness finishes.

Fixed a bug where requests are not removed when test harness finished with
incomplete requests.

BUG=662247
==========

to

==========
[Offline Pages] Clear remaining requests after test harness finishes.

Fixed a bug where requests are not removed when test harness finished with
incomplete requests.

BUG=662247

Committed: https://crrev.com/3a50787136f81047838c1ce3a24e38133c7d13cc
Cr-Commit-Position: refs/heads/master@{#430494}
==========

[commit-bot: I haz the power, 2016-11-08 03:27:41]

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