Break up the chrome/common:common target

Break up the chrome/common:common target

This CL breaks off some targets from chrome/common:common.

Context: https://crrev.com/2850863002 introduced a binary size
regression. In order to address it, chrome_logging.* needs to be 
pulled out from chrome/common:common to a separate target, but this turns out to be hard. This CL is a stepping stone.

BUG=717103
Review-Url: https://codereview.chromium.org/2942643002
Cr-Commit-Position: refs/heads/master@{#479881}
Committed: https://chromium.googlesource.com/chromium/src/+/ea6c95a904cac342b91cc68b06d2851a3d9e6394

[manzagop (departed), 2017-06-14 15:13:00]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:13:16]

Dry run: 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, 2017-06-14 15:17:35]

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

[commit-bot: I haz the power, 2017-06-14 15:17:35]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[manzagop (departed), 2017-06-14 15:30:15]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:30:38]

Dry run: 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, 2017-06-14 15:35:06]

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

[commit-bot: I haz the power, 2017-06-14 15:35:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agrieve, 2017-06-14 15:45:47]

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

[agrieve, 2017-06-14 15:45:48]

https://codereview.chromium.org/2942643002/diff/20001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2942643002/diff/20001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:579: static_library("logging") {
Would this make more sense as a source_set?

gn help static_library says:

  If you only need the static library for intermediate results in the build,
  you should consider a source_set instead since it will skip the (potentially
  slow) step of creating the intermediate library file.

[manzagop (departed), 2017-06-14 16:42:05]

Looks like I missed a file, all_messages.h, which actually pulls in a lot of
stuff. 

I'll see if I can break up 'common' some more.

[manzagop (departed), 2017-06-14 20:37:20]

Had another look and this is not going to work. I pull and more always keeps
coming and it's not making sense anymore.

I'm going to start over and break up targets based on the subfolders. Hopefully
that breaks up nicely and gets me closer
to isolating chrome_logging.

[manzagop (departed), 2017-06-14 21:52:08]

Description was changed from

==========
Limit chrome_watcher's dependency on chrome/common

This addresses a binary size regression introduced in
https://crrev.com/2850863002.

mini_installer.exe size (KB):
 - before offending CL: 632,173
 - after offending CL: 640,942
 - with current CL: 632,205

BUG=717103
==========

to

==========
Break up the chrome/common:common target

This CL breaks off some targets from chrome/common:common.

Context: https://crrev.com/2850863002 introduced a binary size
regression. In order to address it, chrome_logging.* needs to be 
pulled out from chrome/common:common to a separate target, but this turns out to
be hard. This CL is a stepping stone.



mini_installer.exe size (KB):
 - before offending CL: 632,173
 - after offending CL: 640,942
 - with current CL: 632,205

BUG=717103
==========

[manzagop (departed), 2017-06-14 21:52:18]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 21:52:38]

Dry run: CQ is trying da patch.

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

[manzagop (departed), 2017-06-14 21:52:45]

Description was changed from

==========
Break up the chrome/common:common target

This CL breaks off some targets from chrome/common:common.

Context: https://crrev.com/2850863002 introduced a binary size
regression. In order to address it, chrome_logging.* needs to be 
pulled out from chrome/common:common to a separate target, but this turns out to
be hard. This CL is a stepping stone.



mini_installer.exe size (KB):
 - before offending CL: 632,173
 - after offending CL: 640,942
 - with current CL: 632,205

BUG=717103
==========

to

==========
Break up the chrome/common:common target

This CL breaks off some targets from chrome/common:common.

Context: https://crrev.com/2850863002 introduced a binary size
regression. In order to address it, chrome_logging.* needs to be 
pulled out from chrome/common:common to a separate target, but this turns out to
be hard. This CL is a stepping stone.

BUG=717103
==========

[manzagop (departed), 2017-06-14 21:58:05]

Hi Andrew!

Are you ok with the approach of starting by breaking up the common target based
on subfolders? IIUC GN likes many
small targets.

Trying to only extract chrome_logging turned out too 
complex.

If you agree, I'd do it one subfolder at a time, 
to ease reviewing.

Thanks,

Pierre-Antoine

[manzagop (departed), 2017-06-14 22:00:08]

