[Offline Pages] Introducing StorageManagerClient interface.

[Offline Pages] Introducing StorageManagerClient interface.

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

[romax, 2016-05-10 01:18:28]

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

[romax, 2016-05-10 01:18:29]

Adding StorageManagerClient interface in order to make testing easier for
storage manager and decoupling.

[romax, 2016-05-11 21:22:38]

Updated after Filip's patch extracting out callback to offline_page_types.h.
Simplified patch.

[fgorski, 2016-05-11 22:39:01]

This looks very good. I added a few more suggestions, but overall we are getting
there.

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:237: bool IsLoaded() const
override;
Did you consider using the is_loaded() defined above?
Judging by the implementation that should be good enough.

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

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:19:
policy_controller_(client->GetPolicyController()),
I am wondering if injecting policy controller independently would make more
sense. If this is the only place where you are using GetPolicyController method
from the interface, perhaps it does not belong there. Think about it and we can
discuss it.

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:64: return
!in_progress_ && client_->IsLoaded();
I suspect that IsLoaded (in either form) will not survive on the client
interface and we will drop it. After all, we can initialize the storage manager
after the model is loaded and this check never has to happen.

Think about that, please.

Both suggestions in this file serve minimize the interface (and therefore
dependencies), but let me know if you think there is anything wrong with that.

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

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:44: // Selects and
removed pages that need to be expired. Triggered as a callback
why this change?

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:49: // Get offline IDs
of all expired pages and return in |offline_ids|.
why this change?

the full sentence would be:
this method gets...

Hence "Gets ..." is a proper beginning here.

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
File components/offline_pages/storage_manager_client.h (right):

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/storage_manager_client.h:20: // This is an interface
for offline page model to implement, in order to
This interface should presume no knowledge or existence of offline page model.

This interface should be implemented by clients managed by storage manager.

Also, when you remove 2 bottom methods, per suggestion, you can move the
definition of the interface to inside of the StorageManager, and simply call it
Client as the outer class will scope the name well.

[romax, 2016-05-11 23:54:58]

Addressed comments, and simplified interface based on suggestion. PTAL

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:237: bool IsLoaded() const
override;
On 2016/05/11 22:39:00, fgorski wrote:
> Did you consider using the is_loaded() defined above?
> Judging by the implementation that should be good enough.

Per discussion, I'm going to remove it from the interface.

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

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:19:
policy_controller_(client->GetPolicyController()),
On 2016/05/11 22:39:00, fgorski wrote:
> I am wondering if injecting policy controller independently would make more
> sense. If this is the only place where you are using GetPolicyController
method
> from the interface, perhaps it does not belong there. Think about it and we
can
> discuss it.

yes i agree with this.

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:64: return
!in_progress_ && client_->IsLoaded();
On 2016/05/11 22:39:00, fgorski wrote:
> I suspect that IsLoaded (in either form) will not survive on the client
> interface and we will drop it. After all, we can initialize the storage
manager
> after the model is loaded and this check never has to happen.
> 
> Think about that, please.
> 
> Both suggestions in this file serve minimize the interface (and therefore
> dependencies), but let me know if you think there is anything wrong with that.

Done.

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

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:44: // Selects and
removed pages that need to be expired. Triggered as a callback
On 2016/05/11 22:39:00, fgorski wrote:
> why this change?

might be random edits. I think i should prevent this from happening...

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:49: // Get offline IDs
of all expired pages and return in |offline_ids|.
On 2016/05/11 22:39:00, fgorski wrote:
> why this change?
> 
> the full sentence would be:
> this method gets...
> 
> Hence "Gets ..." is a proper beginning here.

same as above..

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
File components/offline_pages/storage_manager_client.h (right):

https://codereview.chromium.org/1965633002/diff/20001/components/offline_page...
components/offline_pages/storage_manager_client.h:20: // This is an interface
for offline page model to implement, in order to
On 2016/05/11 22:39:00, fgorski wrote:
> This interface should presume no knowledge or existence of offline page model.
> 
> This interface should be implemented by clients managed by storage manager.
> 
> Also, when you remove 2 bottom methods, per suggestion, you can move the
> definition of the interface to inside of the StorageManager, and simply call
it
> Client as the outer class will scope the name well.

Acknowledged.

[fgorski, 2016-05-12 01:20:10]

lgtm

[romax, 2016-05-12 01:34:16]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-05-12 01:34:50]

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

[commit-bot: I haz the power, 2016-05-12 02:31:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-12 02:32:44]

Description was changed from

==========
[Offline Pages] Introducing StorageManagerClient interface.

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

to

==========
[Offline Pages] Introducing StorageManagerClient interface.

BUG=609357

Committed: https://crrev.com/44fdf4d230beaab374b3cb6d442e0d2fbeb14366
Cr-Commit-Position: refs/heads/master@{#393153}
==========

[commit-bot: I haz the power, 2016-05-12 02:32:45]

Patchset 3 (id:??) landed as
https://crrev.com/44fdf4d230beaab374b3cb6d442e0d2fbeb14366
Cr-Commit-Position: refs/heads/master@{#393153}