Splits the OfflinePageModel into and interface and and implementation class.

Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static members and methods
were interesting deal with and we might consider better location/resolution to follow-up on.

BUG=614864, 610824
Committed: https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb
Cr-Commit-Position: refs/heads/master@{#396885}

[dougarnett, 2016-05-26 00:11:34]

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

[dougarnett, 2016-05-26 00:11:35]

Yafei, do you want to take the first pass on this?

[romax, 2016-05-26 01:26:49]

Mostly looks good to me. There's a comment about DeletePagesByOfflineId but it's
fine to leave it for another patch.I haven't checked every #include to see if
they're unused anymore... I really hope there's a tool to do this :/

Please fix the compiling error.. it seems you're missing an 'override' when
declaring ClearAll() in _impl.h

And the most important.. I'm not on the OWNER list so hopefully an owner can
drive by and give a LGTM!

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

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:151:
OfflinePageModel::~OfflinePageModel() {
nit:put the braces on the same line as above?

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

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:48: class ClientPolicyController;
can be removed?

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:50: class
OfflinePageStorageManager;
can be removed.

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:138: void
DeletePagesByOfflineId(const std::vector<int64_t>& offline_ids,
I think eventually we want this method in this interface, and I'll remove the
inheritance from OfflinePageStorageManager::Client (that was a temp 'hack' i
guess...). It's fine to remove it for now, maybe we can bring it back in another
patch (if you don't want to get hands dirty on messy code of storage_manager :)

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

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:20: #include "base/optional.h"
this seems not being used in header anymore.

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:202: virtual
ClientPolicyController* GetPolicyController() = 0;
I dont think anyone is using this on the interface, so maybe move to impl-only?

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:205: virtual bool is_loaded()
const = 0;
same with above^

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.h (left):

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:134: void
DeletePageByOfflineId(int64_t offline_id,
Why is this removed? I think this is some kind of variant of the
DeletePagesByOfflineId and probably fine to be removed, but just want to make
sure if it's not used by other code.

[dougarnett, 2016-05-26 16:03:21]

dougarnett@chromium.org changed reviewers:
+ dewittj@chromium.org, fgorski@chromium.org

[dougarnett, 2016-05-26 16:03:22]

Thanks for the first pass Yafei.

Justin or Filip, are either of you available to review this interface refactor?

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

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:151:
OfflinePageModel::~OfflinePageModel() {
On 2016/05/26 01:26:48, romax wrote:
> nit:put the braces on the same line as above?

Done.

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

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:48: class ClientPolicyController;
On 2016/05/26 01:26:48, romax wrote:
> can be removed?

Done.

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:50: class
OfflinePageStorageManager;
On 2016/05/26 01:26:48, romax wrote:
> can be removed.

Done.

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:138: void
DeletePagesByOfflineId(const std::vector<int64_t>& offline_ids,
On 2016/05/26 01:26:48, romax wrote:
> I think eventually we want this method in this interface, and I'll remove the
> inheritance from OfflinePageStorageManager::Client (that was a temp 'hack' i
> guess...). It's fine to remove it for now, maybe we can bring it back in
another
> patch (if you don't want to get hands dirty on messy code of storage_manager
:)

Added TODO on you to pull this up.

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

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:20: #include "base/optional.h"
On 2016/05/26 01:26:48, romax wrote:
> this seems not being used in header anymore.

Done.

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:202: virtual
ClientPolicyController* GetPolicyController() = 0;
On 2016/05/26 01:26:48, romax wrote:
> I dont think anyone is using this on the interface, so maybe move to
impl-only?

I don't see any current uses. Can I get a "2nd" from another reviewer on the
motion to remove it?

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:205: virtual bool is_loaded()
const = 0;
On 2016/05/26 01:26:48, romax wrote:
> same with above^

Did find tests using this. Would like to get it out of this interface though -
but goes a bit farther than hoping to do - to just get split landed for now.

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.h (left):

https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:134: void
DeletePageByOfflineId(int64_t offline_id,
On 2016/05/26 01:26:48, romax wrote:
> Why is this removed? I think this is some kind of variant of the
> DeletePagesByOfflineId and probably fine to be removed, but just want to make
> sure if it's not used by other code.

I was not able to find any implementation nor usage of it. Maybe another
reviewer will know some history on it.

[fgorski, 2016-05-26 16:04:45]

I'll look tomorrow.
On Thu, May 26, 2016 at 9:03 AM <dougarnett@chromium.org> wrote:

> Thanks for the first pass Yafei.
>
> Justin or Filip, are either of you available to review this interface
> refactor?
>
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> File components/offline_pages/offline_page_model.cc (left):
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.cc:151:
> OfflinePageModel::~OfflinePageModel() {
> On 2016/05/26 01:26:48, romax wrote:
> > nit:put the braces on the same line as above?
>
> Done.
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> File components/offline_pages/offline_page_model.h (left):
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.h:48: class
> ClientPolicyController;
> On 2016/05/26 01:26:48, romax wrote:
> > can be removed?
>
> Done.
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.h:50: class
> OfflinePageStorageManager;
> On 2016/05/26 01:26:48, romax wrote:
> > can be removed.
>
> Done.
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.h:138: void
> DeletePagesByOfflineId(const std::vector<int64_t>& offline_ids,
> On 2016/05/26 01:26:48, romax wrote:
> > I think eventually we want this method in this interface, and I'll
> remove the
> > inheritance from OfflinePageStorageManager::Client (that was a temp
> 'hack' i
> > guess...). It's fine to remove it for now, maybe we can bring it back
> in another
> > patch (if you don't want to get hands dirty on messy code of
> storage_manager :)
>
> Added TODO on you to pull this up.
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> File components/offline_pages/offline_page_model.h (right):
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.h:20: #include
> "base/optional.h"
> On 2016/05/26 01:26:48, romax wrote:
> > this seems not being used in header anymore.
>
> Done.
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.h:202: virtual
> ClientPolicyController* GetPolicyController() = 0;
> On 2016/05/26 01:26:48, romax wrote:
> > I dont think anyone is using this on the interface, so maybe move to
> impl-only?
>
> I don't see any current uses. Can I get a "2nd" from another reviewer on
> the motion to remove it?
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model.h:205: virtual bool
> is_loaded() const = 0;
> On 2016/05/26 01:26:48, romax wrote:
> > same with above^
>
> Did find tests using this. Would like to get it out of this interface
> though - but goes a bit farther than hoping to do - to just get split
> landed for now.
>
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> File components/offline_pages/offline_page_model_impl.h (left):
>
>
>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model_impl.h:134: void
> DeletePageByOfflineId(int64_t offline_id,
> On 2016/05/26 01:26:48, romax wrote:
> > Why is this removed? I think this is some kind of variant of the
> > DeletePagesByOfflineId and probably fine to be removed, but just want
> to make
> > sure if it's not used by other code.
>
> I was not able to find any implementation nor usage of it. Maybe another
> reviewer will know some history on it.
>
> https://codereview.chromium.org/2011763005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-05-26 16:26:51]

Justin or me will look at it.
On Thu, May 26, 2016 at 9:04 AM Filip Gorski <fgorski@chromium.org> wrote:

> I'll look tomorrow.
> On Thu, May 26, 2016 at 9:03 AM <dougarnett@chromium.org> wrote:
>
>> Thanks for the first pass Yafei.
>>
>> Justin or Filip, are either of you available to review this interface
>> refactor?
>>
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> File components/offline_pages/offline_page_model.cc (left):
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.cc:151:
>> OfflinePageModel::~OfflinePageModel() {
>> On 2016/05/26 01:26:48, romax wrote:
>> > nit:put the braces on the same line as above?
>>
>> Done.
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> File components/offline_pages/offline_page_model.h (left):
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.h:48: class
>> ClientPolicyController;
>> On 2016/05/26 01:26:48, romax wrote:
>> > can be removed?
>>
>> Done.
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.h:50: class
>> OfflinePageStorageManager;
>> On 2016/05/26 01:26:48, romax wrote:
>> > can be removed.
>>
>> Done.
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.h:138: void
>> DeletePagesByOfflineId(const std::vector<int64_t>& offline_ids,
>> On 2016/05/26 01:26:48, romax wrote:
>> > I think eventually we want this method in this interface, and I'll
>> remove the
>> > inheritance from OfflinePageStorageManager::Client (that was a temp
>> 'hack' i
>> > guess...). It's fine to remove it for now, maybe we can bring it back
>> in another
>> > patch (if you don't want to get hands dirty on messy code of
>> storage_manager :)
>>
>> Added TODO on you to pull this up.
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> File components/offline_pages/offline_page_model.h (right):
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.h:20: #include
>> "base/optional.h"
>> On 2016/05/26 01:26:48, romax wrote:
>> > this seems not being used in header anymore.
>>
>> Done.
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.h:202: virtual
>> ClientPolicyController* GetPolicyController() = 0;
>> On 2016/05/26 01:26:48, romax wrote:
>> > I dont think anyone is using this on the interface, so maybe move to
>> impl-only?
>>
>> I don't see any current uses. Can I get a "2nd" from another reviewer on
>> the motion to remove it?
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model.h:205: virtual bool
>> is_loaded() const = 0;
>> On 2016/05/26 01:26:48, romax wrote:
>> > same with above^
>>
>> Did find tests using this. Would like to get it out of this interface
>> though - but goes a bit farther than hoping to do - to just get split
>> landed for now.
>>
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> File components/offline_pages/offline_page_model_impl.h (left):
>>
>>
>>
https://codereview.chromium.org/2011763005/diff/1/components/offline_pages/of...
>> components/offline_pages/offline_page_model_impl.h:134: void
>> DeletePageByOfflineId(int64_t offline_id,
>> On 2016/05/26 01:26:48, romax wrote:
>> > Why is this removed? I think this is some kind of variant of the
>> > DeletePagesByOfflineId and probably fine to be removed, but just want
>> to make
>> > sure if it's not used by other code.
>>
>> I was not able to find any implementation nor usage of it. Maybe another
>> reviewer will know some history on it.
>>
>> https://codereview.chromium.org/2011763005/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dewittj, 2016-05-26 16:38:44]

A good start!  I think you should do a little bit more in this patch. (see
comments)

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

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:51: class OfflinePageModel :
public KeyedService,
I think that this inheritance is wrong.  OfflinePageModel should be a pure
interface - OffilnePageModelImpl should implement KeyedService and
SupportsUserData.  If that requires extra API methods that's fine with me.

I think we can also get rid of OfflinePageStorageManager::Client altogether now
too.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:75: using
CheckPagesExistOfflineResult =
these types can now just be declared here, and we can nuke offline_page_types.h

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:85: static bool CanSavePage(const
GURL& url);
Not sure that this belongs here now.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:87: OfflinePageModel();
OfflinePageModel() = default;

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:88: ~OfflinePageModel() override;
If we make this just an interface, this can become
 virtual ~OfflinePageModel() = default;

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:105: // TODO(romax): Pull this
interface up from OfflinePageStorageManager::Client
Probably should just do that in this refactor.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:135: // on the internal state of
the model.#include <algorithm>
copy/paste error?

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:200: virtual bool is_loaded()
const = 0;
Since there will only be tests for the impl, this can be moved to the subclass
now.

Although I can't find any usage of this function.  Let's just nuke it!

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

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:209:
AddHistogramSuffix(offline_page_item.client_id,
I'd avoid reformatting code that is unchanged.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:601:
AddHistogramSuffix(offline_page.client_id, "OfflinePages.PageSize")
Avoid reformatting unchanged code

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:715:
AddHistogramSuffix(client_id,
Same, this code hasn't changed, so don't reformat it.

[Dmitry Titov, 2016-05-26 18:27:28]

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

[Dmitry Titov, 2016-05-26 18:27:29]

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

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:203:
DISALLOW_COPY_AND_ASSIGN(OfflinePageModel);
Don't think this is needed for pure abstract interfaces that this will become.

[dougarnett, 2016-05-26 20:50:34]

Moved inheritance and addressed some other comments.
Opened bug to track and follow up on clean up ideas.

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

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:51: class OfflinePageModel :
public KeyedService,
On 2016/05/26 16:38:43, dewittj wrote:
> I think that this inheritance is wrong.  OfflinePageModel should be a pure
> interface - OffilnePageModelImpl should implement KeyedService and
> SupportsUserData.  If that requires extra API methods that's fine with me.
> 
> I think we can also get rid of OfflinePageStorageManager::Client altogether
now
> too.

Moved inheritances to impl. Have TODO wrt StorageManager subinterface.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:75: using
CheckPagesExistOfflineResult =
On 2016/05/26 16:38:43, dewittj wrote:
> these types can now just be declared here, and we can nuke
offline_page_types.h

captured in https://bugs.chromium.org/p/chromium/issues/detail?id=615165

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:85: static bool CanSavePage(const
GURL& url);
On 2016/05/26 16:38:43, dewittj wrote:
> Not sure that this belongs here now.

captured in https://bugs.chromium.org/p/chromium/issues/detail?id=615165

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:87: OfflinePageModel();
On 2016/05/26 16:38:43, dewittj wrote:
> OfflinePageModel() = default;

Done.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:88: ~OfflinePageModel() override;
On 2016/05/26 16:38:43, dewittj wrote:
> If we make this just an interface, this can become
>  virtual ~OfflinePageModel() = default;

Done.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:105: // TODO(romax): Pull this
interface up from OfflinePageStorageManager::Client
On 2016/05/26 16:38:43, dewittj wrote:
> Probably should just do that in this refactor.

captured in https://bugs.chromium.org/p/chromium/issues/detail?id=615165

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:135: // on the internal state of
the model.#include <algorithm>
On 2016/05/26 16:38:43, dewittj wrote:
> copy/paste error?

Done.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:200: virtual bool is_loaded()
const = 0;
On 2016/05/26 16:38:43, dewittj wrote:
> Since there will only be tests for the impl, this can be moved to the subclass
> now.
> 
> Although I can't find any usage of this function.  Let's just nuke it!

Will fix comment. Removing it captured in
https://bugs.chromium.org/p/chromium/issues/detail?id=615165

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:203:
DISALLOW_COPY_AND_ASSIGN(OfflinePageModel);
On 2016/05/26 18:27:29, Dmitry Titov wrote:
> Don't think this is needed for pure abstract interfaces that this will become.

Done.

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

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:209:
AddHistogramSuffix(offline_page_item.client_id,
On 2016/05/26 16:38:43, dewittj wrote:
> I'd avoid reformatting code that is unchanged.

Wasn't my intention - fallout of moving to new file and running git cl format.
If important, I can put back as last update before landing (so I don't run git
cl format again and mess it up).

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:601:
AddHistogramSuffix(offline_page.client_id, "OfflinePages.PageSize")
On 2016/05/26 16:38:43, dewittj wrote:
> Avoid reformatting unchanged code

Acknowledged.

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:715:
AddHistogramSuffix(client_id,
On 2016/05/26 16:38:43, dewittj wrote:
> Same, this code hasn't changed, so don't reformat it.

Acknowledged.

[dougarnett, 2016-05-26 21:26:48]

On 2016/05/26 20:50:34, dougarnett wrote:
> Moved inheritance and addressed some other comments.
> Opened bug to track and follow up on clean up ideas.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> File components/offline_pages/offline_page_model.h (right):
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:51: class OfflinePageModel :
> public KeyedService,
> On 2016/05/26 16:38:43, dewittj wrote:
> > I think that this inheritance is wrong.  OfflinePageModel should be a pure
> > interface - OffilnePageModelImpl should implement KeyedService and
> > SupportsUserData.  If that requires extra API methods that's fine with me.
> > 
> > I think we can also get rid of OfflinePageStorageManager::Client altogether
> now
> > too.
> 
> Moved inheritances to impl. Have TODO wrt StorageManager subinterface.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:75: using
> CheckPagesExistOfflineResult =
> On 2016/05/26 16:38:43, dewittj wrote:
> > these types can now just be declared here, and we can nuke
> offline_page_types.h
> 
> captured in https://bugs.chromium.org/p/chromium/issues/detail?id=615165
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:85: static bool
CanSavePage(const
> GURL& url);
> On 2016/05/26 16:38:43, dewittj wrote:
> > Not sure that this belongs here now.
> 
> captured in https://bugs.chromium.org/p/chromium/issues/detail?id=615165
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:87: OfflinePageModel();
> On 2016/05/26 16:38:43, dewittj wrote:
> > OfflinePageModel() = default;
> 
> Done.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:88: ~OfflinePageModel()
override;
> On 2016/05/26 16:38:43, dewittj wrote:
> > If we make this just an interface, this can become
> >  virtual ~OfflinePageModel() = default;
> 
> Done.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:105: // TODO(romax): Pull this
> interface up from OfflinePageStorageManager::Client
> On 2016/05/26 16:38:43, dewittj wrote:
> > Probably should just do that in this refactor.
> 
> captured in https://bugs.chromium.org/p/chromium/issues/detail?id=615165
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:135: // on the internal state of
> the model.#include <algorithm>
> On 2016/05/26 16:38:43, dewittj wrote:
> > copy/paste error?
> 
> Done.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:200: virtual bool is_loaded()
> const = 0;
> On 2016/05/26 16:38:43, dewittj wrote:
> > Since there will only be tests for the impl, this can be moved to the
subclass
> > now.
> > 
> > Although I can't find any usage of this function.  Let's just nuke it!
> 
> Will fix comment. Removing it captured in
> https://bugs.chromium.org/p/chromium/issues/detail?id=615165
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model.h:203:
> DISALLOW_COPY_AND_ASSIGN(OfflinePageModel);
> On 2016/05/26 18:27:29, Dmitry Titov wrote:
> > Don't think this is needed for pure abstract interfaces that this will
become.
> 
> Done.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> File components/offline_pages/offline_page_model_impl.cc (right):
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model_impl.cc:209:
> AddHistogramSuffix(offline_page_item.client_id,
> On 2016/05/26 16:38:43, dewittj wrote:
> > I'd avoid reformatting code that is unchanged.
> 
> Wasn't my intention - fallout of moving to new file and running git cl format.
> If important, I can put back as last update before landing (so I don't run git
> cl format again and mess it up).
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model_impl.cc:601:
> AddHistogramSuffix(offline_page.client_id, "OfflinePages.PageSize")
> On 2016/05/26 16:38:43, dewittj wrote:
> > Avoid reformatting unchanged code
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model_impl.cc:715:
> AddHistogramSuffix(client_id,
> On 2016/05/26 16:38:43, dewittj wrote:
> > Same, this code hasn't changed, so don't reformat it.
> 
> Acknowledged.

Er, there is some more work to do yet to get the moved inheritance working.

[dougarnett, 2016-05-27 15:21:04]

Description was changed from

==========
Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static
members and methods
were interesting deal with and we might consider better location/resolution to
follow-up on.

BUG=614864
==========

to

==========
Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static
members and methods
were interesting deal with and we might consider better location/resolution to
follow-up on.

BUG=614864,610824
==========

[dougarnett, 2016-05-27 15:22:29]

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

https://codereview.chromium.org/2011763005/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:51: class OfflinePageModel :
public KeyedService,
On 2016/05/26 20:50:34, dougarnett wrote:
> On 2016/05/26 16:38:43, dewittj wrote:
> > I think that this inheritance is wrong.  OfflinePageModel should be a pure
> > interface - OffilnePageModelImpl should implement KeyedService and
> > SupportsUserData.  If that requires extra API methods that's fine with me.
> > 
> > I think we can also get rid of OfflinePageStorageManager::Client altogether
> now
> > too.
> 
> Moved inheritances to impl. Have TODO wrt StorageManager subinterface.

I put StorageManager::Client and SupportsUserData back on this interface class
for now. 

For StorageManager::Client, I think we get rid of it in a follow-up CL that
reworks the mocking for the tests which motivated it in first place (nominating
romax to that). 

For SupportsUserData, I am not sure what is best to do. It only seems to be used
for one key for bridge instance so one idea is to add more specific
SetBridge/GetBridge interface here. Another is to add some GetUserData()
interface to expose the Impl implementation of it. Unless there is some simple
recommendation though, I wonder if this should be a follow-up as well.

[dewittj, 2016-05-27 20:49:25]

lgtm, let's get started on those TODOs :)

[dougarnett, 2016-05-31 17:25:03]

Yafei, I merged in your recent changes, can you take a look at the merge?

[romax, 2016-05-31 18:21:30]

On 2016/05/31 17:25:03, dougarnett wrote:
> Yafei, I merged in your recent changes, can you take a look at the merge?

sorry it was in the wrong bucket of my inbox so i missed it...
The merge looks good to me, thanks for cleaning up the messy part!

[dougarnett, 2016-05-31 18:23:12]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-05-31 18:23:12]

The patchset sent to the CQ was uploaded after l-g-t-m from romax@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2011763005/#ps160001
(title: "Fix to take ExpirePages defn out of model i/f as StorageManager defines
it currently")

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

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

[commit-bot: I haz the power, 2016-05-31 19:07:20]

Description was changed from

==========
Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static
members and methods
were interesting deal with and we might consider better location/resolution to
follow-up on.

BUG=614864,610824
==========

to

==========
Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static
members and methods
were interesting deal with and we might consider better location/resolution to
follow-up on.

BUG=614864,610824
==========

[commit-bot: I haz the power, 2016-05-31 19:07:22]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-31 19:08:27]

Description was changed from

==========
Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static
members and methods
were interesting deal with and we might consider better location/resolution to
follow-up on.

BUG=614864,610824
==========

to

==========
Splits the OfflinePageModel into and interface and and implementation class.

Tried to make mostly straightforward split for this iteration. Some static
members and methods
were interesting deal with and we might consider better location/resolution to
follow-up on.

BUG=614864,610824

Committed: https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb
Cr-Commit-Position: refs/heads/master@{#396885}
==========

[commit-bot: I haz the power, 2016-05-31 19:08:29]

Patchset 9 (id:??) landed as
https://crrev.com/76f2e5a378d9d1ab6daf69cb77723adf6d2c40eb
Cr-Commit-Position: refs/heads/master@{#396885}