Additionally, if you agree with the approach, please be
extra nitty on this first CL, as I'm not super familiar
with GN and this will be good for subsequent ones! :)

[commit-bot: I haz the power, 2017-06-14 22:01:40]

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

[commit-bot: I haz the power, 2017-06-14 22:01:41]

Dry run: 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...)

[agrieve, 2017-06-15 00:32:15]

On 2017/06/14 22:00:08, manzagop wrote:
> Additionally, if you agree with the approach, please be
> extra nitty on this first CL, as I'm not super familiar
> with GN and this will be good for subsequent ones! :)

I'd certainly be supportive of breaking up targets into smaller ones. The
finer-grained, the better IMO. If there's some reason not to prefer this, I'm
sure we'll find out along the way!

[manzagop (departed), 2017-06-15 00:56:57]

On 2017/06/15 00:32:15, agrieve wrote:
> On 2017/06/14 22:00:08, manzagop wrote:
> > Additionally, if you agree with the approach, please be
> > extra nitty on this first CL, as I'm not super familiar
> > with GN and this will be good for subsequent ones! :)
> 
> I'd certainly be supportive of breaking up targets into smaller ones. The
> finer-grained, the better IMO. If there's some reason not to prefer this, I'm
> sure we'll find out along the way!

Cool! Then this is ready for review. (But no hurry, it's late!)

[manzagop (departed), 2017-06-15 00:59:35]

On 2017/06/15 00:56:57, manzagop wrote:
> On 2017/06/15 00:32:15, agrieve wrote:
> > On 2017/06/14 22:00:08, manzagop wrote:
> > > Additionally, if you agree with the approach, please be
> > > extra nitty on this first CL, as I'm not super familiar
> > > with GN and this will be good for subsequent ones! :)
> > 
> > I'd certainly be supportive of breaking up targets into smaller ones. The
> > finer-grained, the better IMO. If there's some reason not to prefer this,
I'm
> > sure we'll find out along the way!
> 
> Cool! Then this is ready for review. (But no hurry, it's late!)

Actually, are you ok taking on the reviews?

[manzagop (departed), 2017-06-15 13:06:42]

manzagop@chromium.org changed reviewers:
+ scottbyer@chromium.org

[manzagop (departed), 2017-06-15 13:06:42]

Hi Scott,
Could you have a look?
Thanks!
Pierre-Antoine

[agrieve, 2017-06-15 13:52:00]

I'm happy to do reviews, but have owners only for BUILD.gn :P.
lgtm /w nits

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (left):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:464: "//chrome/install_static:install_static_util",
Do you know what this might have been for?

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:62: # DO NOT SUBMIT. Preexisting logic. Validate
substractive approach.
Maybe address this DO NOT SUBMIT if the change is ready :P

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:64: sources -= [ "channel_info_posix.cc" ]
Rather than adding and then removing channel_info_posix.cc, it's a bit nicer to
leave it out of the original sources list, and only added it for is_linux

[agrieve, 2017-06-15 13:52:56]

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/cloud_pri...
File chrome/common/cloud_print/BUILD.gn (right):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/cloud_pri...
chrome/common/cloud_print/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All
rights reserved.
Might be nice to add BUILD.gn=* to OWNERS here so that it matches the one in
chrome/common

[manzagop (departed), 2017-06-15 14:54:09]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 14:54:25]

Dry run: CQ is trying da patch.

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

[manzagop (departed), 2017-06-15 14:55:00]

Thanks! 

Addressed comments, but have a question. Another look?

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (left):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:464: "//chrome/install_static:install_static_util",
On 2017/06/15 13:52:00, agrieve wrote:
> Do you know what this might have been for?

Hm.

channel_info_win.cc uses it so I'd moved it above to the channel_info
source_set.

But it looks like logging_chrome.cc in this target uses it too. I guess it works
because this file uses public_deps. Should I add it back here? 

More generally, how do you feel about deps vs public_deps?

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:62: # DO NOT SUBMIT. Preexisting logic. Validate
substractive approach.
On 2017/06/15 13:52:00, agrieve wrote:
> Maybe address this DO NOT SUBMIT if the change is ready :P

Yup! I meant to ask if there's any reason for the "substractive" approach -
seems not!

