[Offline Pages] Implement OfflinePageStorageManager.

[Offline Pages] Implement OfflinePageStorageManager.

OfflinePageStorageManager should be responsible for making offline page
model to clear expired offline pages automatically.

BUG=609357
Committed: https://crrev.com/ff3a7d889cd3326bc5f080cfd7d7663c2abdd604
Cr-Commit-Position: refs/heads/master@{#392455}

[romax, 2016-05-05 01:21:52]

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

[romax, 2016-05-05 01:21:53]

A basic draft without any fancy logic (size limit/time constraint throttling/
etc.) Please take a look.

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

Good start.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/BU...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/BU...
components/offline_pages/BUILD.gn:9: # GYP:
//components/offline_pages.gypi:offline_pages
don't forget to update the gyp file accordingly.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:122: storage_manager_.reset(new
OfflinePageStorageManager(this));
I think this should happen above in the initializer list if possible.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:394: // Storage manager of this
model.
I believe we will run into a problem of distinguishing between metadata storage
and storage manager.

Could you try to add a sentence of this being used to manage the storage
consumed by archives.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_unittest.cc:296:
last_pages_exist_result_.clear();
update here, to clear your variable.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_unittest.cc:944:
->GetPolicyController()
run "git cl format"

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_unittest.cc:967: const
std::vector<OfflinePageItem>& offline_pages = GetAllPages();
don't use & here. returned value might not live long.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:42: int pages_to_clear
= GetExpiredPageIds(pages, offline_ids);
what is the problem you are trying to solve with an extra method?
Let's talk about that.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:52: base::Time now =
base::Time::Now();
this will be more flexible and testable using base::Clock

E.g. like here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/gcm_dri...

