Componentize //chrome/browser/offline_pages

Componentize //chrome/browser/offline_pages

Componentize offline_page_metadata_store_impl.[cc/h], unittest and offline_pages.proto file.

Updated BUILD.gn and corresponding .gypi files for the same.

BUG=507284
Committed: https://crrev.com/faf6421d12a9499b3a3d5266b5ceb33ea81ce1d2
Cr-Commit-Position: refs/heads/master@{#339852}

[Abhishek, 2015-07-13 12:42:06]

abhishek.a21@samsung.com changed reviewers:
+ r.kasibhatla@samsung.com

[Abhishek, 2015-07-13 12:45:53]

PTAL!

[RaviKasibhatla, 2015-07-13 13:21:31]

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

[RaviKasibhatla, 2015-07-13 13:21:32]

https://codereview.chromium.org/1226173004/diff/20001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1226173004/diff/20001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:162:
'browser/offline_pages/offline_page_mhtml_archiver_unittest.cc',
Are you intending to move all mhtml related files in later CL?

https://codereview.chromium.org/1226173004/diff/20001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1226173004/diff/20001/components/offline_page...
components/offline_pages/BUILD.gn:14: "offline_page_model.h",
I don't see the newly moved files listed in source as you have done in
offline_pages.gypi
All other files are listed in both the files.

[Abhishek, 2015-07-13 14:04:03]

PTAL at patch set #3!

https://codereview.chromium.org/1226173004/diff/20001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1226173004/diff/20001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:162:
'browser/offline_pages/offline_page_mhtml_archiver_unittest.cc',
On 2015/07/13 13:21:32, RaviKasibhatla wrote:
> Are you intending to move all mhtml related files in later CL?

Yeah!

https://codereview.chromium.org/1226173004/diff/20001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1226173004/diff/20001/components/offline_page...
components/offline_pages/BUILD.gn:14: "offline_page_model.h",
On 2015/07/13 13:21:32, RaviKasibhatla wrote:
> I don't see the newly moved files listed in source as you have done in
> offline_pages.gypi
> All other files are listed in both the files.

Thanks for your comment!
Done

[RaviKasibhatla, 2015-07-13 14:44:19]

peer review looks good to me with one nit.

As I am not sure the model of gyp deps being followed in the components/ folder,
@blundell might be able to suggest the correct way for the comment I have left.

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
File components/offline_pages.gypi (right):

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
components/offline_pages.gypi:36: },
For the GN build system, we are following a deps model system i.e. offline_pages
target depends on proto target (defined in offline_pages/proto/BUILD.gn).

However, for gyp model, we are directly including the .proto source files and
the generation scripts. This is different from the existing model
(chrome_browser.gypi) which was also following the deps model.

Is this something intentionally intended?

[Abhishek, 2015-07-14 04:59:29]

abhishek.a21@samsung.com changed reviewers:
+ blundell@chromium.org

[blundell, 2015-07-15 09:44:34]

Thanks!

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
File components/offline_pages.gypi (right):

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
components/offline_pages.gypi:36: },
On 2015/07/13 14:44:19, RaviKasibhatla wrote:
> For the GN build system, we are following a deps model system i.e.
offline_pages
> target depends on proto target (defined in offline_pages/proto/BUILD.gn).
> 
> However, for gyp model, we are directly including the .proto source files and
> the generation scripts. This is different from the existing model
> (chrome_browser.gypi) which was also following the deps model.
> 
> Is this something intentionally intended?

+1 that the target structure should be parallel for consistency.

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
File components/offline_pages/DEPS (right):

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
components/offline_pages/DEPS:3: "+components/leveldb_proto",
Add a dependency for this in gyp and gn?

[Abhishek, 2015-07-15 14:24:22]

PTAL #4!

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
File components/offline_pages.gypi (right):

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
components/offline_pages.gypi:36: },
On 2015/07/15 09:44:34, blundell wrote:
> On 2015/07/13 14:44:19, RaviKasibhatla wrote:
> > For the GN build system, we are following a deps model system i.e.
> offline_pages
> > target depends on proto target (defined in offline_pages/proto/BUILD.gn).
> > 
> > However, for gyp model, we are directly including the .proto source files
and
> > the generation scripts. This is different from the existing model
> > (chrome_browser.gypi) which was also following the deps model.
> > 
> > Is this something intentionally intended?
> 
> +1 that the target structure should be parallel for consistency.

Done.
Yeah! I've followed the same deps model system for both the build.

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
File components/offline_pages/DEPS (right):

