Adding components to dependencies on iOS.

Adding components to dependencies on iOS.
Removing all targets from components so i can add one at a time and check that it works correctly.
Adding components/bookmarks.
components/bookmarks is building correctly but it needs ios/web gn files that are not written yet.

BUG=459705
Committed: https://crrev.com/dc35ba27660f5e0a26ab4e9495efdaaf829afb10
Cr-Commit-Position: refs/heads/master@{#350148}

[sherouk, 2015-09-08 17:29:46]

sherouk@google.com changed reviewers:
+ dpranke@chromium.org, jochen@chromium.org, sdefresne@chromium.org

[sherouk, 2015-09-09 14:11:51]

Can you review please?

[Dirk Pranke, 2015-09-09 20:37:29]

https://codereview.chromium.org/1310513006/diff/1/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/1/components/BUILD.gn#newcode186
components/BUILD.gn:186: #}
just delete this block now?

https://codereview.chromium.org/1310513006/diff/1/components/BUILD.gn#newcode312
components/BUILD.gn:312: # These are the deps required by the code in this
target.
I would just delete this comment.

https://codereview.chromium.org/1310513006/diff/1/components/BUILD.gn#newcode414
components/BUILD.gn:414: deps += [ "//components/rlz:unit_tests" ]
You can now move this line up to after line 405 and get rid of this clause.

https://codereview.chromium.org/1310513006/diff/1/components/BUILD.gn#newcode469
components/BUILD.gn:469: #}
delete this block

https://codereview.chromium.org/1310513006/diff/1/components/BUILD.gn#newcode483
components/BUILD.gn:483: "//components/web_cache/browser:unit_tests",
merge this block up with the block on lines 324-403 ?

https://codereview.chromium.org/1310513006/diff/1/components/test/run_all_uni...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/1310513006/diff/1/components/test/run_all_uni...
components/test/run_all_unittests.cc:100: #if !defined(OS_IOS)
why do we need this change if we don't need it in GYP?

[sdefresne, 2015-09-10 08:45:49]

https://codereview.chromium.org/1310513006/diff/1/components/test/run_all_uni...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/1310513006/diff/1/components/test/run_all_uni...
components/test/run_all_unittests.cc:100: #if !defined(OS_IOS)
On 2015/09/09 at 20:37:28, Dirk Pranke wrote:
> why do we need this change if we don't need it in GYP?

This was how it was done in the version of patchset #1 of
https://codereview.chromium.org/1330613004.
This can now be removed as https://codereview.chromium.org/1330613004 landed and
instead just protected the creation of content::TestContentClientInitializer()
instance.

[sherouk, 2015-09-10 15:29:24]

Patchset #2 (id:20001) has been deleted

[sherouk, 2015-09-10 15:41:43]

Patchset #3 (id:60001) has been deleted

[sherouk, 2015-09-10 15:41:48]

Patchset #2 (id:40001) has been deleted

[sherouk, 2015-09-10 15:45:37]

Patchset #3 (id:100001) has been deleted

[sherouk, 2015-09-10 15:45:43]

Patchset #2 (id:80001) has been deleted

[sherouk, 2015-09-10 15:46:53]

Patchset #2 (id:120001) has been deleted

[sherouk, 2015-09-10 15:49:01]

Patchset #2 (id:140001) has been deleted

[sherouk, 2015-09-10 15:52:18]

Patchset #2 (id:160001) has been deleted

[sherouk, 2015-09-10 15:53:58]

Patchset #2 (id:90008) has been deleted

[sherouk, 2015-09-10 15:55:18]

Patchset #2 (id:190001) has been deleted

[sherouk, 2015-09-10 15:56:49]

Patchset #2 (id:150008) has been deleted

[sherouk, 2015-09-10 16:00:05]

Changes done.

[Dirk Pranke, 2015-09-10 22:19:43]

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:184: if (!enable_configuration_policy && !is_ios) {
it seems like many of these changes are unnecessary.

For example, enable_configuration_policy == !is_ios

Maybe you should either move these inside the big (!is_ios) block, above, or add
components/policy back to the unconditional list?

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:198: if (!is_ios && (!is_chromeos || !enable_extensions)) {
same thought

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:202: if (!enable_plugins && !is_ios) {
same thought.

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:288: if (toolkit_views && !is_ios) {
isn't toolkit_views false on ios?

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:473: if (enable_configuration_policy && !is_ios) {
same comment.

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:480: if (toolkit_views && !is_ios) {
same comment.

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:604: if ((enable_basic_printing || enable_print_preview) &&
!is_ios) {
should these flags just be disabled on ios?

[sherouk, 2015-09-11 15:06:15]

Patchset #3 (id:230001) has been deleted

[sherouk, 2015-09-11 15:07:59]

Patchset #3 (id:250001) has been deleted

[sherouk, 2015-09-11 15:35:00]

Patchset #3 (id:270001) has been deleted

[sherouk, 2015-09-11 15:46:34]

Patchset #3 (id:250002) has been deleted

[sherouk, 2015-09-11 16:54:43]

Changes done. can you review it?

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:184: if (!enable_configuration_policy && !is_ios) {
On 2015/09/10 22:19:43, Dirk Pranke wrote:
> it seems like many of these changes are unnecessary.
> 
> For example, enable_configuration_policy == !is_ios
> 
> Maybe you should either move these inside the big (!is_ios) block, above, or
add
> components/policy back to the unconditional list?

Done.

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:288: if (toolkit_views && !is_ios) {
On 2015/09/10 22:19:43, Dirk Pranke wrote:
> isn't toolkit_views false on ios?

yes.

https://codereview.chromium.org/1310513006/diff/220001/components/BUILD.gn#ne...
components/BUILD.gn:604: if ((enable_basic_printing || enable_print_preview) &&
!is_ios) {
On 2015/09/10 22:19:43, Dirk Pranke wrote:
> should these flags just be disabled on ios?

Done.

[Dirk Pranke, 2015-09-11 17:03:59]

lgtm, thanks!

https://codereview.chromium.org/1310513006/diff/300001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/300001/components/BUILD.gn#ne...
components/BUILD.gn:501: if (!is_ios) {
I wouldn't expect this change to be necessary?

[jochen (gone - plz use gerrit), 2015-09-14 09:42:42]

lgtm

[sherouk, 2015-09-14 13:10:58]

On 2015/09/11 17:03:59, Dirk Pranke wrote:
> lgtm, thanks!
> 
> https://codereview.chromium.org/1310513006/diff/300001/components/BUILD.gn
> File components/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1310513006/diff/300001/components/BUILD.gn#ne...
> components/BUILD.gn:501: if (!is_ios) {
> I wouldn't expect this change to be necessary?

Yeah it's building without, Thanks!

[sherouk, 2015-09-14 13:11:04]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 13:11:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps320001
(title: " ")

[commit-bot: I haz the power, 2015-09-14 13:11:20]

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

[commit-bot: I haz the power, 2015-09-14 13:14:21]

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

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

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

[sherouk, 2015-09-14 13:20:29]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 13:20:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps340001
(title: " ")

[commit-bot: I haz the power, 2015-09-14 13:20:40]

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

[commit-bot: I haz the power, 2015-09-14 13:23:42]

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

[commit-bot: I haz the power, 2015-09-14 13:23:43]

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

[sdefresne, 2015-09-14 13:32:03]

https://codereview.chromium.org/1310513006/diff/340001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/340001/components/BUILD.gn#ne...
components/BUILD.gn:37: deps = [
needs to be "deps += ["

https://codereview.chromium.org/1310513006/diff/340001/components/BUILD.gn#ne...
components/BUILD.gn:320: deps = [
needs to be "deps += ["

https://codereview.chromium.org/1310513006/diff/340001/components/favicon/cor...
File components/favicon/core/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/340001/components/favicon/cor...
components/favicon/core/BUILD.gn:38: "fallback_icon_client.h",
can you put those behind OS!="ios" condition in gyp file too?

[sherouk, 2015-09-14 14:09:26]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 14:09:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps360001
(title: "Fixing errors")

[commit-bot: I haz the power, 2015-09-14 14:09:39]

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

[commit-bot: I haz the power, 2015-09-14 14:13:43]

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

[commit-bot: I haz the power, 2015-09-14 14:13:44]

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

[sdefresne, 2015-09-14 14:31:25]

https://codereview.chromium.org/1310513006/diff/360001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/360001/components/BUILD.gn#ne...
components/BUILD.gn:453: sources = [
this should be "sources += ["

[sherouk, 2015-09-14 14:39:51]

Patchset #7 (id:380001) has been deleted

[sherouk, 2015-09-14 14:40:09]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 14:40:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps400001
(title: " ")

[commit-bot: I haz the power, 2015-09-14 14:40:37]

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

[commit-bot: I haz the power, 2015-09-14 14:44:05]

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

[commit-bot: I haz the power, 2015-09-14 14:44:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sdefresne, 2015-09-14 15:52:59]

https://codereview.chromium.org/1310513006/diff/400001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/400001/components/BUILD.gn#ne...
components/BUILD.gn:451: repack("components_tests_pak") {
This is incorrect it should be:

repack("components_tests_pak") {
  sources = [
    "$root_gen_dir/components/components_resources.pak",
    "$root_gen_dir/components/strings/components_strings_en-US.pak",
  ]
  output = "$root_out_dir/components_tests_resources.pak"
  if (!is_ios) {
    deps = [
      "//components/resources",
      "//components/strings",
    ]
  }
}

Though I'm surprised that //components/resources and //components/strings are
not currently building as-is on iOS.

[sherouk, 2015-09-14 16:31:37]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 16:31:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps420001
(title: " ")

[commit-bot: I haz the power, 2015-09-14 16:33:07]

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

[commit-bot: I haz the power, 2015-09-14 16:40:15]

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

[commit-bot: I haz the power, 2015-09-14 16:40:16]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[sherouk, 2015-09-14 16:44:31]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 16:44:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps460001
(title: " ")

[commit-bot: I haz the power, 2015-09-14 16:45:45]

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

[commit-bot: I haz the power, 2015-09-14 16:50:35]

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

[commit-bot: I haz the power, 2015-09-14 16:50:36]

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

[sherouk, 2015-09-14 16:56:54]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-14 16:56:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps480001
(title: "removing duplicate block")

[commit-bot: I haz the power, 2015-09-14 16:58:12]

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

[commit-bot: I haz the power, 2015-09-14 17:04:11]

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

[commit-bot: I haz the power, 2015-09-14 17:04:12]

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

[sdefresne, 2015-09-15 13:14:50]

I think this CL need some more work before we can send it to the CQ. Please ask
for another review once you've fixed the errors (you should be able to send the
CL to the try bots with "git cl try" to validate that it is not breaking other
platforms).

dpranke: I'm surprise that the following snippet does not warn about nested
targets (are nested targets even allowed with gn). Currently, it warns about
incorrect deps (i.e. a target not marked as test_only dependending on a
test_only target).

test("components_unittests") {
  sources = [ "..." ]
  deps = [ "..." ]
  repack("components_test_pack") {
    sources += [ "///" ]
    deps += [ "///" ]
  }
}

https://codereview.chromium.org/1310513006/diff/480001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/480001/components/BUILD.gn#ne...
components/BUILD.gn:451: repack("components_tests_pak") {
repack("components_tests_pak") used to be at the top-level of the file (i.e. not
nested into any other {} block) but your CL move it inside the block of the
test("components_unittests") target.

This is incorrect. This target cannot be nested into the other target. This is
what is causing all the errors (as it pulls the deps/sources from the parent
target and mix with its own). Move it back to the top-level.

Please also check that you have fixed all such other issues. I don't think there
should be that much moving of code around in that file just for putting some
deps/sources behind "if (!is_ios)" conditions.

[sherouk, 2015-09-15 14:09:16]

Patchset #13 (id:520001) has been deleted

[sherouk, 2015-09-15 14:17:26]

On 2015/09/15 13:14:50, sdefresne wrote:
> I think this CL need some more work before we can send it to the CQ. Please
ask
> for another review once you've fixed the errors (you should be able to send
the
> CL to the try bots with "git cl try" to validate that it is not breaking other
> platforms).
> 
> dpranke: I'm surprise that the following snippet does not warn about nested
> targets (are nested targets even allowed with gn). Currently, it warns about
> incorrect deps (i.e. a target not marked as test_only dependending on a
> test_only target).
> 
> test("components_unittests") {
>   sources = [ "..." ]
>   deps = [ "..." ]
>   repack("components_test_pack") {
>     sources += [ "///" ]
>     deps += [ "///" ]
>   }
> }
> 
> https://codereview.chromium.org/1310513006/diff/480001/components/BUILD.gn
> File components/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1310513006/diff/480001/components/BUILD.gn#ne...
> components/BUILD.gn:451: repack("components_tests_pak") {
> repack("components_tests_pak") used to be at the top-level of the file (i.e.
not
> nested into any other {} block) but your CL move it inside the block of the
> test("components_unittests") target.
> 
> This is incorrect. This target cannot be nested into the other target. This is
> what is causing all the errors (as it pulls the deps/sources from the parent
> target and mix with its own). Move it back to the top-level.
> 
> Please also check that you have fixed all such other issues. I don't think
there
> should be that much moving of code around in that file just for putting some
> deps/sources behind "if (!is_ios)" conditions.

Now should it be done?

[sdefresne, 2015-09-15 15:51:48]

On 2015/09/15 at 14:17:26, sherouk wrote:
> On 2015/09/15 13:14:50, sdefresne wrote:
> > I think this CL need some more work before we can send it to the CQ. Please
ask
> > for another review once you've fixed the errors (you should be able to send
the
> > CL to the try bots with "git cl try" to validate that it is not breaking
other
> > platforms).
> > 
> > dpranke: I'm surprise that the following snippet does not warn about nested
> > targets (are nested targets even allowed with gn). Currently, it warns about
> > incorrect deps (i.e. a target not marked as test_only dependending on a
> > test_only target).
> > 
> > test("components_unittests") {
> >   sources = [ "..." ]
> >   deps = [ "..." ]
> >   repack("components_test_pack") {
> >     sources += [ "///" ]
> >     deps += [ "///" ]
> >   }
> > }
> > 
> > https://codereview.chromium.org/1310513006/diff/480001/components/BUILD.gn
> > File components/BUILD.gn (right):
> > 
> >
https://codereview.chromium.org/1310513006/diff/480001/components/BUILD.gn#ne...
> > components/BUILD.gn:451: repack("components_tests_pak") {
> > repack("components_tests_pak") used to be at the top-level of the file (i.e.
not
> > nested into any other {} block) but your CL move it inside the block of the
> > test("components_unittests") target.
> > 
> > This is incorrect. This target cannot be nested into the other target. This
is
> > what is causing all the errors (as it pulls the deps/sources from the parent
> > target and mix with its own). Move it back to the top-level.
> > 
> > Please also check that you have fixed all such other issues. I don't think
there
> > should be that much moving of code around in that file just for putting some
> > deps/sources behind "if (!is_ios)" conditions.
> 
> Now should it be done?

Can you send the CL to the try bots using "git cl try"?

[Dirk Pranke, 2015-09-15 20:54:49]

On 2015/09/15 13:14:50, sdefresne wrote:
> dpranke: I'm surprise that the following snippet does not warn about nested
> targets (are nested targets even allowed with gn). Currently, it warns about
> incorrect deps (i.e. a target not marked as test_only dependending on a
> test_only target).
>

That should probably get filed as a bug against GN.

[Dirk Pranke, 2015-09-15 20:59:05]

lgtm assuming the try jobs pass.

[sherouk, 2015-09-16 09:27:00]

Patchset #15 (id:580001) has been deleted

[sherouk, 2015-09-17 15:23:12]

Patchset #16 (id:620001) has been deleted

[sherouk, 2015-09-17 15:39:15]

Patchset #16 (id:640001) has been deleted

[sherouk, 2015-09-17 17:22:09]

Patchset #17 (id:680001) has been deleted

[sherouk, 2015-09-18 12:03:29]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-18 12:03:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps740001
(title: " ")

[commit-bot: I haz the power, 2015-09-18 12:03:36]

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

[commit-bot: I haz the power, 2015-09-18 12:15:45]

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

[commit-bot: I haz the power, 2015-09-18 12:15:47]

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

[chromium-reviews, 2015-09-18 12:19:17]

Looks like it is again failing with patch failure. I guess you'll have to
again rebase it :-/
-- Sylvain

On Fri, 18 Sep 2015 at 14:15 commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sherouk, 2015-09-18 12:38:34]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-18 12:38:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps760001
(title: "Rebasing.")

[commit-bot: I haz the power, 2015-09-18 12:38:52]

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

[commit-bot: I haz the power, 2015-09-18 12:42:12]

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

[commit-bot: I haz the power, 2015-09-18 12:42:13]

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

[sherouk, 2015-09-18 12:51:47]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-18 12:51:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps780001
(title: "Removing conflict")

[commit-bot: I haz the power, 2015-09-18 12:51:59]

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

[commit-bot: I haz the power, 2015-09-18 12:55:28]

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

[commit-bot: I haz the power, 2015-09-18 12:55:30]

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

[sherouk, 2015-09-18 12:57:35]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-18 12:57:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps800001
(title: "Removing conflict.")

[commit-bot: I haz the power, 2015-09-18 12:57:54]

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

[commit-bot: I haz the power, 2015-09-18 13:08:02]

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

[commit-bot: I haz the power, 2015-09-18 13:08:04]

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

[sherouk, 2015-09-18 13:31:28]

Patchset #23 (id:820001) has been deleted

[sherouk, 2015-09-18 13:33:16]

Patchset #23 (id:840001) has been deleted

[sherouk, 2015-09-18 13:34:11]

sherouk@google.com changed reviewers:
+ brettw@chromium.org
- dpranke@chromium.org, jochen@chromium.org, sdefresne@chromium.org

[sherouk, 2015-09-18 13:34:12]

Can you review please?

[sherouk, 2015-09-18 13:34:36]

sherouk@google.com changed reviewers:
+ dpranke@chromium.org, jochen@chromium.org, sdefresne@chromium.org

[sdefresne, 2015-09-21 12:27:47]

On 2015/09/18 at 13:34:12, sherouk wrote:
> Can you review please?

brettw: I think we need your OWNERS approval for the changes to //base/BUILD.gn.
Thank you.

[brettw, 2015-09-22 05:05:10]

lgtm

[sherouk, 2015-09-22 12:30:20]

The CQ bit was checked by sherouk@google.com

[sherouk, 2015-09-22 12:30:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1310513006/#ps860001
(title: "Rebasing")

[commit-bot: I haz the power, 2015-09-22 12:30:31]

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

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

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

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

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sherouk, 2015-09-22 14:03:43]

The CQ bit was checked by sherouk@google.com

[commit-bot: I haz the power, 2015-09-22 14:04:17]

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

[commit-bot: I haz the power, 2015-09-22 14:09:44]

Committed patchset #23 (id:860001)

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

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

[blundell, 2015-10-15 08:41:38]

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

[blundell, 2015-10-15 08:41:39]

https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn#ne...
components/BUILD.gn:390: "//components/variations:unit_tests",
It looks like this CL didn't move over
//components/variations/service:unit_tests. Not sure if there are any others
missing.

[sdefresne, 2015-10-19 06:16:41]

On 2015/10/15 at 08:41:39, blundell wrote:
> https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn
> File components/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn#ne...
> components/BUILD.gn:390: "//components/variations:unit_tests",
> It looks like this CL didn't move over
//components/variations/service:unit_tests. Not sure if there are any others
missing.

Only components that were know to build successfully on iOS have been added.
Other may build and could be enabled, they probably just have not been tested
yet.

[blundell, 2015-10-19 06:19:54]

On 2015/10/19 06:16:41, sdefresne (OOO till 19th Oct.) wrote:
> On 2015/10/15 at 08:41:39, blundell wrote:
> > https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn
> > File components/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn#ne...
> > components/BUILD.gn:390: "//components/variations:unit_tests",
> > It looks like this CL didn't move over
> //components/variations/service:unit_tests. Not sure if there are any others
> missing.
> 
> Only components that were know to build successfully on iOS have been added.
> Other may build and could be enabled, they probably just have not been tested
> yet.

The problem here is that //components/variations/service:unit_tests disappeared
entirely instead of being moved to the !iOS section.

[sdefresne, 2015-10-19 14:56:24]

On 2015/10/19 at 06:19:54, blundell wrote:
> On 2015/10/19 06:16:41, sdefresne (OOO till 19th Oct.) wrote:
> > On 2015/10/15 at 08:41:39, blundell wrote:
> > > https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn
> > > File components/BUILD.gn (right):
> > > 
> > >
> >
https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn#ne...
> > > components/BUILD.gn:390: "//components/variations:unit_tests",
> > > It looks like this CL didn't move over
> > //components/variations/service:unit_tests. Not sure if there are any others
> > missing.
> > 
> > Only components that were know to build successfully on iOS have been added.
> > Other may build and could be enabled, they probably just have not been
tested
> > yet.
> 
> The problem here is that //components/variations/service:unit_tests
disappeared entirely instead of being moved to the !iOS section.

Oh, didn't catch this. I'll fix it later today. Thank you.

[blundell, 2015-10-19 14:57:07]

On 2015/10/19 14:56:24, sdefresne (OOO till 19th Oct.) wrote:
> On 2015/10/19 at 06:19:54, blundell wrote:
> > On 2015/10/19 06:16:41, sdefresne (OOO till 19th Oct.) wrote:
> > > On 2015/10/15 at 08:41:39, blundell wrote:
> > > >
https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn
> > > > File components/BUILD.gn (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn#ne...
> > > > components/BUILD.gn:390: "//components/variations:unit_tests",
> > > > It looks like this CL didn't move over
> > > //components/variations/service:unit_tests. Not sure if there are any
others
> > > missing.
> > > 
> > > Only components that were know to build successfully on iOS have been
added.
> > > Other may build and could be enabled, they probably just have not been
> tested
> > > yet.
> > 
> > The problem here is that //components/variations/service:unit_tests
> disappeared entirely instead of being moved to the !iOS section.
> 
> Oh, didn't catch this. I'll fix it later today. Thank you.

Thanks!

[Alexei Svitkine (slow), 2015-10-19 15:03:40]

I think it's being fixed already by this CL:
https://codereview.chromium.org/1404583004/

On Mon, Oct 19, 2015 at 10:57 AM, <blundell@chromium.org> wrote:

> On 2015/10/19 14:56:24, sdefresne (OOO till 19th Oct.) wrote:
>
>> On 2015/10/19 at 06:19:54, blundell wrote:
>> > On 2015/10/19 06:16:41, sdefresne (OOO till 19th Oct.) wrote:
>> > > On 2015/10/15 at 08:41:39, blundell wrote:
>> > > >
>>
> https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn
>
>> > > > File components/BUILD.gn (right):
>> > > >
>> > > >
>> > >
>>
>
>
>
https://codereview.chromium.org/1310513006/diff/860001/components/BUILD.gn#ne...
>
>> > > > components/BUILD.gn:390: "//components/variations:unit_tests",
>> > > > It looks like this CL didn't move over
>> > > //components/variations/service:unit_tests. Not sure if there are any
>>
> others
>
>> > > missing.
>> > >
>> > > Only components that were know to build successfully on iOS have been
>>
> added.
>
>> > > Other may build and could be enabled, they probably just have not been
>> tested
>> > > yet.
>> >
>> > The problem here is that //components/variations/service:unit_tests
>> disappeared entirely instead of being moved to the !iOS section.
>>
>
> Oh, didn't catch this. I'll fix it later today. Thank you.
>>
>
> Thanks!
>
> https://codereview.chromium.org/1310513006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2017-03-22 10:29:09]

https://codereview.chromium.org/1310513006/diff/860001/components/favicon/cor...
File components/favicon/core/BUILD.gn (right):

https://codereview.chromium.org/1310513006/diff/860001/components/favicon/cor...
components/favicon/core/BUILD.gn:36: if (!is_ios) {
FYI, these files should just be moved to //components/favicon/content. I'll do
that when I have some free cycles.

In general it merits a second look if you're inserting a !is_ios block in a
//components/foo/core/BUILD.gn file, since the purpose of //components/foo/core
is precisely to share code between iOS and other platforms.