[Offline pages] Switching ChangeRequestsStateTask to use RequestQueueStore::GetRequestsByIds

[Offline pages] Switching ChangeRequestsStateTask to use RequestQueueStore::GetRequestsByIds

* Small refactoring to ChangeRequestsStateTask to use a more efficient
  method to query requests (which was not available at the time it was
  originally created).

BUG=651238
Committed: https://crrev.com/aa20953bbe09465c66ac135f987b9f8bbb383e94
Cr-Commit-Position: refs/heads/master@{#436096}

[fgorski, 2016-12-01 17:26:27]

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

[commit-bot: I haz the power, 2016-12-01 17:26:47]

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

[fgorski, 2016-12-01 17:26:51]

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

[fgorski, 2016-12-01 17:26:52]

PTAL

[commit-bot: I haz the power, 2016-12-01 17:38:59]

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

[commit-bot: I haz the power, 2016-12-01 17:39:00]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fgorski, 2016-12-01 19:16:16]

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

[commit-bot: I haz the power, 2016-12-01 19:16:40]

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-12-01 20:06:10]

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

[commit-bot: I haz the power, 2016-12-01 20:06:11]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-12-02 01:39:06]

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

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.cc:60: // Look up
the missing items.
This comment is a bit misleading - It applies to the overall operation, but the
for loop on the next line is actually removing the IDs that *were* found instead
of looking up missing items.  Maybe leave a blank line after the existing
comment, and add the following comment above the for loop
// If we found a request earlier, remove it from our list of IDs.

That will make it easier on folks who read the comments in the future.

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

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task_unittest.cc:150:
int index_id_1 =
Why are you doing this?  I found the code easier to understand before, is it now
wrong because the algorithm returns things slightly differently?

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task_unittest.cc:200:
ASSERT_EQ(2UL, last_result()->item_statuses.size());
Why the change from EXPECT to ASSERT?

[fgorski, 2016-12-02 22:31:56]

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

[fgorski, 2016-12-02 22:32:03]

PTAL

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

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.cc:60: // Look up
the missing items.
On 2016/12/02 01:39:06, Pete Williamson wrote:
> This comment is a bit misleading - It applies to the overall operation, but
the
> for loop on the next line is actually removing the IDs that *were* found
instead
> of looking up missing items.  Maybe leave a blank line after the existing
> comment, and add the following comment above the for loop
> // If we found a request earlier, remove it from our list of IDs.
> 
> That will make it easier on folks who read the comments in the future.

Done.

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

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task_unittest.cc:150:
int index_id_1 =
On 2016/12/02 01:39:06, Pete Williamson wrote:
> Why are you doing this?  I found the code easier to understand before, is it
now
> wrong because the algorithm returns things slightly differently?

Yes, we did make some changes, mainly the fact that the dictionary might be
returning values in a different order on different platforms... hence this
change.

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task_unittest.cc:200:
ASSERT_EQ(2UL, last_result()->item_statuses.size());
On 2016/12/02 01:39:06, Pete Williamson wrote:
> Why the change from EXPECT to ASSERT?

When we have too many, I don't have to look at the error message too long to
work out what the problem is.,

when we have too little, there will be a crash later (when accessing the first
missing index), and I prefer to crash early.

[commit-bot: I haz the power, 2016-12-02 22:33:22]

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

[fgorski, 2016-12-02 22:38:38]

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

[fgorski, 2016-12-02 22:38:51]

Added the comment we discussed.

[commit-bot: I haz the power, 2016-12-02 22:38:55]

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

[Pete Williamson, 2016-12-02 23:09:10]

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

https://codereview.chromium.org/2541423002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.cc:60: // Look up
the missing items.
On 2016/12/02 22:32:02, fgorski wrote:
> On 2016/12/02 01:39:06, Pete Williamson wrote:
> > This comment is a bit misleading - It applies to the overall operation, but
> the
> > for loop on the next line is actually removing the IDs that *were* found
> instead
> > of looking up missing items.  Maybe leave a blank line after the existing
> > comment, and add the following comment above the for loop
> > // If we found a request earlier, remove it from our list of IDs.
> > 
> > That will make it easier on folks who read the comments in the future.
> 
> Done.

Actually, I meant to add a blank line after "// Look up the missing items." and
after that blank line, to have this comment:
// If we found a request earlier, remove it from our list of IDs.

[fgorski, 2016-12-02 23:21:44]

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

[fgorski, 2016-12-02 23:22:00]

PTAL

[commit-bot: I haz the power, 2016-12-02 23:22:00]

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

[Pete Williamson, 2016-12-02 23:29:40]

lgtm

[fgorski, 2016-12-02 23:31:40]

The CQ bit was unchecked by fgorski@chromium.org

[fgorski, 2016-12-02 23:31:45]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-12-02 23:32:41]

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-12-03 00:23:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480721505082650,
"parent_rev": "1f7412c3f1f12e078daccea3b6ff126c660a9af9", "commit_rev":
"81046c75a584081bff9d5f3188650fee99ddff1c"}

[commit-bot: I haz the power, 2016-12-03 00:23:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-03 00:26:02]

Description was changed from

==========
[Offline pages] Switching ChangeRequestsStateTask to use
RequestQueueStore::GetRequestsByIds

* Small refactoring to ChangeRequestsStateTask to use a more efficient
  method to query requests (which was not available at the time it was
  originally created).


BUG=651238
==========

to

==========
[Offline pages] Switching ChangeRequestsStateTask to use
RequestQueueStore::GetRequestsByIds

* Small refactoring to ChangeRequestsStateTask to use a more efficient
  method to query requests (which was not available at the time it was
  originally created).

BUG=651238

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

[commit-bot: I haz the power, 2016-12-03 00:26:04]

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