https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
components/offline_pages/DEPS:3: "+components/leveldb_proto",
On 2015/07/15 09:44:34, blundell wrote:
> Add a dependency for this in gyp and gn?

Done.

[blundell, 2015-07-15 14:27:13]

On 2015/07/15 14:24:22, Abhishek wrote:
> PTAL #4!
> 
>
https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
> File components/offline_pages.gypi (right):
> 
>
https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
> components/offline_pages.gypi:36: },
> On 2015/07/15 09:44:34, blundell wrote:
> > On 2015/07/13 14:44:19, RaviKasibhatla wrote:
> > > For the GN build system, we are following a deps model system i.e.
> > offline_pages
> > > target depends on proto target (defined in offline_pages/proto/BUILD.gn).
> > > 
> > > However, for gyp model, we are directly including the .proto source files
> and
> > > the generation scripts. This is different from the existing model
> > > (chrome_browser.gypi) which was also following the deps model.
> > > 
> > > Is this something intentionally intended?
> > 
> > +1 that the target structure should be parallel for consistency.
> 
> Done.
> Yeah! I've followed the same deps model system for both the build.
> 
>
https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
> File components/offline_pages/DEPS (right):
> 
>
https://codereview.chromium.org/1226173004/diff/40001/components/offline_page...
> components/offline_pages/DEPS:3: "+components/leveldb_proto",
> On 2015/07/15 09:44:34, blundell wrote:
> > Add a dependency for this in gyp and gn?
> 
> Done.

Looks good, except that I still don't see the //components/leveldb_proto
dependency in the buildfiles.

[Abhishek, 2015-07-16 08:26:17]

PTAL #5.
I've updated dependencies!
Thanks!

[blundell, 2015-07-16 08:36:27]

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

[blundell, 2015-07-16 08:36:28]

LGTM, thanks!

+fgorski for an OWNERS review of *offline_pages*

[Abhishek, 2015-07-16 14:32:21]

@fgorski: PTAL!

[fgorski, 2015-07-16 16:32:43]

lgtm

Could you explain why you are making the move, please?

Also, FYI I'll be likely moving the rest of the files to the android specific
location https://codereview.chromium.org/1236403003/

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
components/offline_pages/BUILD.gn:39: "//sync",
Do I need this?

[blundell, 2015-07-16 17:06:48]

On 2015/07/16 16:32:43, fgorski wrote:
> lgtm
> 
> Could you explain why you are making the move, please?
>

The code move is because this code is shared by the iOS port, which in the long
term will not use //src/chrome.
 
> Also, FYI I'll be likely moving the rest of the files to the android specific
> location https://codereview.chromium.org/1236403003/
> 
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> File components/offline_pages/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> components/offline_pages/BUILD.gn:39: "//sync",
> Do I need this?

[fgorski, 2015-07-16 18:29:56]

On 2015/07/16 17:06:48, blundell wrote:
> On 2015/07/16 16:32:43, fgorski wrote:
> > lgtm
> > 
> > Could you explain why you are making the move, please?
> >
> 
> The code move is because this code is shared by the iOS port, which in the
long
> term will not use //src/chrome.
>  
> > Also, FYI I'll be likely moving the rest of the files to the android
specific
> > location https://codereview.chromium.org/1236403003/
> > 
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > File components/offline_pages/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > components/offline_pages/BUILD.gn:39: "//sync",
> > Do I need this?

Well, for now we are limiting the feature to Andoid only, so sharing with iOS
should not be a factor. I am still OK with the move, just seemed strange that we
would do it.

[blundell, 2015-07-16 18:36:05]

blundell@chromium.org changed reviewers:
+ droger@chromium.org

[blundell, 2015-07-16 18:36:05]

+droger@

David, should this code not be getting built on iOS?

[Abhishek, 2015-07-17 07:50:38]

I've updated gn dependencies.

PTAL #6!

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
components/offline_pages/BUILD.gn:39: "//sync",
On 2015/07/16 16:32:43, fgorski wrote:
> Do I need this?
I had added it due to below error:
"gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal error:
'google/protobuf/stubs/common.h' file not
foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal error:
'google/protobuf/stubs/common.h' file not found"

But this can be solved using "//net" as well. So I'm changing it to "//net".

Please correct me if I'm wrong here!

