Update the request count when a request fails.

Update the request count when a request fails.

Also implements the UpdateRequest method on the request queue needed to
be able to update the request count, and adds the appropriate tests.

BUG=610521
Committed: https://crrev.com/9fb382cb6e8996b0819a398a736249555c3eaa8c
Cr-Commit-Position: refs/heads/master@{#407337}

[Pete Williamson, 2016-07-22 01:12:43]

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

[Pete Williamson, 2016-07-22 01:12:43]

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:209: /* ||
new_attempt_count > policy_->GetMaxRetries()*/) {
This commented out code will be commented back in once changelist 2113383002
lands (implements request picker and policy, we need the policy to be
implemented for this line to compile).  In any case, this code will be either
removed or commented back in before this changelist is submitted.

[fgorski, 2016-07-22 16:14:58]

looks mostly OK.

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:102: LOG(WARNING) <<
"Failed to update a request retry count. "
why not DVLOG?

Also, do we have a UMA for this case? It would be reasonable to report that (and
maybe add a todo), and of course it would not be conditional on success.

Another thing is to consider showing that stuff on the internals page.

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:222:
queue_->UpdateRequest(updated_request,
nit: is this how git cl format worked on this call?

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_queue.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:89:
store_->GetRequests(base::Bind(
This is probably going to be more efficient if we can query for a single request
based on the ID.  Can  you open a bug, and add a TODO here?

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:104: if (!success) {
what about the case where found_request does not contain a request with ID
matching that of update_request?

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_queue_unittest.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_unittest.cc:210:
TEST_F(RequestQueueTest, UpdateRequest) {
Please add a test for UpdateRequest, when it is not present in the DB. (The
missed path in the code)

[Pete Williamson, 2016-07-22 18:27:00]

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:102: LOG(WARNING) <<
"Failed to update a request retry count. "
On 2016/07/22 16:14:58, fgorski wrote:
> why not DVLOG?
> 
> Also, do we have a UMA for this case? It would be reasonable to report that
(and
> maybe add a todo), and of course it would not be conditional on success.
> 
> Another thing is to consider showing that stuff on the internals page.

TODO added, Changed to DLOG (DVLOG doesn't work with warning levels).

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:222:
queue_->UpdateRequest(updated_request,
On 2016/07/22 16:14:58, fgorski wrote:
> nit: is this how git cl format worked on this call?

Nope, that was me.  Changed to be a bit more uniform.

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_queue.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:89:
store_->GetRequests(base::Bind(
On 2016/07/22 16:14:58, fgorski wrote:
> This is probably going to be more efficient if we can query for a single
request
> based on the ID.  Can  you open a bug, and add a TODO here?

TODO added. crbug.com/630657.

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:104: if (!success) {
On 2016/07/22 16:14:58, fgorski wrote:
> what about the case where found_request does not contain a request with ID
> matching that of update_request?

Added case.

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_queue_unittest.cc (right):

https://codereview.chromium.org/2176453002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_unittest.cc:210:
TEST_F(RequestQueueTest, UpdateRequest) {
On 2016/07/22 16:14:58, fgorski wrote:
> Please add a test for UpdateRequest, when it is not present in the DB. (The
> missed path in the code)

Done.

[Pete Williamson, 2016-07-22 18:27:27]

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

[commit-bot: I haz the power, 2016-07-22 18:28:03]

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-07-22 20:58:17]

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

[commit-bot: I haz the power, 2016-07-22 20:58:18]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-07-22 20:59:44]

lgtm

[Pete Williamson, 2016-07-22 23:04:32]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-07-22 23:04:33]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2176453002/#ps40001
(title: "Logical Merge changes")

[commit-bot: I haz the power, 2016-07-22 23:04:58]

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-07-22 23:14:13]

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

[commit-bot: I haz the power, 2016-07-22 23:14:14]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pete Williamson, 2016-07-23 01:02:10]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-07-23 01:02:10]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2176453002/#ps100001
(title: "fix test")

[commit-bot: I haz the power, 2016-07-23 01:02:38]

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-07-23 03:35:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-23 03:38:11]

Description was changed from

==========
Update the request count when a request fails.

Also implements the UpdateRequest method on the request queue needed to
be able to update the request count, and adds the appropriate tests.

BUG=610521
==========

to

==========
Update the request count when a request fails.

Also implements the UpdateRequest method on the request queue needed to
be able to update the request count, and adds the appropriate tests.

BUG=610521

Committed: https://crrev.com/9fb382cb6e8996b0819a398a736249555c3eaa8c
Cr-Commit-Position: refs/heads/master@{#407337}
==========

[commit-bot: I haz the power, 2016-07-23 03:38:12]

Patchset 6 (id:??) landed as
https://crrev.com/9fb382cb6e8996b0819a398a736249555c3eaa8c
Cr-Commit-Position: refs/heads/master@{#407337}