[OfflinePages] Resuming a SavePageRequest now may start processing if network is connected.

[OfflinePages] Resuming a SavePageRequest now may start processing if network is connected.

BUG=641411
Committed: https://crrev.com/f8c26a79f6b7b886787f6f611c3fa9c302b42abf
Cr-Commit-Position: refs/heads/master@{#414887}

[dougarnett, 2016-08-26 20:15:39]

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

[commit-bot: I haz the power, 2016-08-26 20:16:50]

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

[dougarnett, 2016-08-26 20:29:41]

dougarnett@chromium.org changed reviewers:
+ dimich@google.com, petewil@chromium.org

[dougarnett, 2016-08-26 20:29:42]

This prepared against origin so I will need some pointers in applying it to M54
branch if we decide we want it.

[Pete Williamson, 2016-08-26 20:39:13]

mostly looks good, one question.

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

https://codereview.chromium.org/2282973003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:236:
request.request_state() == SavePageRequest::RequestState::AVAILABLE)
We aren't checking the results, which makes me wonder - what would happen if we
tried to pause a request, and pausing failed, and we returned the request in the
request list, but status was available.  Would we start trying to process it
right away?

[dougarnett, 2016-08-26 20:57:47]

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

https://codereview.chromium.org/2282973003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:236:
request.request_state() == SavePageRequest::RequestState::AVAILABLE)
On 2016/08/26 20:39:12, Pete Williamson wrote:
> We aren't checking the results, which makes me wonder - what would happen if
we
> tried to pause a request, and pausing failed, and we returned the request in
the
> request list, but status was available.  Would we start trying to process it
> right away?

Good point - we could start processing whatever next request is picked (if not
already processing).

Would be better to guard it by result. Should we also guard call
NotifyChanged()?

[commit-bot: I haz the power, 2016-08-26 21:10:13]

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

[commit-bot: I haz the power, 2016-08-26 21:10:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dougarnett, 2016-08-26 21:19:17]

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

[commit-bot: I haz the power, 2016-08-26 21:19:58]

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

[dougarnett, 2016-08-26 21:20:19]

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

https://codereview.chromium.org/2282973003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:236:
request.request_state() == SavePageRequest::RequestState::AVAILABLE)
On 2016/08/26 20:57:47, dougarnett wrote:
> On 2016/08/26 20:39:12, Pete Williamson wrote:
> > We aren't checking the results, which makes me wonder - what would happen if
> we
> > tried to pause a request, and pausing failed, and we returned the request in
> the
> > request list, but status was available.  Would we start trying to process it
> > right away?
> 
> Good point - we could start processing whatever next request is picked (if not
> already processing).
> 
> Would be better to guard it by result. Should we also guard call
> NotifyChanged()?

uploaded change to just guard the new flag for calling start processing

