Remove outdated TODOs for romax.

Remove outdated TODOs for romax.

Clearing up the outdated TODOs for romax@, as part of paquete fixit effort.
The removed ones are no longer needed, and added bug numbers for the
remaining ones.

BUG=707405
Review-Url: https://codereview.chromium.org/2791013002
Cr-Commit-Position: refs/heads/master@{#462202}
Committed: https://chromium.googlesource.com/chromium/src/+/722584c2e23d7f2c75bc05d0bfb36f866ca71b4f

[romax, 2017-03-31 21:47:10]

Description was changed from

==========
Remove outdated TODOs for romax.

Clearing up the outdated TODOs for romax@, as part of paquete fixit effort.

BUG=707405
==========

to

==========
Remove outdated TODOs for romax.

Clearing up the outdated TODOs for romax@, as part of paquete fixit effort.
The removed ones are no longer needed, and added bug numbers for the
remaining ones.

BUG=707405
==========

[romax, 2017-03-31 21:47:59]

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

[commit-bot: I haz the power, 2017-03-31 21:48:40]

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

[romax, 2017-03-31 21:50:02]

romax@chromium.org changed reviewers:
+ chili@chromium.org, fgorski@chromium.org

[romax, 2017-03-31 21:50:03]

fgorski@, PTAL
chili@, FYI

Thanks!

[commit-bot: I haz the power, 2017-03-31 23:21:12]

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

[commit-bot: I haz the power, 2017-03-31 23:21:13]

Dry run: This issue passed the CQ dry run.

[chili, 2017-04-04 22:36:57]

lgtm with nit

https://codereview.chromium.org/2791013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2791013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
// It's based on the assumption that if this method is called with
|hasUserGesture| == false
I think this comment can be better phrased:

We assume that |hasUserGesture| == false means resume all pending requests

The last sentence is extraneous as changing the assumption should mean checking
all the callers to make sure nothing breaks. There's no good way for a caller to
constantly verify that your assumption hasn't changed.

[romax, 2017-04-04 23:47:08]

addressed comment from chili@.

https://codereview.chromium.org/2791013002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2791013002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
// It's based on the assumption that if this method is called with
|hasUserGesture| == false
On 2017/04/04 22:36:57, chili wrote:
> I think this comment can be better phrased:
> 
> We assume that |hasUserGesture| == false means resume all pending requests
> 
> The last sentence is extraneous as changing the assumption should mean
checking
> all the callers to make sure nothing breaks. There's no good way for a caller
to
> constantly verify that your assumption hasn't changed.

Done.
Yeah I'm not sure if it's worth to add another API on the interface like
ResumeAllPendingDownloads. But this comment can definitely be better :)

[fgorski, 2017-04-05 16:50:24]

lgtm with comments

https://codereview.chromium.org/2791013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2791013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:199: //
TODO(romax) Decide if we want to also take a snapshot on the last timeout
nit: TODO(romax):

https://codereview.chromium.org/2791013002/diff/20001/components/offline_page...
File components/offline_pages/core/offline_page_metadata_store_impl_unittest.cc
(left):

https://codereview.chromium.org/2791013002/diff/20001/components/offline_page...
components/offline_pages/core/offline_page_metadata_store_impl_unittest.cc:42:
// TODO(romax): move it to sql_unittests.
please open a bug to properly rename this file.

[romax, 2017-04-05 18:41:00]

addressed comments from fgorski. will commit.

https://codereview.chromium.org/2791013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2791013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:199: //
TODO(romax) Decide if we want to also take a snapshot on the last timeout
On 2017/04/05 16:50:24, fgorski wrote:
> nit: TODO(romax):

Done.

https://codereview.chromium.org/2791013002/diff/20001/components/offline_page...
File components/offline_pages/core/offline_page_metadata_store_impl_unittest.cc
(left):

https://codereview.chromium.org/2791013002/diff/20001/components/offline_page...
components/offline_pages/core/offline_page_metadata_store_impl_unittest.cc:42:
// TODO(romax): move it to sql_unittests.
On 2017/04/05 16:50:24, fgorski wrote:
> please open a bug to properly rename this file.

Done. 708700
I was thinking about this, and this file was named as 'impl' probably because
there was another in-memory implementation and they were sharing similar unit
tests. I think renaming this file to *_sql_unittests.cc makes sense since we
don't have that in-memory one anymore. WDYT?

[romax, 2017-04-05 18:41:16]

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

[commit-bot: I haz the power, 2017-04-05 18:41:51]

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, 2017-04-05 20:08:17]

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

[commit-bot: I haz the power, 2017-04-05 20:08:18]

Dry run: This issue passed the CQ dry run.

[romax, 2017-04-05 20:19:12]

The CQ bit was checked by romax@chromium.org

[romax, 2017-04-05 20:19:13]

The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2791013002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-04-05 20:19:57]

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, 2017-04-05 20:34:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491423552585690,
"parent_rev": "ce8ce27e673178544abcb250b51b7860e0e097d7", "commit_rev":
"722584c2e23d7f2c75bc05d0bfb36f866ca71b4f"}

[commit-bot: I haz the power, 2017-04-05 20:35:27]

Description was changed from

==========
Remove outdated TODOs for romax.

Clearing up the outdated TODOs for romax@, as part of paquete fixit effort.
The removed ones are no longer needed, and added bug numbers for the
remaining ones.

BUG=707405
==========

to

==========
Remove outdated TODOs for romax.

Clearing up the outdated TODOs for romax@, as part of paquete fixit effort.
The removed ones are no longer needed, and added bug numbers for the
remaining ones.

BUG=707405

Review-Url: https://codereview.chromium.org/2791013002
Cr-Commit-Position: refs/heads/master@{#462202}
Committed:
https://chromium.googlesource.com/chromium/src/+/722584c2e23d7f2c75bc05d0bfb3...
==========

[commit-bot: I haz the power, 2017-04-05 20:35:28]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/722584c2e23d7f2c75bc05d0bfb3...