You can do it in a separate patch.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:60: return
offline_ids.size();
this information is already available outside through offline_ids.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:67: if (result !=
DeletePageResult::SUCCESS)
this may not be exactly what you want here.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:73: return
model_->is_loaded();
We should pretty much always clear pages?
Where is the throttling mentioned in the documentation?

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:76: bool
OfflinePageStorageManager::isPageExpired(const OfflinePageItem& page) {
I think you meant to use this method above?

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:77: DCHECK(model_);
why?

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:83: return false;
three lines above is a one-liner (return is bool)

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:13: #include
"components/offline_pages/offline_page_model.h"
Both can be forward declared. Include in .cc

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:26: // This manager
should be owned by OfflinePageModel since it needs the
This sentence is misleading.

Message 1: OPM should own and control the life of OPSM
Message 2: OPM will be used to get/remove pages.

They can be in separate sentences. There is no causal dependency between them.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:28: class
OfflinePageStorageManager {
please create explicit tests for this class.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:30: typedef
base::Callback<void(int)> ClearPageCallback;
ClearPagesCallback.

add at least a boolean variable to pass the status (to communicate failure)
You can also consider an enum, to communicate the failure even better.

also I find it reasonable to add a comment next to type, e.g. int /* number of
deleted pages */

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:32:
OfflinePageStorageManager(OfflinePageModel* model,
1. Will this work as an implicit constructor if I only have model where I expect
the Manager? Please figure it out, I am curious

2. Turn it into an explicit ctor accepting the model. Set size_ to the value of
const in initializer list.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:36: void
ClearPagesIfNeeded(const ClearPageCallback& callback);
documentation.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:42: int
GetExpiredPageIds(
document. What is the result?

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:44:
std::vector<int64_t>& offline_ids);
const & ?

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:52: bool
isPageExpired(const OfflinePageItem& page);
IsPageExpired

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:54: // Not owning.
Not owned.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:57: // Not owning
Not owned.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:66:
DISALLOW_COPY_AND_ASSIGN(OfflinePageStorageManager);
include base/macros.h

[romax, 2016-05-05 21:00:03]

addressed some comments, will do more changes and improvments after offline
talk. And also will fix build errors next..

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/BU...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/BU...
components/offline_pages/BUILD.gn:9: # GYP:
//components/offline_pages.gypi:offline_pages
On 2016/05/05 04:53:08, fgorski wrote:
> don't forget to update the gyp file accordingly.

Done.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:122: storage_manager_.reset(new
OfflinePageStorageManager(this));
On 2016/05/05 04:53:08, fgorski wrote:
> I think this should happen above in the initializer list if possible.

Done.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:394: // Storage manager of this
model.
On 2016/05/05 04:53:08, fgorski wrote:
> I believe we will run into a problem of distinguishing between metadata
storage
> and storage manager.
> 
> Could you try to add a sentence of this being used to manage the storage
> consumed by archives.

Done.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_unittest.cc:296:
last_pages_exist_result_.clear();
On 2016/05/05 04:53:08, fgorski wrote:
> update here, to clear your variable.

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_unittest.cc:944:
->GetPolicyController()
On 2016/05/05 04:53:08, fgorski wrote:
> run "git cl format"

this is what I have after running that... and i know it's ugly :S

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_unittest.cc:967: const
std::vector<OfflinePageItem>& offline_pages = GetAllPages();
On 2016/05/05 04:53:08, fgorski wrote:
> don't use & here. returned value might not live long.

i didnt think i would do this... must be magic...

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:42: int pages_to_clear
= GetExpiredPageIds(pages, offline_ids);
On 2016/05/05 04:53:08, fgorski wrote:
> what is the problem you are trying to solve with an extra method?
> Let's talk about that.

Acknowledged.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:52: base::Time now =
base::Time::Now();
On 2016/05/05 04:53:08, fgorski wrote:
> this will be more flexible and testable using base::Clock
> 
> E.g. like here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/gcm_dri...
> 
> You can do it in a separate patch.

Acknowledged.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:60: return
offline_ids.size();
On 2016/05/05 04:53:08, fgorski wrote:
> this information is already available outside through offline_ids.

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:67: if (result !=
DeletePageResult::SUCCESS)
On 2016/05/05 04:53:08, fgorski wrote:
> this may not be exactly what you want here.

let's talk about this...

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:73: return
model_->is_loaded();
On 2016/05/05 04:53:08, fgorski wrote:
> We should pretty much always clear pages?
> Where is the throttling mentioned in the documentation?

I also mentioned it's not implemented yet in the CL comment...

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:76: bool
OfflinePageStorageManager::isPageExpired(const OfflinePageItem& page) {
On 2016/05/05 04:53:08, fgorski wrote:
> I think you meant to use this method above?

yeah.. red bull killed me...

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:77: DCHECK(model_);
On 2016/05/05 04:53:08, fgorski wrote:
> why?

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:83: return false;
On 2016/05/05 04:53:08, fgorski wrote:
> three lines above is a one-liner (return is bool)

Done.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:13: #include
"components/offline_pages/offline_page_model.h"
On 2016/05/05 04:53:08, fgorski wrote:
> Both can be forward declared. Include in .cc

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:26: // This manager
should be owned by OfflinePageModel since it needs the
On 2016/05/05 04:53:09, fgorski wrote:
> This sentence is misleading.
> 
> Message 1: OPM should own and control the life of OPSM
> Message 2: OPM will be used to get/remove pages.
> 
> They can be in separate sentences. There is no causal dependency between them.

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:36: void
ClearPagesIfNeeded(const ClearPageCallback& callback);
On 2016/05/05 04:53:09, fgorski wrote:
> documentation.

Acknowledged.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:42: int
GetExpiredPageIds(
On 2016/05/05 04:53:09, fgorski wrote:
> document. What is the result?

it was the same as offline_ids.size().. so changed to void

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:44:
std::vector<int64_t>& offline_ids);
On 2016/05/05 04:53:09, fgorski wrote:
> const & ?

no I meant to change the value...

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:52: bool
isPageExpired(const OfflinePageItem& page);
On 2016/05/05 04:53:08, fgorski wrote:
> IsPageExpired

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:54: // Not owning.
On 2016/05/05 04:53:09, fgorski wrote:
> Not owned.

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:57: // Not owning
On 2016/05/05 04:53:09, fgorski wrote:
> Not owned.

Done.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:66:
DISALLOW_COPY_AND_ASSIGN(OfflinePageStorageManager);
On 2016/05/05 04:53:09, fgorski wrote:
> include base/macros.h

Done.

[romax, 2016-05-06 01:02:32]

some comments. still struggling writing explicit tests for the new class.

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:13: #include
"components/offline_pages/offline_page_model.h"
On 2016/05/05 04:53:08, fgorski wrote:
> Both can be forward declared. Include in .cc

I don't know how to forward declare a typedef in offline_page_model.h so i only
removed client policy controller...

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:30: typedef
base::Callback<void(int)> ClearPageCallback;
On 2016/05/05 04:53:09, fgorski wrote:
> ClearPagesCallback.
> 
> add at least a boolean variable to pass the status (to communicate failure)
> You can also consider an enum, to communicate the failure even better.
> 
> also I find it reasonable to add a comment next to type, e.g. int /* number of
> deleted pages */

In the next patch I'll be using DeletePageResult. But i'm open to add a new kind
of enum here.

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:32:
OfflinePageStorageManager(OfflinePageModel* model,
On 2016/05/05 04:53:09, fgorski wrote:
> 1. Will this work as an implicit constructor if I only have model where I
expect
> the Manager? Please figure it out, I am curious
> 
> 2. Turn it into an explicit ctor accepting the model. Set size_ to the value
of
> const in initializer list.
> 

Done.

[fgorski, 2016-05-06 05:57:25]

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

https://codereview.chromium.org/1947323002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:28: class
OfflinePageStorageManager {
On 2016/05/05 04:53:09, fgorski wrote:
> please create explicit tests for this class.

??

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:120:
policy_controller_(std::move(new ClientPolicyController())),
I don't think you need std::move here. unique pointer will assume ownership of
the pointer.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:121:
storage_manager_(std::move(new OfflinePageStorageManager(this))),
same

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:982:
EXPECT_EQ(DeletePageResult::SUCCESS, last_delete_result());
last_clear_page_result()

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:40: const
MultipleOfflinePageItemResult& pages) {
DCHECK(in_progress_);

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:61: DeletePageResult
result) {
in_progress_ = false;

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:71: LifetimePolicy
policy =
const ref

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

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:39: typedef
base::Callback<void(int, OfflinePageModel::DeletePageResult)>
typedef base::Callback<void(int /* number of deleted pages */,
   OfflinePageModel::DeletePageResult /* result */)>
 40       ClearPageCallback;

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:51: // Would be used as
callback of GetAllPages().
Selects and removes pages that need to be expired. Triggered as a callback to
|GetAllPages|

Also, I'd work on a better name for this method.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:55: // Get offline_ids
of all expired pages and return in |offline_ids|.
Gets offline IDs of all

[romax, 2016-05-07 02:22:43]

updated with comments and separate test file. However only using dummy
inheritance in the storage_manager_unittest.cc . Will change to the interface
approach later.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:120:
policy_controller_(std::move(new ClientPolicyController())),
On 2016/05/06 05:57:24, fgorski wrote:
> I don't think you need std::move here. unique pointer will assume ownership of
> the pointer.

Done.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:121:
storage_manager_(std::move(new OfflinePageStorageManager(this))),
On 2016/05/06 05:57:24, fgorski wrote:
> same

Done.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:982:
EXPECT_EQ(DeletePageResult::SUCCESS, last_delete_result());
On 2016/05/06 05:57:24, fgorski wrote:
> last_clear_page_result()

Done.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:40: const
MultipleOfflinePageItemResult& pages) {
On 2016/05/06 05:57:24, fgorski wrote:
> DCHECK(in_progress_);

Done.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:61: DeletePageResult
result) {
On 2016/05/06 05:57:24, fgorski wrote:
> in_progress_ = false;

Done.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:71: LifetimePolicy
policy =
On 2016/05/06 05:57:24, fgorski wrote:
> const ref

Done.

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

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:39: typedef
base::Callback<void(int, OfflinePageModel::DeletePageResult)>
On 2016/05/06 05:57:24, fgorski wrote:
> typedef base::Callback<void(int /* number of deleted pages */,
>    OfflinePageModel::DeletePageResult /* result */)>
>  40       ClearPageCallback;

i dont see a difference other than making it inline. will not change

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:51: // Would be used as
callback of GetAllPages().
On 2016/05/06 05:57:24, fgorski wrote:
> Selects and removes pages that need to be expired. Triggered as a callback to
> |GetAllPages|
> 
> Also, I'd work on a better name for this method.

Done.

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:55: // Get offline_ids
of all expired pages and return in |offline_ids|.
On 2016/05/06 05:57:24, fgorski wrote:
> Gets offline IDs of all

Done.

https://codereview.chromium.org/1947323002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1947323002/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:33: class
OfflinePageTestModel : public OfflinePageModel {
I'm just using the worst way to do it here... Will use the interface approach if
needed to change.

[fgorski, 2016-05-09 20:20:38]

lgtm.

Let's put this in and continue moving tests (and introduce the right interface
for storage manager client/consumer/subject in the patches to follow).

Please fix the compilation issue. Ask questions if you have any regarding this
problem or others, please.

Thanks.
F

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

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:51: // Would be used as
callback of GetAllPages().
On 2016/05/06 05:57:24, fgorski wrote:
> Selects and removes pages that need to be expired. Triggered as a callback to
> |GetAllPages|
> 
> Also, I'd work on a better name for this method.

no sure you applied first part of the comment

https://codereview.chromium.org/1947323002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:55: // Get offline_ids
of all expired pages and return in |offline_ids|.
On 2016/05/07 02:22:42, romax wrote:
> On 2016/05/06 05:57:24, fgorski wrote:
> > Gets offline IDs of all
> 
> Done.

not sure you applied the comment

https://codereview.chromium.org/1947323002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1947323002/diff/80001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:25: const
base::FilePath kFilePath(std::string("TEST_FILEPATH");
you need to use a file path literal for this to work.

Example from mthml archiver unit test:
const base::FilePath::CharType kTestFilePath[] = FILE_PATH_LITERAL(
    "/archive_dir/offline_page.mhtml");

that will sort out your compilation issue.

[romax, 2016-05-09 22:44:24]

The CQ bit was checked by romax@chromium.org

[romax, 2016-05-09 22:44:25]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1947323002/#ps120001
(title: "fix rebasing")

[commit-bot: I haz the power, 2016-05-09 22:44:56]

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

[commit-bot: I haz the power, 2016-05-09 22:50:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-09 22:51:58]

Description was changed from

==========
[Offline Pages] Implement OfflinePageStorageManager.

OfflinePageStorageManager should be responsible for making offline page
model to clear expired offline pages automatically.

BUG=609357
==========

to

==========
[Offline Pages] Implement OfflinePageStorageManager.

OfflinePageStorageManager should be responsible for making offline page
model to clear expired offline pages automatically.

BUG=609357

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

[commit-bot: I haz the power, 2016-05-09 22:52:00]

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