Componentize Reading List

Componentize Reading List

This fixes a dependency problem from components/browser_sync.
Also enable reading_list sync by default.

BUG=661668
Committed: https://crrev.com/5abaf23de0c7c8fbc8c10dab0280b0a60f464c3a
Cr-Commit-Position: refs/heads/master@{#433905}

[Olivier, 2016-11-21 14:28:29]

Patchset #1 (id:1) has been deleted

[Olivier, 2016-11-21 14:31:26]

Patchset #1 (id:20001) has been deleted

[Olivier, 2016-11-21 14:31:50]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org, jif@chromium.org, sdefresne@chromium.org

[sdefresne, 2016-11-21 14:48:31]

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
File components/reading_list/BUILD.gn (right):

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/BUILD.gn:33: "//components/reading_list/proto",
This should go to public_deps

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/BUILD.gn:54: "//components/reading_list/proto",
This is only required here if directly included by one of the four .cc files.

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
File components/reading_list/reading_list_switches.cc (right):

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/reading_list_switches.cc:23: }
}  // namespace switches
}  // namespace reading_list

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
File components/reading_list/reading_list_switches.h (right):

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/reading_list_switches.h:15: }
}  // namespace switches
}  // namespace reading_list

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/BUILD.gn (right):

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/BUILD.gn:26: "//components/reading_list",
Should be in public_deps if any of the header file include one of the
reading_list proto header.

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/BUILD.gn:27: "//components/reading_list/proto",
Only required here if one of the .cc or .h file include the proto generated
header.

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/BUILD.gn:52: "//components/reading_list/proto",
ditto

[Olivier, 2016-11-21 15:46:41]

Done and added forwarder to avoid breaking compilation.

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
File components/reading_list/BUILD.gn (right):

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/BUILD.gn:33: "//components/reading_list/proto",
On 2016/11/21 14:48:31, sdefresne wrote:
> This should go to public_deps

Done.

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/BUILD.gn:54: "//components/reading_list/proto",
On 2016/11/21 14:48:31, sdefresne wrote:
> This is only required here if directly included by one of the four .cc files.

Done.

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
File components/reading_list/reading_list_switches.cc (right):

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/reading_list_switches.cc:23: }
On 2016/11/21 14:48:31, sdefresne wrote:
> }  // namespace switches
> }  // namespace reading_list

Done.

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
File components/reading_list/reading_list_switches.h (right):

https://codereview.chromium.org/2514333003/diff/40001/components/reading_list...
components/reading_list/reading_list_switches.h:15: }
On 2016/11/21 14:48:31, sdefresne wrote:
> }  // namespace switches
> }  // namespace reading_list

Done.

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/BUILD.gn (right):

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/BUILD.gn:26: "//components/reading_list",
On 2016/11/21 14:48:31, sdefresne wrote:
> Should be in public_deps if any of the header file include one of the
> reading_list proto header.

I don't think this is the case

$ git grep -l reading_list.pb.h
components/reading_list/reading_list_entry.cc
components/reading_list/reading_list_entry_unittest.cc
components/reading_list/reading_list_store.cc

https://codereview.chromium.org/2514333003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/BUILD.gn:27: "//components/reading_list/proto",
On 2016/11/21 14:48:31, sdefresne wrote:
> Only required here if one of the .cc or .h file include the proto generated
> header.

Done.

[sdefresne, 2016-11-21 15:51:48]

lgtm if downstream builds cleanly with this CL applied

[jif-google, 2016-11-21 17:49:14]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2016-11-21 17:49:15]

lgtm

[pavely, 2016-11-22 00:45:02]

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

[pavely, 2016-11-22 00:45:02]

lgtm

[Olivier, 2016-11-22 06:31:46]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-22 06:32:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 06:39:50]

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

[commit-bot: I haz the power, 2016-11-22 06:39:51]

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

[Olivier, 2016-11-22 06:51:19]

olivierrobin@chromium.org changed reviewers:
+ mef@chromium.org

[Olivier, 2016-11-22 06:51:20]

+mef as net owner as this add dependency to net.

[mef, 2016-11-22 15:36:11]

On 2016/11/22 06:51:20, Olivier Robin wrote:
> +mef as net owner as this add dependency to net.

Adding dependency to net lgtm.

[Olivier, 2016-11-22 16:31:09]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-22 16:31:09]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
jif@google.com, sdefresne@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2514333003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-22 16:31:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 17:35:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479832269107930,
"parent_rev": "33a0363b2910b444d4ae0cfb8b99e195960d4ebf", "commit_rev":
"ee19268e15c12feca2da0f4ad5b2ac76bef7f396"}

[commit-bot: I haz the power, 2016-11-22 17:35:31]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-11-22 17:40:30]

Description was changed from

==========
Componentize Reading List

This fixes a dependency problem from components/browser_sync.
Also enable reading_list sync by default.

BUG=661668
==========

to

==========
Componentize Reading List

This fixes a dependency problem from components/browser_sync.
Also enable reading_list sync by default.

BUG=661668

Committed: https://crrev.com/5abaf23de0c7c8fbc8c10dab0280b0a60f464c3a
Cr-Commit-Position: refs/heads/master@{#433905}
==========

[commit-bot: I haz the power, 2016-11-22 17:40:31]

Patchset 4 (id:??) landed as
https://crrev.com/5abaf23de0c7c8fbc8c10dab0280b0a60f464c3a
Cr-Commit-Position: refs/heads/master@{#433905}