[Pete Williamson, 2016-08-26 21:23:55]

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:237: results) {
What if we resume a large batch of results?  If so, we get N^2 behavior.  Should
we use a std::unordered_set here (it wraps a hash map, so we get O(1) lookup). 
I got the same feedback on a similar change.

[dougarnett, 2016-08-26 21:40:52]

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:237: results) {
On 2016/08/26 21:23:55, Pete Williamson wrote:
> What if we resume a large batch of results?  If so, we get N^2 behavior. 
Should
> we use a std::unordered_set here (it wraps a hash map, so we get O(1) lookup).

> I got the same feedback on a similar change.

Yeah, bulk Pause looks like N^2 - bulk Resume does not. So perhaps could "know"
what the operation was, to avoid the N^2 (if it meant simpler code).

I'm not clear about your suggestion. Do you mean change the returned result
object from RequestQueue or something locally? Seems good to do something at
RequestQueue interface level at some point rather than (prematurely?) worry
about N^2 in every code review trying to cope with this result vector. Initially
I wondered about vector of successful requests and vector of pairs failed
requests with result codes in case that might generally be easier to consume.

[Dmitry Titov, 2016-08-26 21:46:57]

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

[Dmitry Titov, 2016-08-26 21:46:57]

lgtm with a note:

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:241:
available_user_request = true;
+1 to what Pete says, we could load all request_ids of AVAILABLE into a
hash_map, then walk over results and .find() every SUCCESSful update... then
break;

[Pete Williamson, 2016-08-26 21:52:22]

On 2016/08/26 21:40:52, dougarnett wrote:
>
https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
> File components/offline_pages/background/request_coordinator.cc (right):
> 
>
https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
> components/offline_pages/background/request_coordinator.cc:237: results) {
> On 2016/08/26 21:23:55, Pete Williamson wrote:
> > What if we resume a large batch of results?  If so, we get N^2 behavior. 
> Should
> > we use a std::unordered_set here (it wraps a hash map, so we get O(1)
lookup).
> 
> > I got the same feedback on a similar change.
> 
> Yeah, bulk Pause looks like N^2 - bulk Resume does not. So perhaps could
"know"
> what the operation was, to avoid the N^2 (if it meant simpler code).
> 
> I'm not clear about your suggestion. Do you mean change the returned result
> object from RequestQueue or something locally? Seems good to do something at
> RequestQueue interface level at some point rather than (prematurely?) worry
> about N^2 in every code review trying to cope with this result vector.
Initially
> I wondered about vector of successful requests and vector of pairs failed
> requests with result codes in case that might generally be easier to consume.

Dmitry explained my suggestion pretty well.  Instead of doing a for loop over
results inside the for loop over requests,
put the results into a hash map, and use find() in an O(1) operation (we would
still have a for loop for requests, but not for results)

[Dmitry Titov, 2016-08-26 21:57:04]

dimich@chromium.org changed reviewers:
- dimich@google.com

[Dmitry Titov, 2016-08-26 21:57:05]

-dimich@google.com, leave dimich@chromium.org

[dougarnett, 2016-08-26 22:33:44]

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:241:
available_user_request = true;
On 2016/08/26 21:46:57, Dmitry Titov wrote:
> +1 to what Pete says, we could load all request_ids of AVAILABLE into a
> hash_map, then walk over results and .find() every SUCCESSful update... then
> break;

I added a TODO for the moment to flag the topic in the code. 

I do wonder if we would be better off looking to improve the interface from
RequestQueue (for M55) to avoid the need for consumers to create a hash map to
cope with it. Is the hash map allocation cheap enough in C++ environment that we
don't mind creating it for the existing N==1 case?

[Pete Williamson, 2016-08-26 23:38:02]

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:241:
available_user_request = true;
On 2016/08/26 22:33:44, dougarnett wrote:
> On 2016/08/26 21:46:57, Dmitry Titov wrote:
> > +1 to what Pete says, we could load all request_ids of AVAILABLE into a
> > hash_map, then walk over results and .find() every SUCCESSful update... then
> > break;
> 
> I added a TODO for the moment to flag the topic in the code. 
> 
> I do wonder if we would be better off looking to improve the interface from
> RequestQueue (for M55) to avoid the need for consumers to create a hash map to
> cope with it. Is the hash map allocation cheap enough in C++ environment that
we
> don't mind creating it for the existing N==1 case? 

It's really pretty easy.  I think I'd prefer to fix it now.  We can leave the
TODO to change the design in M55  Here's an example from one of my patches out
for review.  However, if you need to get this into M54, I am OK with just doing
a TODO.

   std::unordered_set<int64_t> request_id_set(
       request_ids.begin(), request_ids.end());
   for (const SavePageRequest& request : requests) {
     // If this request is in our list, update it.
     if (request_id_set.count(request.request_id()) > 0) {
       SavePageRequest updated_request = request;
       updated_request.set_request_state(new_state);
       // TODO(petewil): Add a batch method so we only make a single call to the
       // store.
       queue_->UpdateRequest(
           updated_request,
           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                      weak_ptr_factory_.GetWeakPtr(), request.request_id()));
     }
   }

[dougarnett, 2016-08-26 23:48:22]

