[Offline pages] Extracting callbacks from the Offline Page Model

[Offline pages] Extracting callbacks from the Offline Page Model

This patch:
* Moves the callbacks and enums from offline page model
* Updates all of the consumers of the callbacks and enums

BUG=610824
Committed: https://crrev.com/fe8a72da89b1941cccdd54a0da548540360286a6
Cr-Commit-Position: refs/heads/master@{#393044}

[fgorski, 2016-05-10 21:20:22]

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

[fgorski, 2016-05-10 21:20:23]

PTAL.

Question I have to you is: should I also add the _types.h in every file that
consumes this thing, or wait for the model to be ported, so that everything goes
back to norm...

Also to prevent churn in the remote areas depending on: OPM::CallbackType, I can
probably alias the callback type inside of the Model (as a temp solution)

[dewittj, 2016-05-10 22:38:28]

yes, I think we should IWYU, even if it's temporary.

[dewittj, 2016-05-10 22:39:17]

On 2016/05/10 22:38:28, dewittj wrote:
> yes, I think we should IWYU, even if it's temporary.

although, if you alias the types in Model, then the clients wouldn't need to
include _types.h any more, so that could result in a much tidier patch.

[romax, 2016-05-10 22:39:38]

lgtm

1. I don't really know why we have to include _types.h in every file... Seems
like every file which would use anything declared in _types.h would include at
least one of the header files which already includes the _types.h? (not sure) SO
for now I guess it's fine?
2. I think it's great to have aliases inside Model, but eventually those should
be removed after everyone gets to know that callbacks/enums have been extracted.

[msramek, 2016-05-11 11:38:34]

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

[msramek, 2016-05-11 11:38:35]

browsing_data/ drive-by LGTM

https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gypi
File components/offline_pages.gypi (right):

https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gy...
components/offline_pages.gypi:26: 'offline_pages/client_policy_controller.h',
Spaces instead of tabs please :)

[fgorski, 2016-05-11 18:39:05]

OK, updated the following way:
* Aliased several types that are used outside of the model to minimize the
impact of the change
* Included _types everywhere in the unit tests

Yafei, I think putting callbacks back into OPM makes sense, once it is an
interface.

Accidental inclusion of _types.h is not a good excuse not to be explicit per
Justin. Please talk to him today to learn more.

https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gypi
File components/offline_pages.gypi (right):

https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gy...
components/offline_pages.gypi:26: 'offline_pages/client_policy_controller.h',
On 2016/05/11 11:38:34, msramek (OOO until May 11) wrote:
> Spaces instead of tabs please :)

Yeah, notice that that is exactly what I was trying to do... I added spaces
where they needed to be, but my IDE (hhhm VIM) didn't show me that there is
still a tab at the beginning of the line.

[dewittj, 2016-05-11 18:42:56]

lgtm

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

https://codereview.chromium.org/1968563003/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:12: #include
"components/offline_pages/offline_page_types.h"
you could revert this file, if you like.

[fgorski, 2016-05-11 19:14:20]

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

https://codereview.chromium.org/1968563003/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:12: #include
"components/offline_pages/offline_page_types.h"
On 2016/05/11 18:42:56, dewittj wrote:
> you could revert this file, if you like.

I did it on purpose to prevent another aliasing of the same types.

[fgorski, 2016-05-11 19:24:45]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-11 19:24:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, romax@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1968563003/#ps40001
(title: "Adding a missing fix to gypi file")

[commit-bot: I haz the power, 2016-05-11 19:25:16]

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

[romax, 2016-05-11 20:00:34]

On 2016/05/11 18:39:05, fgorski wrote:
> OK, updated the following way:
> * Aliased several types that are used outside of the model to minimize the
> impact of the change
> * Included _types everywhere in the unit tests
> 
> Yafei, I think putting callbacks back into OPM makes sense, once it is an
> interface.
> 
> Accidental inclusion of _types.h is not a good excuse not to be explicit per
> Justin. Please talk to him today to learn more.
> 
>
https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gypi
> File components/offline_pages.gypi (right):
> 
>
https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gy...
> components/offline_pages.gypi:26: 'offline_pages/client_policy_controller.h',
> On 2016/05/11 11:38:34, msramek (OOO until May 11) wrote:
> > Spaces instead of tabs please :)
> 
> Yeah, notice that that is exactly what I was trying to do... I added spaces
> where they needed to be, but my IDE (hhhm VIM) didn't show me that there is
> still a tab at the beginning of the line.

Yeah I hit send before I saw Justin's comments...
BTW when building locally I'm seeing you're missing #include "base/optional.h"
in the _types.h...
However it's not showing in try bots? Why is that?

[fgorski, 2016-05-11 20:17:13]

On 2016/05/11 20:00:34, romax wrote:
> On 2016/05/11 18:39:05, fgorski wrote:
> > OK, updated the following way:
> > * Aliased several types that are used outside of the model to minimize the
> > impact of the change
> > * Included _types everywhere in the unit tests
> > 
> > Yafei, I think putting callbacks back into OPM makes sense, once it is an
> > interface.
> > 
> > Accidental inclusion of _types.h is not a good excuse not to be explicit per
> > Justin. Please talk to him today to learn more.
> > 
> >
>
https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gypi
> > File components/offline_pages.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1968563003/diff/1/components/offline_pages.gy...
> > components/offline_pages.gypi:26:
'offline_pages/client_policy_controller.h',
> > On 2016/05/11 11:38:34, msramek (OOO until May 11) wrote:
> > > Spaces instead of tabs please :)
> > 
> > Yeah, notice that that is exactly what I was trying to do... I added spaces
> > where they needed to be, but my IDE (hhhm VIM) didn't show me that there is
> > still a tab at the beginning of the line.
> 
> Yeah I hit send before I saw Justin's comments...
> BTW when building locally I'm seeing you're missing #include "base/optional.h"
> in the _types.h...
> However it's not showing in try bots? Why is that?

I think this is because of this being included through _model and that is where
we already have an optional definition.
I don't want to stop the checkin here. Let's update in a separate patch.

[commit-bot: I haz the power, 2016-05-11 20:25:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-11 20:26:04]

Description was changed from

==========
[Offline pages] Extracting callbacks from the Offline Page Model

This patch:
* Moves the callbacks and enums from offline page model
* Updates all of the consumers of the callbacks and enums

BUG=610824
==========

to

==========
[Offline pages] Extracting callbacks from the Offline Page Model

This patch:
* Moves the callbacks and enums from offline page model
* Updates all of the consumers of the callbacks and enums

BUG=610824

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

[commit-bot: I haz the power, 2016-05-11 20:26:05]

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