Splits components/sessions into separate core and content targets.

Renames the main components/sessions target to sessions_content.

This makes it clear that this target depends on //content and lays the
groundwork for a future sessions_ios target.

The files in components/sessions/core/ are moved to a variable so that
they can be shared across targets.

TBR=finnur@chromium.org
TBR=sky@chromium.org
BUG=371476
TEST=None

Committed: https://crrev.com/d16f44e6bb404dcb5b841f231b74da5afd6c5789
Cr-Commit-Position: refs/heads/master@{#301103}

[rohitrao (ping after 24h), 2014-10-23 18:39:06]

rohitrao@chromium.org changed reviewers:
+ blundell@chromium.org, marja@chromium.org

[rohitrao (ping after 24h), 2014-10-23 18:39:08]

rohitrao@chromium.org changed required reviewers:
+ blundell@chromium.org

[rohitrao (ping after 24h), 2014-10-23 18:39:09]

+Colin as the primary reviewer for this CL, since it's all GYP/GN
+marja FYI

+Sylvain and Stuart FYI, if they want to be another set of eyes on this


Non-clobber try jobs were green, and I'm running a set of clobber try jobs now. 
At a high level, I've mashed together the GYP/GN structure from autofill
(separate core/content/ios targets) and policy (supports the component build).

[blundell, 2014-10-24 07:23:14]

I got about halfway through but then realized I'm not understanding something
fundamental:

Why can't we have the core and content targets just be of type "component", and
no "sessions" target? Is it because of this issue of the static function
declared/used in core and defined in content?

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi
File components/sessions.gypi (right):

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi...
components/sessions.gypi:12: 'type': 'none',
I don't understand the use of 'none' here.

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi...
components/sessions.gypi:50: }, {  # component=="static_library"
s/static_library/shared_library

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi...
components/sessions.gypi:70: 'type': 'none',
I don't understand the use of 'none' here.

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi...
components/sessions.gypi:121: 
kill

https://codereview.chromium.org/673843002/diff/60001/components/sessions/BUIL...
File components/sessions/BUILD.gn (right):

https://codereview.chromium.org/673843002/diff/60001/components/sessions/BUIL...
components/sessions/BUILD.gn:23: group("sessions_core") {
What is this?

https://codereview.chromium.org/673843002/diff/60001/components/sessions/BUIL...
components/sessions/BUILD.gn:28: group("sessions_content") {
and this?

[rohitrao (ping after 24h), 2014-10-24 12:11:11]

From a component build on my mac, setting sessions_core to be a shared_library:

[7827/14066] SOLINK libsessions_core.dylib, POSTBUILDS

Undefined symbols for architecture i386:
  "__ZN8sessions26SerializedNavigationDriver3GetEv", referenced from:
      __ZN8sessions25SerializedNavigationEntryC2Ev in
sessions_core.serialized_navigation_entry.o
     
__ZN8sessions25SerializedNavigationEntry12FromSyncDataEiRKN7sync_pb13TabNavigationE
in sessions_core.serialized_navigation_entry.o
      __ZNK8sessions25SerializedNavigationEntry13WriteToPickleEiP6Pickle in
sessions_core.serialized_navigation_entry.o
      __ZN8sessions25SerializedNavigationEntry14ReadFromPickleEP14PickleIterator
in sessions_core.serialized_navigation_entry.o
ld: symbol(s) not found for architecture i386


This is what Sylvain was warning about.  Something about the component build is
unhappy when the implementation of SerializedNavigationEntry::Get() is in a
different library from where it is used.

[rohitrao (ping after 24h), 2014-10-24 12:18:29]

I'm not sure who to turn to next if you're not comfortable reviewing this. 
Maybe I'll add Joao (who wrote the policy gyp files) and Brett (who wrote the
policy gn files)?

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi
File components/sessions.gypi (right):

https://codereview.chromium.org/673843002/diff/60001/components/sessions.gypi...
components/sessions.gypi:70: 'type': 'none',
I think this is how the GYP file is structured:

In the shared_library build, there can only be one top-level library for
sessions.  We accomplish that by moving the sources/dependency lists into gypi
files and including them all into one giant "sessions" target.

Other gyp files (such as chrome_browser.gypi) will still have dependencies onto
"sessions_core" and "sessions_content," however.  We handle those by defining
dummy targets of type "none" that simply redirect to the one main target.

In the static_library build, the "sessions_core" and "sessions_content" targets
can be implemented as we normally would, albeit using gypi files for the guts. 
We also define a dummy "sessions" target for symmetry, although I think we might
be able to get away without having that.

https://codereview.chromium.org/673843002/diff/60001/components/sessions/BUIL...
File components/sessions/BUILD.gn (right):

https://codereview.chromium.org/673843002/diff/60001/components/sessions/BUIL...
components/sessions/BUILD.gn:23: group("sessions_core") {
On 2014/10/24 07:23:14, blundell wrote:
> What is this?

I'm not really sure, I copied it from components/policy.  I believe it's the GN
equivalent of a "none" target.

[blundell, 2014-10-24 13:07:32]

lgtm, but please add more explanation in sessions_core.gypi as to why it's just
a list of sources and not a target. I think you could just put that list as a
variable sessions_core_sources in sessions.gypi, I don't think you need a
separate gypi file.

[rohitrao (ping after 24h), 2014-10-24 13:45:03]

I've updated this CL to simplify the build files by removing the separate
sessions_core target.  For now, the plan is to have a single target, either
sessions_content or sessions_ios, depending on platform.

I've added a TODO explaining the trouble with splitting the target up.  We can
revisit this setup if sessions grows large enough to cause problems.

[blundell, 2014-10-24 14:10:43]

lgtm

https://codereview.chromium.org/673843002/diff/120001/components/sessions.gypi
File components/sessions.gypi (right):

https://codereview.chromium.org/673843002/diff/120001/components/sessions.gyp...
components/sessions.gypi:9: # TODO(rohitrao): We are including these sources
directly into each
I think this still doesn't explain the key issue, which is that sessions_core
can't be a <(component) due to the fact that it declares a static function that
it doesn't define.

I really don't think that the decision of whether to split up sessions_core into
its own target is dependent on how large the set of core sources is, fwiw.

https://codereview.chromium.org/673843002/diff/120001/components/sessions/BUI...
File components/sessions/BUILD.gn (right):

https://codereview.chromium.org/673843002/diff/120001/components/sessions/BUI...
components/sessions/BUILD.gn:9: source_set("sessions_core") {
I would have an explanation here also of why this is a source_set and not a
component.

[rohitrao (ping after 24h), 2014-10-24 14:41:30]

rohitrao@chromium.org changed reviewers:
+ finnur@chromium.org, sky@chromium.org

[rohitrao (ping after 24h), 2014-10-24 14:43:55]

Thanks for the review!

+TBR finnur for extensions/ gyp+gn changes
+TBR sky for chrome/ gyp+gn changes

I believe TBR is appropriate here because I am simply renaming the
components/sessions targets.

[rohitrao (ping after 24h), 2014-10-24 14:44:36]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2014-10-24 14:45:50]

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

[Finnur, 2014-10-24 15:14:35]

LGTM

[rohitrao (ping after 24h), 2014-10-24 15:16:37]

The CQ bit was unchecked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2014-10-24 15:24:29]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2014-10-24 15:25:06]

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

[commit-bot: I haz the power, 2014-10-24 15:34:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-24 15:35:56]

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