Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808993003:
Build ZIP archiver in ui/file_manager. (Closed)
DescriptionBuild ZIP archiver.
This CL adds and changes some files in ui/file_manager to build ZIP archiver.
We are going to add ZIP archiver to the list of component extensions
in another CL, so ZIP archiver still won't show up in the context
menu with this patch.
BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2808993003
Cr-Commit-Position: refs/heads/master@{#464309}
Committed: https://chromium.googlesource.com/chromium/src/+/77065d9816b34a5cc7913894b8b756e7c3e6fb79
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix indentation and add some comments. #
Total comments: 4
Patch Set 3 : Build ZIP archiver. #
Total comments: 4
Patch Set 4 : Fix the code so that the built successes with enable_nable=false. #Patch Set 5 : Temporally disabled zip packing from the context menu. #
Messages
Total messages: 33 (18 generated)
Description was changed from ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested ========== to ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
takise@google.com changed reviewers: + mtomasz@chromium.org
The CQ bit was checked by takise@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager to build ZIP archiver. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Thanks!
mtomasz@chromium.org changed reviewers: + fukino@chromium.org
I'm not very familiar with GN. Deferring review to @fukino who may have more expertise here. https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... File ui/file_manager/zip_archiver/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... ui/file_manager/zip_archiver/BUILD.gn:10: "//ui/file_manager/zip_archiver/zip_archiver/cpp:zip_archiver_pnacl(//build/toolchain/nacl:newlib_pnacl)", nit: Is this indent correct? https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... File ui/file_manager/zip_archiver/manifest.json (right): https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... ui/file_manager/zip_archiver/manifest.json:2: "key": "MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCxGxJCOLUzHIYc812NFoBC1eV8PhOTuF6he3gSuqzxckUyrDLdl5++DAd1AkQkv6i8SSMWFvDKLg2b+zfCOwk6P7uu3tqNavXXy61Okaq5HKF3xhciNDl4zF6ZlegvE9AhJOTo2eCHVIMS0+YuK5hyno/+xMwN4byvsrOYXQnhcJeOHxkFb9TfVUb3SOBgl4pBZ7+EIMNntEvzY7mxjBzOgnCjBePvwnoMRyAqljCJarz2WSbUOLP3yoCuH9vPKj+0D6hF1woXmd6qBr0ln/7tHdbr1cYmkosfFuJO2y6d00FAJY/G5L6o8JAEBbWG5D0qELt+aBjkG0uos5gcR4ZPAgMBAAECggEBAK3aIjFJU25J6MjQiRvvY5a4O56bnUIb8SDZgAP6pbwZ7R2R9hiaN6AqVMOiptvgHDZAISYU/OerD4b3s0OCCkvYtlcxwh6iSZQ9BvIighFWrpZRqPHVjDktfQuNIS/dZiiy+9Yr0oFmD4jS45idCPgy+K0h6CEUX9GlPTEq24ElECDwQHVyB9LHdenleCdvldIEDxf6/D+zkc/PmCPlZPfwdppK6wgH2GvgqbxV+OoSnNp0XhNinjCN37P5yAo4xEi0UGOxOwkNGkJn0V5bYjH6/JHzmdVH74D40N4/Fcy0bC79oFGeiP0ZzW8AAArfIxbxStodWlBOCsTVtvi4RMECgYEA2pyZRThGx4OH8uXCC94LkdpVjKvLvbUlIVd2zk3UEFpWujgmAI+erkAaE1JSlcJpFNSlfonTX1vQuMgTOfnK7soy4677P1CMQH++GxjMWRIAQsMyx7vLtKOISr5/vQQKAyuFmxzt9xbMOmPzqWxwkuuiF74GtPgE5VXslhvsoyECgYEAz2U7L6YS4u2aMRK4DMDxcf/hZ3BxwHmUl5euknRNcaFJSdv6392y8w3t9Lz7sp8CK56GADXL1bmLrDgg2tlL82f60rtPd6IOoJk10uMmCnyjbZh7aJzuw1CTSs+dwi6qpGUB4YbJn8R2AN79SHxUb4dwVOh4lHeNa415Wka+a28CgYA3Vf5iDB22cO/fpxLYSCtrjvWqtu3KpmiwqOAU1pSAUy2y03WjHLeQ6f7vtx3adKx+rlj5z89mSuppa5OaUEVy7lG1WlyUqUHnLa6kU0GepjTUsW5QKpQktGRSbygMY1JZfRHDsq31ppqpiRVrZFyWg/iyw9IUytcKahaJ5KWgoQKBgFbgY/ugyNaQi3+1BK4rALktZAGNo8jp5SnfWzx0RaCs3GN5J80xNG4GTsCvjYwUebdF74IVBu7fi7e3x2OFlQBAdVxjJHXLx+7UXyyZBG1uKpOVRVTcMFRW42x6Le6S196HhVMwwDMR/BB/WIBNvJz/kjmvLBudPPtpxwTfD5M3AoGBALrrXX4QwqBiq4q09SPKoeOwlV35QETUhQaAKKag9aSrNMONcf77TXUBZ0d9Z+tabHLTGGa6E7q2BL82NdZSZvVeVWA+KaE4ezW2t5KyZqg14Cc0uY9Xys9VkFcVgMqsvtkUzDvAVJcmNAgcrMIEiapUR6LPrneLLXH1ikOt+hM8", nit: Please add an extension id comment, like in: https://cs.chromium.org/chromium/src/ui/file_manager/image_loader/manifest.js...
fukino@ PTAL. Thanks! https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... File ui/file_manager/zip_archiver/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... ui/file_manager/zip_archiver/BUILD.gn:10: "//ui/file_manager/zip_archiver/zip_archiver/cpp:zip_archiver_pnacl(//build/toolchain/nacl:newlib_pnacl)", On 2017/04/11 09:21:34, mtomasz wrote: > nit: Is this indent correct? Done. https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... File ui/file_manager/zip_archiver/manifest.json (right): https://codereview.chromium.org/2808993003/diff/1/ui/file_manager/zip_archive... ui/file_manager/zip_archiver/manifest.json:2: "key": "MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCxGxJCOLUzHIYc812NFoBC1eV8PhOTuF6he3gSuqzxckUyrDLdl5++DAd1AkQkv6i8SSMWFvDKLg2b+zfCOwk6P7uu3tqNavXXy61Okaq5HKF3xhciNDl4zF6ZlegvE9AhJOTo2eCHVIMS0+YuK5hyno/+xMwN4byvsrOYXQnhcJeOHxkFb9TfVUb3SOBgl4pBZ7+EIMNntEvzY7mxjBzOgnCjBePvwnoMRyAqljCJarz2WSbUOLP3yoCuH9vPKj+0D6hF1woXmd6qBr0ln/7tHdbr1cYmkosfFuJO2y6d00FAJY/G5L6o8JAEBbWG5D0qELt+aBjkG0uos5gcR4ZPAgMBAAECggEBAK3aIjFJU25J6MjQiRvvY5a4O56bnUIb8SDZgAP6pbwZ7R2R9hiaN6AqVMOiptvgHDZAISYU/OerD4b3s0OCCkvYtlcxwh6iSZQ9BvIighFWrpZRqPHVjDktfQuNIS/dZiiy+9Yr0oFmD4jS45idCPgy+K0h6CEUX9GlPTEq24ElECDwQHVyB9LHdenleCdvldIEDxf6/D+zkc/PmCPlZPfwdppK6wgH2GvgqbxV+OoSnNp0XhNinjCN37P5yAo4xEi0UGOxOwkNGkJn0V5bYjH6/JHzmdVH74D40N4/Fcy0bC79oFGeiP0ZzW8AAArfIxbxStodWlBOCsTVtvi4RMECgYEA2pyZRThGx4OH8uXCC94LkdpVjKvLvbUlIVd2zk3UEFpWujgmAI+erkAaE1JSlcJpFNSlfonTX1vQuMgTOfnK7soy4677P1CMQH++GxjMWRIAQsMyx7vLtKOISr5/vQQKAyuFmxzt9xbMOmPzqWxwkuuiF74GtPgE5VXslhvsoyECgYEAz2U7L6YS4u2aMRK4DMDxcf/hZ3BxwHmUl5euknRNcaFJSdv6392y8w3t9Lz7sp8CK56GADXL1bmLrDgg2tlL82f60rtPd6IOoJk10uMmCnyjbZh7aJzuw1CTSs+dwi6qpGUB4YbJn8R2AN79SHxUb4dwVOh4lHeNa415Wka+a28CgYA3Vf5iDB22cO/fpxLYSCtrjvWqtu3KpmiwqOAU1pSAUy2y03WjHLeQ6f7vtx3adKx+rlj5z89mSuppa5OaUEVy7lG1WlyUqUHnLa6kU0GepjTUsW5QKpQktGRSbygMY1JZfRHDsq31ppqpiRVrZFyWg/iyw9IUytcKahaJ5KWgoQKBgFbgY/ugyNaQi3+1BK4rALktZAGNo8jp5SnfWzx0RaCs3GN5J80xNG4GTsCvjYwUebdF74IVBu7fi7e3x2OFlQBAdVxjJHXLx+7UXyyZBG1uKpOVRVTcMFRW42x6Le6S196HhVMwwDMR/BB/WIBNvJz/kjmvLBudPPtpxwTfD5M3AoGBALrrXX4QwqBiq4q09SPKoeOwlV35QETUhQaAKKag9aSrNMONcf77TXUBZ0d9Z+tabHLTGGa6E7q2BL82NdZSZvVeVWA+KaE4ezW2t5KyZqg14Cc0uY9Xys9VkFcVgMqsvtkUzDvAVJcmNAgcrMIEiapUR6LPrneLLXH1ikOt+hM8", On 2017/04/11 09:21:34, mtomasz wrote: > nit: Please add an extension id comment, like in: > https://cs.chromium.org/chromium/src/ui/file_manager/image_loader/manifest.js... Done.
fukino@chromium.org changed reviewers: + hashimoto@chromium.org
Looking good to me, but I don't have much experience on edition gn files. hashimoto@ san, can I ask you a double check?
I'm not an expert of grit and nacl so please forgive me if my comments are irrelevant. https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager_resources.grd (right): https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager_resources.grd:209: <include name="IDR_ZIP_ARCHIVER_MODULE_NMF" file="zip_archiver/module.nmf.txt" type="BINDATA" /> Why this file's extension is ".txt"? https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager_resources.grd:210: <include name="IDR_ZIP_ARCHIVER_PEXE" file="${pexe}" use_base_dir="false" type="BINDATA" /> "pexe" sounds too general. How about a more specific name like "zip_archiver_pexe"? https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/zip_arc... File ui/file_manager/zip_archiver/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/zip_arc... ui/file_manager/zip_archiver/BUILD.gn:18: "$pexe_dir/../gen/ui/file_manager/{{source_file_part}}.js", 1. Why do we need to copy this file once, instead of directly referencing the pexe file in ui/file_manager/BUILD.gn? 2. Can't we avoid using "../" in the path? 3. Why is the extension ".js"?
https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/zip_arc... File ui/file_manager/zip_archiver/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/20001/ui/file_manager/zip_arc... ui/file_manager/zip_archiver/BUILD.gn:18: "$pexe_dir/../gen/ui/file_manager/{{source_file_part}}.js", On 2017/04/12 07:59:43, hashimoto wrote: > 1. Why do we need to copy this file once, instead of directly referencing the > pexe file in ui/file_manager/BUILD.gn? > 3. Why is the extension ".js"? This is actually a really tricky workaround which ImageLoader already uses to load NaCL module. Basically, .grd file can not read files that end with .pexe or .nmf. https://cs.chromium.org/chromium/src/ui/file_manager/image_loader/piex_loader... NaCL toolchain in GN automatically create .pexe file, so we need to change the file extension here. I tried to use .pexe.txt instead of .pexe before, but it didn't work. That's why I'm using .pexe.js here. I investigated this issue for a few days before, but I couldn't find out why .pexe and .nmf don't work, so now I just followed the same way as ImageLoader. > 2. Can't we avoid using "../" in the path? Done.
https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/BUILD.gn File ui/file_manager/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/BUILD.g... ui/file_manager/BUILD.gn:22: "//ui/file_manager/zip_archiver/:zip_archiver(//build/toolchain/nacl:newlib_pnacl)", Shouldn't this be just //ui/file_manager/zip_archiver/:zip_archiver? Do you need to specify the toolchain here? https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/zip_arc... File ui/file_manager/zip_archiver/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/zip_arc... ui/file_manager/zip_archiver/BUILD.gn:7: if (enable_nacl) { Shouldn't this check be performed at a more upper level (e.g. excluding zip_archiver entirely wnen enable_nacl==false)? Please make sure that you can build even when enable_nacl=false.
PTAL again, thanks! https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/BUILD.gn File ui/file_manager/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/BUILD.g... ui/file_manager/BUILD.gn:22: "//ui/file_manager/zip_archiver/:zip_archiver(//build/toolchain/nacl:newlib_pnacl)", On 2017/04/12 10:45:37, hashimoto wrote: > Shouldn't this be just //ui/file_manager/zip_archiver/:zip_archiver? > Do you need to specify the toolchain here? Done. https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/zip_arc... File ui/file_manager/zip_archiver/BUILD.gn (right): https://codereview.chromium.org/2808993003/diff/40001/ui/file_manager/zip_arc... ui/file_manager/zip_archiver/BUILD.gn:7: if (enable_nacl) { On 2017/04/12 10:45:37, hashimoto wrote: > Shouldn't this check be performed at a more upper level (e.g. excluding > zip_archiver entirely wnen enable_nacl==false)? > Please make sure that you can build even when enable_nacl=false. Done.
GN changes lgtm
The CQ bit was checked by takise@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hashimoto@ Thank you for the review! mtomasz@, fukino@ I think I also need lgtm from one of you two.
lgtm
The CQ bit was checked by takise@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2808993003/#ps80001 (title: "Temporally disabled zip packing from the context menu.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492057482394800,
"parent_rev": "84d7dbe51cfa93499956ef7551eef7dd1a1a9ec9", "commit_rev":
"77065d9816b34a5cc7913894b8b756e7c3e6fb79"}
Message was sent while issue was closed.
Description was changed from ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager to build ZIP archiver. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Build ZIP archiver. This CL adds and changes some files in ui/file_manager to build ZIP archiver. We are going to add ZIP archiver to the list of component extensions in another CL, so ZIP archiver still won't show up in the context menu with this patch. BUG=607078 TEST=manually tested CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2808993003 Cr-Commit-Position: refs/heads/master@{#464309} Committed: https://chromium.googlesource.com/chromium/src/+/77065d9816b34a5cc7913894b8b7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/77065d9816b34a5cc7913894b8b7...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2811183006/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit(https://goo.gl/kROfz5) identified CL at revision 464309 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
On 2017/04/13 05:45:57, findit-for-me wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/2811183006/ by > mailto:findit-for-me@appspot.gserviceaccount.com. > > The reason for reverting is: > Findit(https://goo.gl/kROfz5) identified CL at revision 464309 as the > culprit for failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb.... This sounds correct, landing the revert. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