[fgorski, 2015-07-17 17:25:01]

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
components/offline_pages/BUILD.gn:39: "//sync",
On 2015/07/17 07:50:37, Abhishek wrote:
> On 2015/07/16 16:32:43, fgorski wrote:
> > Do I need this?
> I had added it due to below error:
> "gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal error:
> 'google/protobuf/stubs/common.h' file not
> foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal error:
> 'google/protobuf/stubs/common.h' file not found"
> 
> But this can be solved using "//net" as well. So I'm changing it to "//net".
> 
> Please correct me if I'm wrong here!

Did you try taking dependency on the protobuf?
"//components/offline_pages/proto:offline_pages_proto",

[droger, 2015-07-17 17:29:32]

On 2015/07/16 18:36:05, blundell wrote:
> +droger@
> 
> David, should this code not be getting built on iOS?

It seems that we're building it, I don't see it being used at all.
I am not familiar with this feature, so I don't know what is intended.

[fgorski, 2015-07-17 17:39:13]

On 2015/07/17 17:29:32, droger wrote:
> On 2015/07/16 18:36:05, blundell wrote:
> > +droger@
> > 
> > David, should this code not be getting built on iOS?
> 
> It seems that we're building it, I don't see it being used at all.
> I am not familiar with this feature, so I don't know what is intended.

As I said, I have a patch pending where this is only going to be built on
Android. We don't intend to release this feature for iOS right now.

I am still happy with making it compile for iOS, so this patch is useful.

[Abhishek, 2015-07-20 07:24:07]

PTAL #7!

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
components/offline_pages/BUILD.gn:39: "//sync",
On 2015/07/17 17:25:00, fgorski wrote:
> On 2015/07/17 07:50:37, Abhishek wrote:
> > On 2015/07/16 16:32:43, fgorski wrote:
> > > Do I need this?
> > I had added it due to below error:
> > "gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal error:
> > 'google/protobuf/stubs/common.h' file not
> > foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
error:
> > 'google/protobuf/stubs/common.h' file not found"
> > 
> > But this can be solved using "//net" as well. So I'm changing it to "//net".
> > 
> > Please correct me if I'm wrong here!
> 
> Did you try taking dependency on the protobuf?
> "//components/offline_pages/proto:offline_pages_proto",

Thanks!
Done.

[fgorski, 2015-07-20 13:38:47]

On 2015/07/20 07:24:07, Abhishek wrote:
> PTAL #7!
> 
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> File components/offline_pages/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> components/offline_pages/BUILD.gn:39: "//sync",
> On 2015/07/17 17:25:00, fgorski wrote:
> > On 2015/07/17 07:50:37, Abhishek wrote:
> > > On 2015/07/16 16:32:43, fgorski wrote:
> > > > Do I need this?
> > > I had added it due to below error:
> > > "gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal error:
> > > 'google/protobuf/stubs/common.h' file not
> > > foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
> error:
> > > 'google/protobuf/stubs/common.h' file not found"
> > > 
> > > But this can be solved using "//net" as well. So I'm changing it to
"//net".
> > > 
> > > Please correct me if I'm wrong here!
> > 
> > Did you try taking dependency on the protobuf?
> > "//components/offline_pages/proto:offline_pages_proto",
> 
> Thanks!
> Done.

After you make this move, DEPS and OWNERS are the only 2 files in that location,
if I am not mistaken (please sync and check). You should delete them as well.

[fgorski, 2015-07-20 13:39:30]

On 2015/07/20 13:38:47, fgorski wrote:
> On 2015/07/20 07:24:07, Abhishek wrote:
> > PTAL #7!
> > 
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > File components/offline_pages/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > components/offline_pages/BUILD.gn:39: "//sync",
> > On 2015/07/17 17:25:00, fgorski wrote:
> > > On 2015/07/17 07:50:37, Abhishek wrote:
> > > > On 2015/07/16 16:32:43, fgorski wrote:
> > > > > Do I need this?
> > > > I had added it due to below error:
> > > > "gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
error:
> > > > 'google/protobuf/stubs/common.h' file not
> > > > foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
> > error:
> > > > 'google/protobuf/stubs/common.h' file not found"
> > > > 
> > > > But this can be solved using "//net" as well. So I'm changing it to
> "//net".
> > > > 
> > > > Please correct me if I'm wrong here!
> > > 
> > > Did you try taking dependency on the protobuf?
> > > "//components/offline_pages/proto:offline_pages_proto",
> > 
> > Thanks!
> > Done.
> 
> After you make this move, DEPS and OWNERS are the only 2 files in that
location,
> if I am not mistaken (please sync and check). You should delete them as well.

