[Offline pages] Hooking up Archive Manager to Offline Page Model

[Offline pages] Hooking up Archive Manager to Offline Page Model

* Replaces all the file thread actions in offline page model with calls
  to ArchiveManager
* Updates/simplifies the method checking for file deletion.

BUG=605615
Committed: https://crrev.com/77b9300cb9d8b3ecec43333a46a837dcfe7f468a
Cr-Commit-Position: refs/heads/master@{#394474}

[fgorski, 2016-05-17 03:44:46]

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

[fgorski, 2016-05-17 03:44:46]

PTAL

[jianli, 2016-05-17 21:48:13]

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
File components/offline_pages/archive_manager.cc (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:103:
base::Bind(GetAllArchivesImplDone, callback, base::Owned(archive_paths)));
I think you don't need GetAllArchivesImplDone.

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
File components/offline_pages/archive_manager.h (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:50: // Lists all files int the
archives in the archive directory.
nit: typo for int.

Simpler to say: // Retrieves all archive files in the archive directory.

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
File components/offline_pages/archive_manager_unittest.cc (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager_unittest.cc:233: EXPECT_EQ(3UL,
last_archvie_paths().size());
nit: ASSERT_EQ

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:705: void
OfflinePageModel::ScanForMissingArchivePaths(
nit: ScanForMissingArchiveFiles

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:369: // Manager for the offline
archive files and directory, where these files are
I think you can skip "directory, where ..." in the comment.

[fgorski, 2016-05-17 22:50:29]

PTAL

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
File components/offline_pages/archive_manager.cc (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:103:
base::Bind(GetAllArchivesImplDone, callback, base::Owned(archive_paths)));
On 2016/05/17 21:48:13, jianli wrote:
> I think you don't need GetAllArchivesImplDone.

Changed per offline discussion.

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
File components/offline_pages/archive_manager.h (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:50: // Lists all files int the
archives in the archive directory.
On 2016/05/17 21:48:13, jianli wrote:
> nit: typo for int.
> 
> Simpler to say: // Retrieves all archive files in the archive directory.

Done.

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
File components/offline_pages/archive_manager_unittest.cc (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager_unittest.cc:233: EXPECT_EQ(3UL,
last_archvie_paths().size());
On 2016/05/17 21:48:13, jianli wrote:
> nit: ASSERT_EQ

Done.

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:705: void
OfflinePageModel::ScanForMissingArchivePaths(
On 2016/05/17 21:48:13, jianli wrote:
> nit: ScanForMissingArchiveFiles

Done.

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1981093002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:369: // Manager for the offline
archive files and directory, where these files are
On 2016/05/17 21:48:13, jianli wrote:
> I think you can skip "directory, where ..." in the comment.

Done.

[jianli, 2016-05-17 23:20:58]

lgtm

[fgorski, 2016-05-17 23:21:27]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-05-17 23:21:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1981093002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1981093002/20001

[commit-bot: I haz the power, 2016-05-18 01:28:33]

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

[commit-bot: I haz the power, 2016-05-18 01:28:34]

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

[fgorski, 2016-05-18 17:13:25]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-18 17:13:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1981093002/#ps60001
(title: "Fixing the windows bot")

[commit-bot: I haz the power, 2016-05-18 17:14:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1981093002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1981093002/60001

[commit-bot: I haz the power, 2016-05-18 17:19:41]

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

[commit-bot: I haz the power, 2016-05-18 17:19:42]

Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[fgorski, 2016-05-18 17:48:11]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-05-18 17:48:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1981093002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1981093002/60001

[commit-bot: I haz the power, 2016-05-18 18:33:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-18 18:35:46]

Description was changed from

==========
[Offline pages] Hooking up Archive Manager to Offline Page Model

* Replaces all the file thread actions in offline page model with calls
  to ArchiveManager
* Updates/simplifies the method checking for file deletion.

BUG=605615
==========

to

==========
[Offline pages] Hooking up Archive Manager to Offline Page Model

* Replaces all the file thread actions in offline page model with calls
  to ArchiveManager
* Updates/simplifies the method checking for file deletion.

BUG=605615

Committed: https://crrev.com/77b9300cb9d8b3ecec43333a46a837dcfe7f468a
Cr-Commit-Position: refs/heads/master@{#394474}
==========

[commit-bot: I haz the power, 2016-05-18 18:35:47]

Patchset 4 (id:??) landed as
https://crrev.com/77b9300cb9d8b3ecec43333a46a837dcfe7f468a
Cr-Commit-Position: refs/heads/master@{#394474}