Done.

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:64: sources -= [ "channel_info_posix.cc" ]
On 2017/06/15 13:52:00, agrieve wrote:
> Rather than adding and then removing channel_info_posix.cc, it's a bit nicer
to
> leave it out of the original sources list, and only added it for is_linux

Totally! Done.

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/cloud_pri...
File chrome/common/cloud_print/BUILD.gn (right):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/cloud_pri...
chrome/common/cloud_print/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All
rights reserved.
On 2017/06/15 13:52:55, agrieve wrote:
> Might be nice to add BUILD.gn=* to OWNERS here so that it matches the one in
> chrome/common

Done.

[agrieve, 2017-06-15 15:06:17]

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (left):

https://codereview.chromium.org/2942643002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:464: "//chrome/install_static:install_static_util",
On 2017/06/15 14:54:59, manzagop wrote:
> On 2017/06/15 13:52:00, agrieve wrote:
> > Do you know what this might have been for?
> 
> Hm.
> 
> channel_info_win.cc uses it so I'd moved it above to the channel_info
> source_set.
> 
> But it looks like logging_chrome.cc in this target uses it too. I guess it
works
> because this file uses public_deps. Should I add it back here? 
> 
> More generally, how do you feel about deps vs public_deps?

Ah, just missed that it was added above.

Unless you consider the dep a part of your public api (e.g. aka, your header
files use types defined by the other library), then non-public is the way to go.

[commit-bot: I haz the power, 2017-06-15 15:10:20]

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

[commit-bot: I haz the power, 2017-06-15 15:10:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[manzagop (departed), 2017-06-15 18:16:41]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 18:17:28]

Dry run: CQ is trying da patch.

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

[manzagop (departed), 2017-06-15 18:36:04]

Hm, looks like is_linux includes chromeos and others. I switched the condition
back to what it was before (or rather the negation of that).

Will submit if it passes the CQ!

[commit-bot: I haz the power, 2017-06-15 20:16:37]

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

[commit-bot: I haz the power, 2017-06-15 20:16:38]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2017-06-15 20:34:20]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2017-06-15 20:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2942643002/#ps100001
(title: "revise an is_linux condition back to what it was (e.g. avoid
chromeos)")

[commit-bot: I haz the power, 2017-06-15 20:35:01]

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, 2017-06-15 20:46:04]

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

[commit-bot: I haz the power, 2017-06-15 20:46:06]

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...)

[Scott Byer, 2017-06-15 21:31:09]

lgtm

[manzagop (departed), 2017-06-15 21:38:07]

On 2017/06/15 21:31:09, Scott Byer wrote:
> lgtm

Thanks for the review!

[manzagop (departed), 2017-06-15 21:38:16]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2017-06-15 21:38:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, scottbyer@chromium.org
Link to the patchset: https://codereview.chromium.org/2942643002/#ps120001
(title: "Remove obsolete owners from chrome/common/cloud_print")

[commit-bot: I haz the power, 2017-06-15 21:38:55]

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, 2017-06-15 22:59:17]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497562696614320,
"parent_rev": "6ebcb95f8c4fadb29b69d2fbcbf8c964974e76aa", "commit_rev":
"ea6c95a904cac342b91cc68b06d2851a3d9e6394"}

[commit-bot: I haz the power, 2017-06-15 22:59:31]

Description was changed from

==========
Break up the chrome/common:common target

This CL breaks off some targets from chrome/common:common.

Context: https://crrev.com/2850863002 introduced a binary size
regression. In order to address it, chrome_logging.* needs to be 
pulled out from chrome/common:common to a separate target, but this turns out to
be hard. This CL is a stepping stone.

BUG=717103
==========

to

==========
Break up the chrome/common:common target

This CL breaks off some targets from chrome/common:common.

Context: https://crrev.com/2850863002 introduced a binary size
regression. In order to address it, chrome_logging.* needs to be 
pulled out from chrome/common:common to a separate target, but this turns out to
be hard. This CL is a stepping stone.

BUG=717103

Review-Url: https://codereview.chromium.org/2942643002
Cr-Commit-Position: refs/heads/master@{#479881}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea6c95a904cac342b91cc68b06d2...
==========

[commit-bot: I haz the power, 2017-06-15 22:59:33]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ea6c95a904cac342b91cc68b06d2...