Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667023002:
Android: Add owned resources to android studio (Closed)
DescriptionAndroid: Add owned resources to android studio
Include owned resources and unzip owned resource zips in android studio
generated gradle projects. This allows a res directory to show up for
each project.
BUG=682846
Review-Url: https://codereview.chromium.org/2667023002
Cr-Commit-Position: refs/heads/master@{#447764}
Committed: https://chromium.googlesource.com/chromium/src/+/9af0deb4bd0d2eb6ec47063ce2a024641e0da52d
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix for review #Patch Set 3 : missed one #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
wnwen@chromium.org changed reviewers: + estevenson@chromium.org
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.
Lgtm!
wnwen@chromium.org changed reviewers: + agrieve@chromium.org
Thanks for the review, Eric! +agrieve@ for OWNERS and review.
lgtm https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:217: res_dirs = entry.DepsInfo().get('owned_resources_dirs', []) nit: you're mutating the list that lives in the config. It's not hurting anything, but generally should be avoided. Just duplicated it with list(...) https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:218: zips = entry.DepsInfo().get('owned_resources_zips', []) nit: don't need the default value of [] since you guard on it on the next line https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:220: res_dir = os.path.join(self.EntryOutputDir(entry), _RES_SUBDIR) nit: add a comment that these are just for generated resources (e.g. grit(), jinja_template_resources()) https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:222: logging.info('Extracting %s to %s', zip_path, res_dir) maybe: Make a helper function for this and share it with _ExtractSrcjars() https://codereview.chromium.org/2667023002/diff/1/docs/android_studio.md File docs/android_studio.md (right): https://codereview.chromium.org/2667023002/diff/1/docs/android_studio.md#newc... docs/android_studio.md:121: * Some resources are unzipped so edits won't be reflected. Probably don't need to say this since this looks to apply only to generated resources (which have comments at the top saying "generated by ...")
PTAL, thanks for the review. https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:217: res_dirs = entry.DepsInfo().get('owned_resources_dirs', []) On 2017/02/01 19:24:12, agrieve wrote: > nit: you're mutating the list that lives in the config. It's not hurting > anything, but generally should be avoided. Just duplicated it with list(...) Done. https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:218: zips = entry.DepsInfo().get('owned_resources_zips', []) On 2017/02/01 19:24:12, agrieve wrote: > nit: don't need the default value of [] since you guard on it on the next line Done. https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:220: res_dir = os.path.join(self.EntryOutputDir(entry), _RES_SUBDIR) On 2017/02/01 19:24:12, agrieve wrote: > nit: add a comment that these are just for generated resources (e.g. grit(), > jinja_template_resources()) Done. https://codereview.chromium.org/2667023002/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:222: logging.info('Extracting %s to %s', zip_path, res_dir) On 2017/02/01 19:24:12, agrieve wrote: > maybe: Make a helper function for this and share it with _ExtractSrcjars() Done. https://codereview.chromium.org/2667023002/diff/1/docs/android_studio.md File docs/android_studio.md (right): https://codereview.chromium.org/2667023002/diff/1/docs/android_studio.md#newc... docs/android_studio.md:121: * Some resources are unzipped so edits won't be reflected. On 2017/02/01 19:24:12, agrieve wrote: > Probably don't need to say this since this looks to apply only to generated > resources (which have comments at the top saying "generated by ...") Done.
The CQ bit was checked by wnwen@chromium.org 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.
still lgtm :)
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estevenson@chromium.org Link to the patchset: https://codereview.chromium.org/2667023002/#ps40001 (title: "missed one")
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": 40001, "attempt_start_ts": 1486044716342190,
"parent_rev": "5a1ebea8a247366ac822858e141983037a1f0059", "commit_rev":
"9af0deb4bd0d2eb6ec47063ce2a024641e0da52d"}
Message was sent while issue was closed.
Description was changed from ========== Android: Add owned resources to android studio Include owned resources and unzip owned resource zips in android studio generated gradle projects. This allows a res directory to show up for each project. BUG=682846 ========== to ========== Android: Add owned resources to android studio Include owned resources and unzip owned resource zips in android studio generated gradle projects. This allows a res directory to show up for each project. BUG=682846 Review-Url: https://codereview.chromium.org/2667023002 Cr-Commit-Position: refs/heads/master@{#447764} Committed: https://chromium.googlesource.com/chromium/src/+/9af0deb4bd0d2eb6ec47063ce2a0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9af0deb4bd0d2eb6ec47063ce2a0...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2678463003/ by agrieve@chromium.org. The reason for reverting is: Script now fails if resources aren't already built. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