lgtm with the removal of extra files

[Abhishek, 2015-07-20 13:41:43]

The CQ bit was checked by abhishek.a21@samsung.com

[Abhishek, 2015-07-20 13:41:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1226173004/#ps120001
(title: "updated gn dependencies")

[commit-bot: I haz the power, 2015-07-20 13:41:47]

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

[commit-bot: I haz the power, 2015-07-20 13:43:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-20 13:43:13]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Abhishek, 2015-07-20 13:43:38]

On 2015/07/20 13:38:47, fgorski wrote:
> On 2015/07/20 07:24:07, Abhishek wrote:
> > PTAL #7!
> > 
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > File components/offline_pages/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > components/offline_pages/BUILD.gn:39: "//sync",
> > On 2015/07/17 17:25:00, fgorski wrote:
> > > On 2015/07/17 07:50:37, Abhishek wrote:
> > > > On 2015/07/16 16:32:43, fgorski wrote:
> > > > > Do I need this?
> > > > I had added it due to below error:
> > > > "gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
error:
> > > > 'google/protobuf/stubs/common.h' file not
> > > > foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
> > error:
> > > > 'google/protobuf/stubs/common.h' file not found"
> > > > 
> > > > But this can be solved using "//net" as well. So I'm changing it to
> "//net".
> > > > 
> > > > Please correct me if I'm wrong here!
> > > 
> > > Did you try taking dependency on the protobuf?
> > > "//components/offline_pages/proto:offline_pages_proto",
> > 
> > Thanks!
> > Done.
> 
> After you make this move, DEPS and OWNERS are the only 2 files in that
location,
> if I am not mistaken (please sync and check). You should delete them as well.

Sure!
I'll upload itno patch after this.
Thanks for the review!

[Abhishek, 2015-07-21 04:55:08]

On 2015/07/20 13:39:30, fgorski wrote:
> On 2015/07/20 13:38:47, fgorski wrote:
> > On 2015/07/20 07:24:07, Abhishek wrote:
> > > PTAL #7!
> > > 
> > >
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > > File components/offline_pages/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1226173004/diff/80001/components/offline_page...
> > > components/offline_pages/BUILD.gn:39: "//sync",
> > > On 2015/07/17 17:25:00, fgorski wrote:
> > > > On 2015/07/17 07:50:37, Abhishek wrote:
> > > > > On 2015/07/16 16:32:43, fgorski wrote:
> > > > > > Do I need this?
> > > > > I had added it due to below error:
> > > > > "gen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
> error:
> > > > > 'google/protobuf/stubs/common.h' file not
> > > > > foundgen/components/offline_pages/proto/offline_pages.pb.h:9:10: fatal
> > > error:
> > > > > 'google/protobuf/stubs/common.h' file not found"
> > > > > 
> > > > > But this can be solved using "//net" as well. So I'm changing it to
> > "//net".
> > > > > 
> > > > > Please correct me if I'm wrong here!
> > > > 
> > > > Did you try taking dependency on the protobuf?
> > > > "//components/offline_pages/proto:offline_pages_proto",
> > > 
> > > Thanks!
> > > Done.
> > 
> > After you make this move, DEPS and OWNERS are the only 2 files in that
> location,
> > if I am not mistaken (please sync and check). You should delete them as
well.
> 
> lgtm with the removal of extra files

Thanks!

I'll remove those extra file and re-base my patch and upload it again.

[Abhishek, 2015-07-21 09:26:51]

The CQ bit was checked by abhishek.a21@samsung.com

[Abhishek, 2015-07-21 09:26:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1226173004/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-21 09:27:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226173004/140001

[commit-bot: I haz the power, 2015-07-21 09:35:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-21 09:35:21]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Abhishek, 2015-07-21 12:01:43]

abhishek.a21@samsung.com changed reviewers:
+ jochen@chromium.org

[Abhishek, 2015-07-21 12:01:43]

@jochen 
PTAL chrome/browser/BUILD.gn

[jochen (gone - plz use gerrit), 2015-07-21 14:08:54]

lgtm

[Abhishek, 2015-07-22 08:06:29]

The CQ bit was checked by abhishek.a21@samsung.com

[Abhishek, 2015-07-22 08:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1226173004/#ps160001
(title: "updated for android port")

[commit-bot: I haz the power, 2015-07-22 08:07:11]

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

[commit-bot: I haz the power, 2015-07-22 09:11:50]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-22 09:13:07]

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