On 2016/08/26 23:38:02, Pete Williamson wrote:
>
https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
> File components/offline_pages/background/request_coordinator.cc (right):
> 
>
https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
> components/offline_pages/background/request_coordinator.cc:241:
> available_user_request = true;
> On 2016/08/26 22:33:44, dougarnett wrote:
> > On 2016/08/26 21:46:57, Dmitry Titov wrote:
> > > +1 to what Pete says, we could load all request_ids of AVAILABLE into a
> > > hash_map, then walk over results and .find() every SUCCESSful update...
then
> > > break;
> > 
> > I added a TODO for the moment to flag the topic in the code. 
> > 
> > I do wonder if we would be better off looking to improve the interface from
> > RequestQueue (for M55) to avoid the need for consumers to create a hash map
to
> > cope with it. Is the hash map allocation cheap enough in C++ environment
that
> we
> > don't mind creating it for the existing N==1 case? 
> 
> It's really pretty easy.  I think I'd prefer to fix it now.  We can leave the
> TODO to change the design in M55  Here's an example from one of my patches out
> for review.  However, if you need to get this into M54, I am OK with just
doing
> a TODO.
> 
>    std::unordered_set<int64_t> request_id_set(
>        request_ids.begin(), request_ids.end());
>    for (const SavePageRequest& request : requests) {
>      // If this request is in our list, update it.
>      if (request_id_set.count(request.request_id()) > 0) {
>        SavePageRequest updated_request = request;
>        updated_request.set_request_state(new_state);
>        // TODO(petewil): Add a batch method so we only make a single call to
the
>        // store.
>        queue_->UpdateRequest(
>            updated_request,
>            base::Bind(&RequestCoordinator::UpdateRequestCallback,
>                       weak_ptr_factory_.GetWeakPtr(), request.request_id()));
>      }
>    }

I propose land for M54 merging with TODO since only seem to be able to pause or
resume a single item at a time.
If I am missing the UX where we can do multiple, then I can add.

[Pete Williamson, 2016-08-26 23:48:59]

lgtm

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2282973003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:241:
available_user_request = true;
On 2016/08/26 23:38:02, Pete Williamson wrote:
> On 2016/08/26 22:33:44, dougarnett wrote:
> > On 2016/08/26 21:46:57, Dmitry Titov wrote:
> > > +1 to what Pete says, we could load all request_ids of AVAILABLE into a
> > > hash_map, then walk over results and .find() every SUCCESSful update...
then
> > > break;
> > 
> > I added a TODO for the moment to flag the topic in the code. 
> > 
> > I do wonder if we would be better off looking to improve the interface from
> > RequestQueue (for M55) to avoid the need for consumers to create a hash map
to
> > cope with it. Is the hash map allocation cheap enough in C++ environment
that
> we
> > don't mind creating it for the existing N==1 case? 
> 
> It's really pretty easy.  I think I'd prefer to fix it now.  We can leave the
> TODO to change the design in M55  Here's an example from one of my patches out
> for review.  However, if you need to get this into M54, I am OK with just
doing
> a TODO.
> 
>    std::unordered_set<int64_t> request_id_set(
>        request_ids.begin(), request_ids.end());
>    for (const SavePageRequest& request : requests) {
>      // If this request is in our list, update it.
>      if (request_id_set.count(request.request_id()) > 0) {
>        SavePageRequest updated_request = request;
>        updated_request.set_request_state(new_state);
>        // TODO(petewil): Add a batch method so we only make a single call to
the
>        // store.
>        queue_->UpdateRequest(
>            updated_request,
>            base::Bind(&RequestCoordinator::UpdateRequestCallback,
>                       weak_ptr_factory_.GetWeakPtr(), request.request_id()));
>      }
>    }

DougArnett pointed out that the UI only supports pause and resume of a single
request at a time, so we can postpone the map optimization until we have a way
to resume multiples.

[dougarnett, 2016-08-26 23:49:51]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-08-26 23:49:51]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2282973003/#ps40001
(title: "Added TODO for double loop")

[commit-bot: I haz the power, 2016-08-26 23:51:04]

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-27 01:47:34]

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

[commit-bot: I haz the power, 2016-08-27 01:47:35]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougarnett, 2016-08-27 02:03:01]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-08-27 02:03:46]

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-27 10:29:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-27 10:31:16]

Description was changed from

==========
[OfflinePages] Resuming a SavePageRequest now may start processing if network is
connected.

BUG=641411
==========

to

==========
[OfflinePages] Resuming a SavePageRequest now may start processing if network is
connected.

BUG=641411

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

[commit-bot: I haz the power, 2016-08-27 10:31:17]

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