Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1038903002:
Upstream Android App Restrictions. (Closed)
DescriptionUpstream Android App Restrictions.
BUG=470877
Committed: https://crrev.com/efe21c3e50ca94de217760498a2158b83235532c
Cr-Commit-Position: refs/heads/master@{#322596}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Addressed comments: Chris #
Total comments: 29
Patch Set 3 : Addressed Comments: Philipp #Patch Set 4 : Refactor #Patch Set 5 : Add policy_templates as explicit deps #
Messages
Total messages: 17 (5 generated)
Patchset #1 (id:1) has been deleted
knn@chromium.org changed reviewers: + cjhopman@chromium.org
Any suggestions on handling the TODO @ line 257?
lgtm https://codereview.chromium.org/1038903002/diff/20001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/1038903002/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:5: import("//build/config/android/rules.gni") This should only be imported for android builds (it'll assert on non-android).
On 2015/03/26 15:13:11, knn wrote: > Any suggestions on handling the TODO @ line 257? Not really. We should file a bug for that and maybe I'll get some time to look at it and comment there.
knn@chromium.org changed reviewers: + pneubeck@chromium.org
@Chris, Done thanks! Created crbug.com/471115 to track that. @ Philipp, PTAL. https://codereview.chromium.org/1038903002/diff/20001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/1038903002/diff/20001/components/policy/BUILD... components/policy/BUILD.gn:5: import("//build/config/android/rules.gni") On 2015/03/26 22:05:08, cjhopman wrote: > This should only be imported for android builds (it'll assert on non-android). Done.
I guess all comments on the .GN apply to the gyp as well. I'm not really knowledgeable about either build system. Maybe you want someone to review it also who knows them better. Maybe cjhopman does. https://codereview.chromium.org/1038903002/diff/40001/components/policy.gypi File components/policy.gypi (right): https://codereview.chromium.org/1038903002/diff/40001/components/policy.gypi#... components/policy.gypi:353: 'destination': '<(input_resources_dir)/xml-v21/', same question as in the GN file: It seems weird to copy into an input folder. https://codereview.chromium.org/1038903002/diff/40001/components/policy.gypi#... components/policy.gypi:377: 'all_dependent_settings': { i don't know what this is for. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:243: resources_zip = "res.java/$target_name.zip" do we really have a directory with a dot in its name? "res.java" ? https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:244: input_resources_dir = "$root_gen_dir/chrome/app/policy/android" can you maybe rename 'input_resources_dir' to something that highlights that this is a temporary folder? https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:245: input_resources = [ "$input_resources_dir/xml-v21/app_restrictions.xml" ] input_resources should be moved to the zip action https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:246: input_resources += policy_templates_android_outputs aren't you supposed to depend on the target that produces these outputs, namely policy_templates ? https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:247: build_config_path = "$target_gen_dir/$target_name.build_config" move to __build_config https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:250: "$root_gen_dir/policy/app_restrictions.xml", nit: remove newlines https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:253: "$root_gen_dir/chrome/app/policy/android/xml-v21/app_restrictions.xml", == "$input_resources_dir/xml-v21/app_restrictions.xml" https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:268: outputs = [ nit: remove newlines https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:273: args = [ can you make the use of '=' vs. two array entries consistent? https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:281: ":${target_name}__consolidate_resources", is it guaranteed that these deps are executed in order?
Addressed Comments. PTAL. https://codereview.chromium.org/1038903002/diff/40001/components/policy.gypi File components/policy.gypi (right): https://codereview.chromium.org/1038903002/diff/40001/components/policy.gypi#... components/policy.gypi:353: 'destination': '<(input_resources_dir)/xml-v21/', On 2015/03/27 09:09:24, pneubeck wrote: > same question as in the GN file: It seems weird to copy into an input folder. This is mainly because I felt all outputs of a target should have a single parent directory at some level which is not root. This may lead to confusion if we start generating resources at arbitrary places. [End of opinion] I can remove this copy stage if you disagree. 'cloud_policy_code_generate' generates files into, SHARED_INTERMEDIATE_DIR/policy/policy 'policy_templates' generates files into, SHARED_INTERMEDIATE_DIR/chrome/app/policy/<platform-name> Can we move cloud_policy_code_generate into SHARED_INTERMEDIATE_DIR/chrome/app/policy/core or something? https://codereview.chromium.org/1038903002/diff/40001/components/policy.gypi#... components/policy.gypi:377: 'all_dependent_settings': { On 2015/03/27 09:09:24, pneubeck wrote: > i don't know what this is for. This is a setting used in android resource targets to transfer input resource dependencies to the final resource target. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:243: resources_zip = "res.java/$target_name.zip" On 2015/03/27 09:09:24, pneubeck wrote: > do we really have a directory with a dot in its name? "res.java" ? yes https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:244: input_resources_dir = "$root_gen_dir/chrome/app/policy/android" On 2015/03/27 09:09:24, pneubeck wrote: > can you maybe rename 'input_resources_dir' to something that highlights that > this is a temporary folder? Done. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:245: input_resources = [ "$input_resources_dir/xml-v21/app_restrictions.xml" ] On 2015/03/27 09:09:24, pneubeck wrote: > input_resources should be moved to the zip action Sorry this is actually used below. My bad. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:246: input_resources += policy_templates_android_outputs On 2015/03/27 09:09:25, pneubeck wrote: > aren't you supposed to depend on the target that produces these outputs, namely > policy_templates ? GN derives dependencies by looking at the inputs/outputs list. the deps section in line 281 does not really need consolidate_resources section but it seems to be a style to include all deps if any are specified. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:247: build_config_path = "$target_gen_dir/$target_name.build_config" On 2015/03/27 09:09:25, pneubeck wrote: > move to __build_config Can't do that as $target_name will change. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:250: "$root_gen_dir/policy/app_restrictions.xml", On 2015/03/27 09:09:24, pneubeck wrote: > nit: remove newlines GN has it's own format tool that fails presubmit checks. We probably have to file a bug for any nit issue. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:253: "$root_gen_dir/chrome/app/policy/android/xml-v21/app_restrictions.xml", On 2015/03/27 09:09:24, pneubeck wrote: > == "$input_resources_dir/xml-v21/app_restrictions.xml" Thanks for this! Don't know how this ended up this way. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:268: outputs = [ On 2015/03/27 09:09:24, pneubeck wrote: > nit: remove newlines Answered in line 250. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:273: args = [ On 2015/03/27 09:09:25, pneubeck wrote: > can you make the use of '=' vs. two array entries consistent? Not sure if I understand correctly but do you mean = [ entry ] vs = [ entry, ] Then it is a Nit issue again. Answered in line 250. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:281: ":${target_name}__consolidate_resources", On 2015/03/27 09:09:24, pneubeck wrote: > is it guaranteed that these deps are executed in order? As I answered above, GN derives dependencies from inputs/outputs. It is customary (admittedly in downstream code) to include all the sub dependencies. I can remove __consolidate_resources though without any effect if you want me to.
https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:246: input_resources += policy_templates_android_outputs On 2015/03/27 11:46:49, knn wrote: > On 2015/03/27 09:09:25, pneubeck wrote: > > aren't you supposed to depend on the target that produces these outputs, > namely > > policy_templates ? > > GN derives dependencies by looking at the inputs/outputs list. > the deps section in line 281 does not really need consolidate_resources section > but it seems to be a style to include all deps if any are specified. If you don't see concrete reason (like common practice) why not to depend on policy_templates, then please add it to deps. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:247: build_config_path = "$target_gen_dir/$target_name.build_config" On 2015/03/27 11:46:49, knn wrote: > On 2015/03/27 09:09:25, pneubeck wrote: > > move to __build_config > > Can't do that as $target_name will change. I looked a bit more into this. It seems to be common to use target_name for templates. Here, you're not in a template. Why not giving all of these nested actions a fixed name and using fixed paths like you partially already do? I.e. couldn't you remove all usages of target_name from your change? https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:268: outputs = [ On 2015/03/27 11:46:49, knn wrote: > On 2015/03/27 09:09:24, pneubeck wrote: > > nit: remove newlines > > Answered in line 250. If this is anyways subject to autoformatting, then forget about the style nits. https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:273: args = [ On 2015/03/27 11:46:49, knn wrote: > On 2015/03/27 09:09:25, pneubeck wrote: > > can you make the use of '=' vs. two array entries consistent? > > Not sure if I understand correctly but do you mean = [ entry ] vs > = [ > entry, > ] > Then it is a Nit issue again. Answered in line 250. No, I meant the notation ..., "--XYZ=$VAR", ... vs. ..., "--XYZ", VAR, ... https://codereview.chromium.org/1038903002/diff/40001/components/policy/BUILD... components/policy/BUILD.gn:281: ":${target_name}__consolidate_resources", On 2015/03/27 11:46:49, knn wrote: > On 2015/03/27 09:09:24, pneubeck wrote: > > is it guaranteed that these deps are executed in order? > > As I answered above, GN derives dependencies from inputs/outputs. > It is customary (admittedly in downstream code) to include all the sub > dependencies. I can remove __consolidate_resources though without any effect if > you want me to. It seems to me that deps should be complete (if you have output files of actions that no one depends on, the action would never be executed). I read some documentation that if the deps is given other forms of dependencies become redundant. So, please don't remove it.
I think I have addressed all the comments, except for removing $target_name.
I think that is much more readable than replacing with
app_restriction_resources_{blah blah} (I actually tried it out)
Further this prefix avoids any namespace clashes.
We could also just use simple names and set visibility to this target though if
you dislike the prefix but I think the present one is more concise and readable.
One additional minor point, once the TODO above write_build_config is addressed
this will be much cleaner.
thanks, lgtm
The CQ bit was checked by knn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1038903002/#ps100001 (title: "Add policy_templates as explicit deps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038903002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/efe21c3e50ca94de217760498a2158b83235532c Cr-Commit-Position: refs/heads/master@{#322596} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
