[Offline pages] Adding ArchiveManager to abstract offline archives management

[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.
* Adding missing destructor for OfflinePageStorageManager::Client (found by ASAN tests enabled in this patch)

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

[fgorski, 2016-05-13 23:07:41]

Description was changed from

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.

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

to

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.

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

[fgorski, 2016-05-13 23:07:41]

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

[fgorski, 2016-05-13 23:07:50]

PTAL

[jianli, 2016-05-13 23:50:49]

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

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:11: namespace offline_pages {
nit: add empty line

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:12: namespace {
nit: add empty line

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:18: DCHECK(exists);
Please include logging.h

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

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:18: namespace offline_pages {
nit: add empty line

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:19: // Class that helps managing
archive files. It will fail any operations on
The comment "It will fail any operations on ..." is kind of confusing. How
about:
  // Class that manages all archive files for offline pages. They're all stored
in a specific archive directory.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:27: // Creates archives directory of
one does not exist yet;
Please also comment above this line that all operations are performed in the
task runner.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:30: // Verifies whether an archive
with specified |archive_path| exists.
Verifies => Checks

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:31: virtual void ArchiveExists(const
base::FilePath& archive_path,
ExistsArchive

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:38: // Deletes all archives in
|archive_paths|.
The comment "all archives" is very confusing. How about updating it to:
  // Deletes multiple archives that are specified in |archive_paths| collection.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:47: };
nit: add empty line

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

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager_unittest.cc:117:
TEST_F(ArchiveManagerTest, DeleteArchives) {
Please also test the cases
1) All the archive files do not exist.
2) Some archive files do not exist.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager_unittest.cc:137:
TEST_F(ArchiveManagerTest, DeleteArchive) {
Please also test the case that the archive does not exist.

[fgorski, 2016-05-16 17:46:52]

all done. PTAL

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

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:11: namespace offline_pages {
On 2016/05/13 23:50:48, jianli wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:12: namespace {
On 2016/05/13 23:50:48, jianli wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.cc:18: DCHECK(exists);
On 2016/05/13 23:50:48, jianli wrote:
> Please include logging.h

Done.

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

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:18: namespace offline_pages {
On 2016/05/13 23:50:48, jianli wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:19: // Class that helps managing
archive files. It will fail any operations on
On 2016/05/13 23:50:48, jianli wrote:
> The comment "It will fail any operations on ..." is kind of confusing. How
> about:
>   // Class that manages all archive files for offline pages. They're all
stored
> in a specific archive directory.

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:27: // Creates archives directory of
one does not exist yet;
On 2016/05/13 23:50:48, jianli wrote:
> Please also comment above this line that all operations are performed in the
> task runner.

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:30: // Verifies whether an archive
with specified |archive_path| exists.
On 2016/05/13 23:50:48, jianli wrote:
> Verifies => Checks

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:31: virtual void ArchiveExists(const
base::FilePath& archive_path,
On 2016/05/13 23:50:48, jianli wrote:
> ExistsArchive

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:38: // Deletes all archives in
|archive_paths|.
On 2016/05/13 23:50:48, jianli wrote:
> The comment "all archives" is very confusing. How about updating it to:
>   // Deletes multiple archives that are specified in |archive_paths|
collection.

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager.h:47: };
On 2016/05/13 23:50:48, jianli wrote:
> nit: add empty line

Done.

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

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager_unittest.cc:117:
TEST_F(ArchiveManagerTest, DeleteArchives) {
On 2016/05/13 23:50:49, jianli wrote:
> Please also test the cases
> 1) All the archive files do not exist.
> 2) Some archive files do not exist.

Done.

https://codereview.chromium.org/1977853004/diff/1/components/offline_pages/ar...
components/offline_pages/archive_manager_unittest.cc:137:
TEST_F(ArchiveManagerTest, DeleteArchive) {
On 2016/05/13 23:50:48, jianli wrote:
> Please also test the case that the archive does not exist.

Done.

[Dmitry Titov, 2016-05-16 21:04:56]

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

[Dmitry Titov, 2016-05-16 21:04:57]

drive-by:

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
File components/offline_pages/archive_manager.h (right):

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
components/offline_pages/archive_manager.h:29: // Creates archives directory of
one does not exist yet;
of->if

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
components/offline_pages/archive_manager.h:33: // It is considered successful to
attempt to delete a file that does not
copy/pasted comment?

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
components/offline_pages/archive_manager.h:45: virtual void DeleteArchives(const
std::vector<base::FilePath>& archive_paths,
I'd suggest DeleteMultipleArchives to make these names more visibly distinct.

[fgorski, 2016-05-16 21:22:08]

Patchset #4 (id:60001) has been deleted

[fgorski, 2016-05-16 21:23:01]

PTAL. Additional feedback addressed. GYP build fixed.

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
File components/offline_pages/archive_manager.h (right):

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
components/offline_pages/archive_manager.h:29: // Creates archives directory of
one does not exist yet;
On 2016/05/16 21:04:56, Dmitry Titov wrote:
> of->if

Done.

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
components/offline_pages/archive_manager.h:33: // It is considered successful to
attempt to delete a file that does not
On 2016/05/16 21:04:56, Dmitry Titov wrote:
> copy/pasted comment?

Done.

https://codereview.chromium.org/1977853004/diff/40001/components/offline_page...
components/offline_pages/archive_manager.h:45: virtual void DeleteArchives(const
std::vector<base::FilePath>& archive_paths,
On 2016/05/16 21:04:56, Dmitry Titov wrote:
> I'd suggest DeleteMultipleArchives to make these names more visibly distinct.

Done.

[Dmitry Titov, 2016-05-16 22:07:42]

lgtm

[jianli, 2016-05-16 22:39:14]

lgtm

[fgorski, 2016-05-17 03:40:22]

Description was changed from

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.

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

to

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.
* Adding missing destructor for OfflinePageStorageManager::Client (found by ASAN
tests enabled in this patch)

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

[fgorski, 2016-05-17 03:40:43]

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

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

+romax@ FYI

[fgorski, 2016-05-17 04:53:17]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-17 04:53:18]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1977853004/#ps100001
(title: "Adding a missing destructor for OfflinePageStorageManager::Client")

[commit-bot: I haz the power, 2016-05-17 04:53:47]

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

[commit-bot: I haz the power, 2016-05-17 04:58:03]

Description was changed from

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.
* Adding missing destructor for OfflinePageStorageManager::Client (found by ASAN
tests enabled in this patch)

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

to

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.
* Adding missing destructor for OfflinePageStorageManager::Client (found by ASAN
tests enabled in this patch)

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

[commit-bot: I haz the power, 2016-05-17 04:58:04]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-05-17 04:59:35]

Description was changed from

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.
* Adding missing destructor for OfflinePageStorageManager::Client (found by ASAN
tests enabled in this patch)

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

to

==========
[Offline pages] Adding ArchiveManager to abstract offline archives management

* This patch only creates the archiver (it will be hooked up to the model
  in the next one.)
* Creates set of tests for the archiver.
* Adding missing destructor for OfflinePageStorageManager::Client (found by ASAN
tests enabled in this patch)

BUG=605615

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

[commit-bot: I haz the power, 2016-05-17 04:59:36]

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