Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006273005:
[ios Mojo] Integration test for Mojo WebUI. (Closed)
|
Created:
4 years, 7 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios Mojo] Integration test for Mojo WebUI.
Tests communication between WebUI page and the native code.
When page is loaded it sends "syn" message to the native code,
when native code receives "syn" it replies back with "ack",
when page receives "ack" it replies back with "fin". Once "fin" is
received the test succeeds.
BUG=567809
Committed: https://crrev.com/dc9e9709213948d4db6308f71c4097950b9da8fa
Cr-Commit-Position: refs/heads/master@{#396972}
Patch Set 1 #Patch Set 2 : Updated build files #
Total comments: 15
Patch Set 3 : Updated GN files #
Total comments: 6
Patch Set 4 : Addressed review comments #
Total comments: 4
Patch Set 5 : Addressed review comments #
Total comments: 4
Patch Set 6 : Addressed review comments #Patch Set 7 : Removed "data" from ios_web_inttest GN target. #
Total comments: 17
Patch Set 8 : Addressed review comments (GN and GYP) #
Total comments: 9
Patch Set 9 : Fixed GN #
Total comments: 1
Messages
Total messages: 64 (25 generated)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2006273005/diff/20001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/20001/ios/web/BUILD.gn#newcod... ios/web/BUILD.gn:492: data = [ "data" is not used in iOS, remove. If you want the file to be in the application bundle, use bundle_data target instead ("git grep bundle_data" for examples). https://codereview.chromium.org/2006273005/diff/20001/ios/web/BUILD.gn#newcod... ios/web/BUILD.gn:551: output_dir = "$root_gen_dir/ios/web" This should not be necessary, output_dir defaults to target_gen_dir (see "tools/grit/grit_rule.gni" line 329). target_gen_dir is by default equal to "$root_gen_dir/ios/web" for that target (for a target defined in "//foo/bar/BUILD.gn", then target_gen_dir is equal to "$root_gen_dir/foo/bar"). You can confirm this by checking that the output of //ios/web:resources_grit target are located into //$root_out_dir/gen/ios/web using the following command without your patch: $ gn desc out/gn-Debug-iphonesimulator //ios/web:resources_grit outputs //out/gn-Debug-iphonesimulator/gen/ios/web/resources_stamp.d.stamp //out/gn-Debug-iphonesimulator/gen/ios/web/grit/ios_web_resources.h //out/gn-Debug-iphonesimulator/gen/ios/web/ios_web_resources.pak Remove. https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:15: repack("repacked_resources") { Rename the target "packed_resources" as it is the more common name in BUILD.gn files. If you want the files to be in the generated application bundle, use repack_and_bundle instead: repack_and_bundle("packed_resources") { testonly = true sources = [ "$root_gen_dir/ios/web/ios_web_resources.pak", "$root_gen_dir/ios/web/test/test_resources.pak", ] output = "$target_gen_dir/resources.pack" bundle_output = "{{bundle_resources_dir}}/{{source_file_part}}" } https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:16: sources = [ testonly = true https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:24: output = "$target_gen_dir/ios/web/test/resources.pak" See my previous comment about target_gen_dir, this should be: output = "$target_gen_dir/resources.pak" https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:28: source = "test_resources.grd" testonly = true https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:36: "root_out_dir=" + rebase_path(root_out_dir, root_build_dir), This should be "root_gen_dir=".
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/40001
Description was changed from ========== [ios Mojo] Integration test for Mojo WebUI. BUG= ========== to ========== [ios Mojo] Integration test for Mojo WebUI. Tests communication between WebUI page and the native code. When page is loaded it sends "syn" message to the native code, when native code receives "syn" it replies back with "ack", when page receives "ack" it replies back with "fin". Once "fin" is received the test succeeds. BUG=567809 ==========
eugenebut@chromium.org changed reviewers: + jyquinn@chromium.org, rockot@chromium.org
Jackie, Ken, could you please review the test. I will work with Sylvain to fix GN file, which is still incorrect. Thanks! https://codereview.chromium.org/2006273005/diff/20001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/20001/ios/web/BUILD.gn#newcod... ios/web/BUILD.gn:492: data = [ On 2016/05/26 17:45:18, sdefresne wrote: > "data" is not used in iOS, remove. > > If you want the file to be in the application bundle, use bundle_data target > instead ("git grep bundle_data" for examples). Done. https://codereview.chromium.org/2006273005/diff/20001/ios/web/BUILD.gn#newcod... ios/web/BUILD.gn:551: output_dir = "$root_gen_dir/ios/web" On 2016/05/26 17:45:18, sdefresne wrote: > This should not be necessary, output_dir defaults to target_gen_dir (see > "tools/grit/grit_rule.gni" line 329). target_gen_dir is by default equal to > "$root_gen_dir/ios/web" for that target (for a target defined in > "//foo/bar/BUILD.gn", then target_gen_dir is equal to "$root_gen_dir/foo/bar"). > > You can confirm this by checking that the output of //ios/web:resources_grit > target are located into //$root_out_dir/gen/ios/web using the following command > without your patch: > > $ gn desc out/gn-Debug-iphonesimulator //ios/web:resources_grit outputs > //out/gn-Debug-iphonesimulator/gen/ios/web/resources_stamp.d.stamp > //out/gn-Debug-iphonesimulator/gen/ios/web/grit/ios_web_resources.h > //out/gn-Debug-iphonesimulator/gen/ios/web/ios_web_resources.pak > > > Remove. Done. https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:15: repack("repacked_resources") { On 2016/05/26 17:45:19, sdefresne wrote: > Rename the target "packed_resources" as it is the more common name in BUILD.gn > files. > > If you want the files to be in the generated application bundle, use > repack_and_bundle instead: > > repack_and_bundle("packed_resources") { > testonly = true > sources = [ > "$root_gen_dir/ios/web/ios_web_resources.pak", > "$root_gen_dir/ios/web/test/test_resources.pak", > ] > output = "$target_gen_dir/resources.pack" > bundle_output = > "{{bundle_resources_dir}}/{{source_file_part}}" > } Done except I could not add testonly because of this error: ERROR at //ios/web/test/BUILD.gn:17:14: Assignment had no effect. testonly = true https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:16: sources = [ On 2016/05/26 17:45:19, sdefresne wrote: > testonly = true This gives me an error: ERROR at //ios/web/test/BUILD.gn:17:14: Assignment had no effect. testonly = true ^--- You set the variable "testonly" here and it was unused before it went out of scope. See //ios/web/BUILD.gn:317:5: which caused the file to be included. "//ios/web/test:mojo_bindings", ^----------------------------- https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:24: output = "$target_gen_dir/ios/web/test/resources.pak" On 2016/05/26 17:45:19, sdefresne wrote: > See my previous comment about target_gen_dir, this should be: > > output = "$target_gen_dir/resources.pak" > Done. https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:28: source = "test_resources.grd" On 2016/05/26 17:45:19, sdefresne wrote: > testonly = true Gives me same error: ERROR at //ios/web/test/BUILD.gn:26:14: Assignment had no effect. testonly = true https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:36: "root_out_dir=" + rebase_path(root_out_dir, root_build_dir), On 2016/05/26 17:45:19, sdefresne wrote: > This should be "root_gen_dir=". Done.
Sylvain, GN build still fails: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bu... Could you please take a look. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:49: // Received "syn" message from WebUI page, send "ack" as reply. Can you add some logic to ensure that syn is received exactly once, and always before fin? https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:54: // Received "fin" from the WebUI page in response to "ack". Similarly can you also check that fin_received_ is false here? https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:67: class TestUIHandlerFactory : public shell::InterfaceFactory<TestUIHandlerMojo> { nit: I would just merge this into TestUIHandler. It can implement shell::InterfaceFactory<TestUIHandlerMojo> in addition to TestUIHandlerMojo.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:49: // Received "syn" message from WebUI page, send "ack" as reply. On 2016/05/26 21:11:51, Ken Rockot wrote: > Can you add some logic to ensure that syn is received exactly once, and always > before fin? Done. https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:54: // Received "fin" from the WebUI page in response to "ack". On 2016/05/26 21:11:51, Ken Rockot wrote: > Similarly can you also check that fin_received_ is false here? Done. https://codereview.chromium.org/2006273005/diff/40001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:67: class TestUIHandlerFactory : public shell::InterfaceFactory<TestUIHandlerMojo> { On 2016/05/26 21:11:51, Ken Rockot wrote: > nit: I would just merge this into TestUIHandler. It can implement > shell::InterfaceFactory<TestUIHandlerMojo> in addition to TestUIHandlerMojo. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/60001
lgtm with nits https://codereview.chromium.org/2006273005/diff/60001/ios/web/webui/web_ui_mo... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2006273005/diff/60001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:74: // Bindings required by shell::InterfaceFactory. nit: This comment isn't really accurate or particularly helpful. InterfaceFactory certainly doesn't require bindings, it's just an interface through which the system can hand you pipe handles. Bindings are required to bind pipe handles, but the type fairly self-documenting IMHO. https://codereview.chromium.org/2006273005/diff/60001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:77: bool syn_received_; nit: use default values instead of initializers? i.e. bool syn_received_ = false;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
https://codereview.chromium.org/2006273005/diff/60001/ios/web/webui/web_ui_mo... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2006273005/diff/60001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:74: // Bindings required by shell::InterfaceFactory. On 2016/05/26 22:00:40, Ken Rockot wrote: > nit: This comment isn't really accurate or particularly helpful. > InterfaceFactory certainly doesn't require bindings, it's just an interface > through which the system can hand you pipe handles. Bindings are required to > bind pipe handles, but the type fairly self-documenting IMHO. Done. https://codereview.chromium.org/2006273005/diff/60001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:77: bool syn_received_; On 2016/05/26 22:00:40, Ken Rockot wrote: > nit: use default values instead of initializers? i.e. bool syn_received_ = > false; Done.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
https://codereview.chromium.org/2006273005/diff/80001/ios/web/webui/web_ui_mo... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2006273005/diff/80001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:36: // that communication was successfull. See test WebUI page for more details: s/successfull/successful https://codereview.chromium.org/2006273005/diff/80001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:37: // ios/web/test/data/mojo_test.js You've got a circular reference here, since mojo_test.js points to this file for "more detailed documentation." One could argue that by "more details," you mean "read the code." Maybe you could say "See test WebUI page for implementation details," or something to that effect.
Meant to say lgtm with comment nits
eugenebut@chromium.org changed reviewers: + thestig@chromium.org
Thanks! +thestig@ for resources https://codereview.chromium.org/2006273005/diff/80001/ios/web/webui/web_ui_mo... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2006273005/diff/80001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:36: // that communication was successfull. See test WebUI page for more details: On 2016/05/27 01:03:41, Jackie Quinn wrote: > s/successfull/successful Done. https://codereview.chromium.org/2006273005/diff/80001/ios/web/webui/web_ui_mo... ios/web/webui/web_ui_mojo_inttest.mm:37: // ios/web/test/data/mojo_test.js On 2016/05/27 01:03:41, Jackie Quinn wrote: > You've got a circular reference here, since mojo_test.js points to this file for > "more detailed documentation." > > One could argue that by "more details," you mean "read the code." Maybe you > could say "See test WebUI page for implementation details," or something to that > effect. Done.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
lgtm, but you have red bots
Thanks. Yeah, I know about GN problem, hopefully Sylvain will help me to fix them :)
https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/20001/ios/web/test/BUILD.gn#n... ios/web/test/BUILD.gn:15: repack("repacked_resources") { On 2016/05/26 20:53:32, Eugene But wrote: > On 2016/05/26 17:45:19, sdefresne wrote: > > Rename the target "packed_resources" as it is the more common name in BUILD.gn > > files. > > > > If you want the files to be in the generated application bundle, use > > repack_and_bundle instead: > > > > repack_and_bundle("packed_resources") { > > testonly = true > > sources = [ > > "$root_gen_dir/ios/web/ios_web_resources.pak", > > "$root_gen_dir/ios/web/test/test_resources.pak", > > ] > > output = "$target_gen_dir/resources.pack" > > bundle_output = > > "{{bundle_resources_dir}}/{{source_file_part}}" > > } > > Done except I could not add testonly because of this error: > > ERROR at //ios/web/test/BUILD.gn:17:14: Assignment had no effect. > testonly = true > Ack, this is because the "repack" template did not forward the variable. I've created https://codereview.chromium.org/2006273005 to address this. Can you still add the "testonly" here and everywhere else but commented with a TODO to remove it? We can then fix it as soon as my other CL lands. https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:475: "test:mojo_bindings", Please only use relative name for target defined in the same BUILD.gn file (this makes it much easier to rename/move targets), so this should be: "//ios/web/test:mojo_bindings", "//ios/web/test:packed_resources", "//ios/web/test:resources", https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:538: source = "ios_web_resources.grd" testonly = true https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:539: use_qualified_include = true There is an indirect dependency on generated file ""${root_gen_dir}/ios/web/test/mojo_test.mojom.js" so you need to add it to the "inputs" variable and add a dependency on the target that generate that file: inputs = [ "${root_gen_dir}/ios/web/test/mojo_test.mojom.js", ] deps = [ "//ios/web/test:mojo_bindings", ] https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:546: "root_gen_dir=" + rebase_path(root_out_dir, root_build_dir), You want to have the grit variable "root_gen_dir" expands to the value of the gn variable "root_gen_dir", so you should use the following: grit_flags = [ "-E", "root_gen_dir=" + rebase_path(root_gen_dir, root_build_dir), ] If you look at the tool failure, you'll see the error is: IOError: [Errno 2] No such file or directory: u'../../out/Release-iphoneos/ios/web/test/mojo_test.mojom.js' This was because the file tried to include "${root_gen_dir}/ios/web/test/mojo_test.mojom.js" and "root_gen_dir" grit variable was expanded from this argument "-E root_gen_dir=../Release-iphoneos" while it should have been "-E gen". https://codereview.chromium.org/2006273005/diff/120001/ios/web/ios_web_inttes... File ios/web/ios_web_inttests.gyp (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/ios_web_inttes... ios/web/ios_web_inttests.gyp:52: 'hard_dependency': 1, As in GN, this target needs a dependency on the target that generated the file mojo_test.mojom.js (i.e. mojo_bindings). https://codereview.chromium.org/2006273005/diff/120001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:34: ] Ditto, this needs to list "${root_gen_dir}/ios/web/test/mojo_test.mojom.js" in inputs and needs a dependency on ":mojo_bindings". inputs = [ "${root_gen_dir}/ios/web/test/mojo_test.mojom.js", ] deps = [ ":mojo_bindings", ] https://codereview.chromium.org/2006273005/diff/120001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:37: "root_gen_dir=" + rebase_path(root_out_dir, root_build_dir), Ditto, if you want the grit variable "root_gen_dir" to correspond to the gn variable "root_gen_dir", you need the following: grit_flats = [ "-E", "root_gen_dir=" + rebase_path(root_gen_dir, root_build_dir), ] https://codereview.chromium.org/2006273005/diff/120001/tools/gritsettings/res... File tools/gritsettings/resource_ids (right): https://codereview.chromium.org/2006273005/diff/120001/tools/gritsettings/res... tools/gritsettings/resource_ids:187: "includes": [24000], You should use the same value as content/shell/shell_resources.grd as we do not pack them on iOS. This will leave more space of ios/web/ios_web_resources.grd.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Sylvain, resource file is still not copied :( https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:475: "test:mojo_bindings", On 2016/05/28 13:04:01, sdefresne wrote: > Please only use relative name for target defined in the same BUILD.gn file (this > makes it much easier to rename/move targets), so this should be: > > "//ios/web/test:mojo_bindings", > "//ios/web/test:packed_resources", > "//ios/web/test:resources", Done. https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:538: source = "ios_web_resources.grd" On 2016/05/28 13:04:02, sdefresne wrote: > testonly = true This is not test only thing. This is for production. https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:539: use_qualified_include = true On 2016/05/28 13:04:01, sdefresne wrote: > There is an indirect dependency on generated file > ""${root_gen_dir}/ios/web/test/mojo_test.mojom.js" so you need to add it to the > "inputs" variable and add a dependency on the target that generate that file: > > inputs = [ > "${root_gen_dir}/ios/web/test/mojo_test.mojom.js", > ] > deps = [ > "//ios/web/test:mojo_bindings", > ] ditto. This target does not contain any test resources. https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:546: "root_gen_dir=" + rebase_path(root_out_dir, root_build_dir), On 2016/05/28 13:04:02, sdefresne wrote: > You want to have the grit variable "root_gen_dir" expands to the value of the gn > variable "root_gen_dir", so you should use the following: > > grit_flags = [ > "-E", > "root_gen_dir=" + rebase_path(root_gen_dir, root_build_dir), > ] > > If you look at the tool failure, you'll see the error is: > IOError: [Errno 2] No such file or directory: > u'../../out/Release-iphoneos/ios/web/test/mojo_test.mojom.js' > > This was because the file tried to include > "${root_gen_dir}/ios/web/test/mojo_test.mojom.js" and "root_gen_dir" grit > variable was expanded from this argument "-E root_gen_dir=../Release-iphoneos" > while it should have been "-E gen". Done. Thank you for explanation! https://codereview.chromium.org/2006273005/diff/120001/ios/web/ios_web_inttes... File ios/web/ios_web_inttests.gyp (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/ios_web_inttes... ios/web/ios_web_inttests.gyp:52: 'hard_dependency': 1, On 2016/05/28 13:04:02, sdefresne wrote: > As in GN, this target needs a dependency on the target that generated the file > mojo_test.mojom.js (i.e. mojo_bindings). Done. https://codereview.chromium.org/2006273005/diff/120001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:34: ] On 2016/05/28 13:04:02, sdefresne wrote: > Ditto, this needs to list "${root_gen_dir}/ios/web/test/mojo_test.mojom.js" in > inputs and needs a dependency on ":mojo_bindings". > > inputs = [ > "${root_gen_dir}/ios/web/test/mojo_test.mojom.js", > ] > deps = [ > ":mojo_bindings", > ] Done. https://codereview.chromium.org/2006273005/diff/120001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:37: "root_gen_dir=" + rebase_path(root_out_dir, root_build_dir), On 2016/05/28 13:04:02, sdefresne wrote: > Ditto, if you want the grit variable "root_gen_dir" to correspond to the gn > variable "root_gen_dir", you need the following: > > grit_flats = [ > "-E", > "root_gen_dir=" + rebase_path(root_gen_dir, root_build_dir), > ] Done. https://codereview.chromium.org/2006273005/diff/120001/tools/gritsettings/res... File tools/gritsettings/resource_ids (right): https://codereview.chromium.org/2006273005/diff/120001/tools/gritsettings/res... tools/gritsettings/resource_ids:187: "includes": [24000], On 2016/05/28 13:04:02, sdefresne wrote: > You should use the same value as content/shell/shell_resources.grd as we do not > pack them on iOS. This will leave more space of ios/web/ios_web_resources.grd. Done. https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:478: "//ios/web/test:packed_resources", Sylvain, this does not work and resources were not copied: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn...
https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/120001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:538: source = "ios_web_resources.grd" On 2016/05/31 17:26:56, Eugene But wrote: > On 2016/05/28 13:04:02, sdefresne wrote: > > testonly = true > This is not test only thing. This is for production. I made this comment because the only grd file that appears to use root_gen_dir in the cl (ios/web/test/test_resources.grd ) is to include generated mojo_test.mojom.js that I assume to be a test file. So either the root_gen_dir is unissued and should be removed or there is a dependency on a test file in that production target which is a bug. https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:478: "//ios/web/test:packed_resources", On 2016/05/31 17:26:57, Eugene But wrote: > Sylvain, this does not work and resources were not copied: > https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn... [7323:1803:0531/100248:467360551880:ERROR:memory_mapped_file.cc(52)] Couldn't open /tmp/tmpBMv0p0/Library/Developer/CoreSimulator/Devices/C37C31D1-756F-446A-894C-477B80960FE0/data/Containers/Bundle/Application/92EEC7EC-0607-4355-988D-E80637AE77E9/ios_web_inttests.app/resources.pak It looks for resources.pak but the target generate resources.pack. https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:544: grit_flags = [ I think you should remove this. https://codereview.chromium.org/2006273005/diff/140001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/140001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:16: sources = [ Please add testonly as it is now supported: testonly = true https://codereview.chromium.org/2006273005/diff/140001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:24: output = "$target_gen_dir/resources.pack" This needs to be (.pak not .pack): output = "$target_gen_dir/resources.pak"
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/160001
Looks like compilation with gn succeeded. Lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn File ios/web/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:478: "//ios/web/test:packed_resources", On 2016/05/31 18:52:25, sdefresne wrote: > On 2016/05/31 17:26:57, Eugene But wrote: > > Sylvain, this does not work and resources were not copied: > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn... > > [7323:1803:0531/100248:467360551880:ERROR:memory_mapped_file.cc(52)] Couldn't > open > /tmp/tmpBMv0p0/Library/Developer/CoreSimulator/Devices/C37C31D1-756F-446A-894C-477B80960FE0/data/Containers/Bundle/Application/92EEC7EC-0607-4355-988D-E80637AE77E9/ios_web_inttests.app/resources.pak > > > It looks for resources.pak but the target generate resources.pack. D'oh! Sorry about that :) https://codereview.chromium.org/2006273005/diff/140001/ios/web/BUILD.gn#newco... ios/web/BUILD.gn:544: grit_flags = [ On 2016/05/31 18:52:26, sdefresne wrote: > I think you should remove this. Done. https://codereview.chromium.org/2006273005/diff/140001/ios/web/test/BUILD.gn File ios/web/test/BUILD.gn (right): https://codereview.chromium.org/2006273005/diff/140001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:16: sources = [ On 2016/05/31 18:52:26, sdefresne wrote: > Please add testonly as it is now supported: > > testonly = true Done. https://codereview.chromium.org/2006273005/diff/140001/ios/web/test/BUILD.gn#... ios/web/test/BUILD.gn:24: output = "$target_gen_dir/resources.pack" On 2016/05/31 18:52:26, sdefresne wrote: > This needs to be (.pak not .pack): > > output = "$target_gen_dir/resources.pak" Done.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, jyquinn@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2006273005/#ps160001 (title: "Fixed GN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006273005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006273005/160001
Message was sent while issue was closed.
Description was changed from ========== [ios Mojo] Integration test for Mojo WebUI. Tests communication between WebUI page and the native code. When page is loaded it sends "syn" message to the native code, when native code receives "syn" it replies back with "ack", when page receives "ack" it replies back with "fin". Once "fin" is received the test succeeds. BUG=567809 ========== to ========== [ios Mojo] Integration test for Mojo WebUI. Tests communication between WebUI page and the native code. When page is loaded it sends "syn" message to the native code, when native code receives "syn" it replies back with "ack", when page receives "ack" it replies back with "fin". Once "fin" is received the test succeeds. BUG=567809 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [ios Mojo] Integration test for Mojo WebUI. Tests communication between WebUI page and the native code. When page is loaded it sends "syn" message to the native code, when native code receives "syn" it replies back with "ack", when page receives "ack" it replies back with "fin". Once "fin" is received the test succeeds. BUG=567809 ========== to ========== [ios Mojo] Integration test for Mojo WebUI. Tests communication between WebUI page and the native code. When page is loaded it sends "syn" message to the native code, when native code receives "syn" it replies back with "ack", when page receives "ack" it replies back with "fin". Once "fin" is received the test succeeds. BUG=567809 Committed: https://crrev.com/dc9e9709213948d4db6308f71c4097950b9da8fa Cr-Commit-Position: refs/heads/master@{#396972} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/dc9e9709213948d4db6308f71c4097950b9da8fa Cr-Commit-Position: refs/heads/master@{#396972}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2027983002/ by stkhapugin@chromium.org. The reason for reverting is: Breaks downstream ios_internal_unittests execution because ios_internal_unittests already has resource load code and the bundle ends up being loaded twice. Please remove this code downstream, also see comments in code review tool. .
Message was sent while issue was closed.
stkhapugin@chromium.org changed reviewers: + stkhapugin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2006273005/diff/160001/ios/web/test/web_test_... File ios/web/test/web_test_suite.mm (right): https://codereview.chromium.org/2006273005/diff/160001/ios/web/test/web_test_... ios/web/test/web_test_suite.mm:49: // Load test resources if they are present in the bundle. This code should be: // Force unittests to run using en-US so if we test against string // output, it'll pass regardless of the system language. ui::ResourceBundle::InitSharedInstanceWithLocale( "en-US", nullptr, ui::ResourceBundle::LOAD_COMMON_RESOURCES); base::FilePath resources_pack_path; PathService::Get(ios::FILE_RESOURCES_PACK, &resources_pack_path); ResourceBundle::GetSharedInstance().AddDataPackFromPath( resources_pack_path, ui::SCALE_FACTOR_NONE); because this code doesn't force locale to en-US and it should not use literal but should use PathService to get the name of the resource pack to load.
Message was sent while issue was closed.
On 2016/06/01 09:47:40, stkhapugin wrote: > https://codereview.chromium.org/2006273005/diff/160001/ios/web/test/web_test_... > File ios/web/test/web_test_suite.mm (right): > > https://codereview.chromium.org/2006273005/diff/160001/ios/web/test/web_test_... > ios/web/test/web_test_suite.mm:49: // Load test resources if they are present in > the bundle. > This code should be: > > // Force unittests to run using en-US so if we test against string > // output, it'll pass regardless of the system language. > ui::ResourceBundle::InitSharedInstanceWithLocale( > "en-US", nullptr, ui::ResourceBundle::LOAD_COMMON_RESOURCES); > base::FilePath resources_pack_path; > PathService::Get(ios::FILE_RESOURCES_PACK, &resources_pack_path); > ResourceBundle::GetSharedInstance().AddDataPackFromPath( > resources_pack_path, ui::SCALE_FACTOR_NONE); > > because this code doesn't force locale to en-US and it should not use literal > but should use PathService to get the name of the resource pack to load. There is another problem with the patch as is, as the following messages are printed to stderr when building with gyp+ninja: ninja: Entering directory `/b/build/slave/ios-simulator/build/src/out/Debug-iphonesimulator' ninja: warning: multiple rules generate gen/ios/web/test/mojo_test.mojom.cc. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] ninja: warning: multiple rules generate gen/ios/web/test/mojo_test.mojom.h. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] ninja: warning: multiple rules generate gen/ios/web/test/mojo_test.mojom-internal.h. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] ninja: warning: multiple rules generate gen/ios/web/test/mojo_test.mojom.js. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu... This should be fixed in any reland as it means that the build is currently flaky. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
