Handle storyboards in GN.

Handle storyboards in GN.

BUG=none
R=sdefresne@chromium.org,rohitrao@chromium.org

Committed: https://crrev.com/39fe426a5bec5352ddacab96ac10c6c33f9d5ac1
Cr-Commit-Position: refs/heads/master@{#437779}

[lpromero, 2016-12-09 14:57:47]

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

[commit-bot: I haz the power, 2016-12-09 14:58:06]

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

[lpromero, 2016-12-09 15:02:19]

Description was changed from

==========
Handle storyboards in GN.

BUG=none
R=sdefresne@google.com,rohitrao@google.com
==========

to

==========
Handle storyboards in GN.

BUG=none
R=sdefresne@chromium.org,rohitrao@chromium.org
==========

[lpromero, 2016-12-09 15:02:19]

lpromero@chromium.org changed reviewers:
+ rohitrao@chromium.org, sdefresne@chromium.org
- rohitrao@google.com, sdefresne@google.com

[sdefresne, 2016-12-09 15:27:16]

https://codereview.chromium.org/2564023002/diff/1/build/config/mac/base_rules...
File build/config/mac/base_rules.gni (right):

https://codereview.chromium.org/2564023002/diff/1/build/config/mac/base_rules...
build/config/mac/base_rules.gni:214: template("compile_xibs") {
There is only one difference between compile_xibs and compile_storyboards (the
extension of the output) and I think we would be better factoring this out than
duplicating the code.

What about:

# Template to combile .xib or .storyboard files.
#
# Arguments
#
#     sources:
#         list of string, sources to compile
#
#     output_extensions:
#         string, extension of the compiled files
#
#     ibtool_flags:
#         (optional) list of string, additional flags to pass to the ibtool
template("compile_ib_files") {
  action_foreach(target_name) {
    forward_variables_from(invoker,
                           [
                             "testonly",
                             "visibility",
                           ])
    assert(defined(invoker.sources),
           "Sources must be specified for $target_name")
    assert(defined(invoker.output_extension),
           "output_extension must be specified for $target_name")

    ibtool_flags = []
    if (defined(invoker.ibtool_flags)) {
      ibtool_flags = invoker.ibtool_flags
    }

    _output_extension = invoker.output_extension

    script = "//build/config/mac/compile_xib.py"
    sources = invoker.sources
    outputs = [
      "$target_gen_dir/$target_name/{{source_name_part}}.$_output_extension",
    ]
    args = [
      "--input",
      "{{source}}",
      "--output",
    ] + rebase_path(outputs, root_build_dir)
    if (!use_system_xcode) {
      args += [
        "--developer_dir",
        hermetic_xcode_path,
      ]
    }
    args += ibtool_flags
  }
}

[sdefresne, 2016-12-09 15:29:55]

https://codereview.chromium.org/2564023002/diff/1/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/2564023002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:752: template("bundle_data_ib_file") {
Don't remove the old template name, you'll break downstream. You can rename it,
but keep a forwarding-template until all downstream code has been ported to the
new name:

template("bundle_data_xib") {
  bundle_data_ib_file(target_name) {
    forward_variables_from(invoker, "*")
  }
}

[lpromero, 2016-12-09 15:34:19]

On 2016/12/09 15:29:55, sdefresne wrote:
> https://codereview.chromium.org/2564023002/diff/1/build/config/ios/rules.gni
> File build/config/ios/rules.gni (right):
> 
>
https://codereview.chromium.org/2564023002/diff/1/build/config/ios/rules.gni#...
> build/config/ios/rules.gni:752: template("bundle_data_ib_file") {
> Don't remove the old template name, you'll break downstream. You can rename
it,
> but keep a forwarding-template until all downstream code has been ported to
the
> new name:
> 
> template("bundle_data_xib") {
>   bundle_data_ib_file(target_name) {
>     forward_variables_from(invoker, "*")
>   }
> }

I started doing that (to land 3 sided patch so that the roll doesn't break).
Turns out all uses of bundle_data_xib I could find are upstream already.

[lpromero, 2016-12-09 15:44:49]

https://codereview.chromium.org/2564023002/diff/1/build/config/mac/base_rules...
File build/config/mac/base_rules.gni (right):

https://codereview.chromium.org/2564023002/diff/1/build/config/mac/base_rules...
build/config/mac/base_rules.gni:214: template("compile_xibs") {
Right, I tried to factor out. invoker.output_extension is definitely
interesting, but there is one thing I don't understand.

invoker.sources means there could be several files, right? So I don't understand
what invoker.output_extension returns, especially if there are several sources
files with different extensions.

[lpromero, 2016-12-09 15:46:17]

Starting to understand:
ERROR at //build/config/mac/base_rules.gni:223:5: Assertion failed.
    assert(defined(invoker.output_extension),
    ^-----
output_extension must be specified for
native_content_controller_test_xib_compile_xib
See //build/config/ios/rules.gni:766:3: whence it was called.
  compile_ib_files(_compile_ib_file) {
  ^-----------------------------------
See //ios/chrome/browser/ui/BUILD.gn:80:1: whence it was called.
bundle_data_ib_file("native_content_controller_test_xib") {
^----------------------------------------------------------
See //ios/chrome/BUILD.gn:41:5: which caused the file to be included.
    "//ios/chrome/browser/ui:unit_tests",
    ^-----------------------------------

output_extension is a variable we set explicitly in the targets? It's not
infered from the source files, right?

[lpromero, 2016-12-09 15:54:03]

SHould be OK now.

[sdefresne, 2016-12-09 16:01:46]

lgtm

[lpromero, 2016-12-09 16:51:05]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2016-12-09 16:51:26]

Thank you for the review!

[commit-bot: I haz the power, 2016-12-09 16:51:28]

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-12-09 16:56:14]

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

[commit-bot: I haz the power, 2016-12-09 16:56:15]

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

[lpromero, 2016-12-09 17:11:01]

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

[commit-bot: I haz the power, 2016-12-09 17:11:33]

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, 2016-12-09 17:17:50]

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

[commit-bot: I haz the power, 2016-12-09 17:17:51]

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

[sdefresne, 2016-12-09 17:35:33]

On 2016/12/09 16:56:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

Please fix mac template too, and ask rsesek@ to review the mac changes.

[lpromero, 2016-12-09 17:41:50]

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

[commit-bot: I haz the power, 2016-12-09 17:42:28]

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

[sdefresne, 2016-12-09 17:45:11]

sdefresne@chromium.org changed reviewers:
+ rsesek@chromium.org

[sdefresne, 2016-12-09 17:45:11]

sdefresne@chromium.org changed required reviewers:
+ rsesek@chromium.org

[sdefresne, 2016-12-09 17:45:41]

https://codereview.chromium.org/2564023002/diff/80001/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/2564023002/diff/80001/build/config/mac/rules....
build/config/mac/rules.gni:84: output_extension = "xib"
This should be "nib".

[lpromero, 2016-12-09 17:48:11]

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

[commit-bot: I haz the power, 2016-12-09 17:48:19]

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

[lpromero, 2016-12-09 18:04:45]

https://codereview.chromium.org/2564023002/diff/80001/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/2564023002/diff/80001/build/config/mac/rules....
build/config/mac/rules.gni:84: output_extension = "xib"
On 2016/12/09 17:45:41, sdefresne wrote:
> This should be "nib".

Done.

[commit-bot: I haz the power, 2016-12-09 20:23:47]

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

[commit-bot: I haz the power, 2016-12-09 20:23:48]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2016-12-09 21:15:08]

rsesek: can you review changes to build/mac/*?

[Robert Sesek, 2016-12-09 21:24:00]

lgtm

[lpromero, 2016-12-10 19:30:30]

Thank you!

[lpromero, 2016-12-10 19:30:37]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2016-12-10 19:30:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2564023002/#ps100001
(title: "nib is the output_extension")

[commit-bot: I haz the power, 2016-12-10 19:30:52]

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-12-10 21:15:08]

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

[commit-bot: I haz the power, 2016-12-10 21:15:09]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lpromero, 2016-12-10 22:55:06]

The CQ bit was checked by lpromero@chromium.org

[commit-bot: I haz the power, 2016-12-10 22:55:19]

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-12-10 23:48:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481410506219980,
"parent_rev": "97a091cffa5566f9d7746331a5bd1a6e10560c67", "commit_rev":
"a6d0bd73f65a139a6278658ce4f7603ea0208a97"}

[commit-bot: I haz the power, 2016-12-10 23:48:46]

Description was changed from

==========
Handle storyboards in GN.

BUG=none
R=sdefresne@chromium.org,rohitrao@chromium.org
==========

to

==========
Handle storyboards in GN.

BUG=none
R=sdefresne@chromium.org,rohitrao@chromium.org

Review-Url: https://codereview.chromium.org/2564023002
==========

[commit-bot: I haz the power, 2016-12-10 23:48:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-12 15:07:24]

Description was changed from

==========
Handle storyboards in GN.

BUG=none
R=sdefresne@chromium.org,rohitrao@chromium.org

Review-Url: https://codereview.chromium.org/2564023002
==========

to

==========
Handle storyboards in GN.

BUG=none
R=sdefresne@chromium.org,rohitrao@chromium.org

Committed: https://crrev.com/39fe426a5bec5352ddacab96ac10c6c33f9d5ac1
Cr-Commit-Position: refs/heads/master@{#437779}
==========

[commit-bot: I haz the power, 2016-12-12 15:07:25]

Patchset 6 (id:??) landed as
https://crrev.com/39fe426a5bec5352ddacab96ac10c6c33f9d5ac1
Cr-Commit-Position: refs/heads/master@{#437779}