Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083383003:
Reland: Refactor process_resources.py to use aapt's --extra-packages (Closed)
DescriptionReland: Refactor process_resources.py to use aapt's --extra-packages
Reason for Reland:
- No longer generating assignments for non-array styleable fields within
onResourcesLoaded() (matches aapt's behaviour)
We actually never pass --include-all-resources, so this is just
refactoring dead code. I think it's an improvement though,
as the new comments explain more of why things work the way they do.
TBR=agrieve
BUG=none
Committed: https://crrev.com/c04a207f591c1cae455fee6c9c7d8b0dbca20abe
Cr-Commit-Position: refs/heads/master@{#401355}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 23 (8 generated)
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083383003/1
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Reland: Refactor process_resources.py to use aapt's --extra-packages Reason for Reland: - No longer generating assignments for non-array styleable fields within onResourcesLoaded() (matches aapt's behaviour) We actually never pass --include-all-resources, so this is just refactoring dead code. I think it's an improvement though, as the new comments explain more of why things work the way they do. BUG=none ========== to ========== Reland: Refactor process_resources.py to use aapt's --extra-packages Reason for Reland: - No longer generating assignments for non-array styleable fields within onResourcesLoaded() (matches aapt's behaviour) We actually never pass --include-all-resources, so this is just refactoring dead code. I think it's an improvement though, as the new comments explain more of why things work the way they do. TBR=agrieve BUG=none ==========
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083383003/1
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
do you have the delta between what you landed originally & the fixed version?
On 2016/06/22 17:03:19, jbudorick (EMEA til June 30) wrote:
> do you have the delta between what you landed originally & the fixed version?
Erg, sorry, forgot to upload the original :(.
The only difference is that there are now changes to: def
_CreateExtraRJavaFile(). The specific fix is:
- {% if e.java_type != 'int[]' %}
+ {% if resource_type != 'styleable' and e.java_type != 'int[]' %}
Message was sent while issue was closed.
Description was changed from ========== Reland: Refactor process_resources.py to use aapt's --extra-packages Reason for Reland: - No longer generating assignments for non-array styleable fields within onResourcesLoaded() (matches aapt's behaviour) We actually never pass --include-all-resources, so this is just refactoring dead code. I think it's an improvement though, as the new comments explain more of why things work the way they do. TBR=agrieve BUG=none ========== to ========== Reland: Refactor process_resources.py to use aapt's --extra-packages Reason for Reland: - No longer generating assignments for non-array styleable fields within onResourcesLoaded() (matches aapt's behaviour) We actually never pass --include-all-resources, so this is just refactoring dead code. I think it's an improvement though, as the new comments explain more of why things work the way they do. TBR=agrieve BUG=none ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Reland: Refactor process_resources.py to use aapt's --extra-packages Reason for Reland: - No longer generating assignments for non-array styleable fields within onResourcesLoaded() (matches aapt's behaviour) We actually never pass --include-all-resources, so this is just refactoring dead code. I think it's an improvement though, as the new comments explain more of why things work the way they do. TBR=agrieve BUG=none ========== to ========== Reland: Refactor process_resources.py to use aapt's --extra-packages Reason for Reland: - No longer generating assignments for non-array styleable fields within onResourcesLoaded() (matches aapt's behaviour) We actually never pass --include-all-resources, so this is just refactoring dead code. I think it's an improvement though, as the new comments explain more of why things work the way they do. TBR=agrieve BUG=none Committed: https://crrev.com/c04a207f591c1cae455fee6c9c7d8b0dbca20abe Cr-Commit-Position: refs/heads/master@{#401355} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c04a207f591c1cae455fee6c9c7d8b0dbca20abe Cr-Commit-Position: refs/heads/master@{#401355}
Message was sent while issue was closed.
On 2016/06/22 17:56:03, agrieve wrote:
> On 2016/06/22 17:03:19, jbudorick (EMEA til June 30) wrote:
> > do you have the delta between what you landed originally & the fixed
version?
>
> Erg, sorry, forgot to upload the original :(.
>
> The only difference is that there are now changes to: def
> _CreateExtraRJavaFile(). The specific fix is:
> - {% if e.java_type != 'int[]' %}
> + {% if resource_type != 'styleable' and e.java_type != 'int[]' %}
sgtm / lgtm
Message was sent while issue was closed.
phoglund@chromium.org changed reviewers: + phoglund@chromium.org
Message was sent while issue was closed.
This CL breaks WebRTC: https://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14367 when the import bot attempts to import it: https://codereview.webrtc.org/2096523002/ Is this fixable with a simple CL in WebRTC? Otherwise, please revert this one.
Message was sent while issue was closed.
On 2016/06/23 12:08:24, phoglund_chrome wrote: > This CL breaks WebRTC: > https://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14367 > > when the import bot attempts to import it: > https://codereview.webrtc.org/2096523002/ > > Is this fixable with a simple CL in WebRTC? Otherwise, please revert this one. looking, not sure if agrieve is online yet
Message was sent while issue was closed.
https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... File build/android/gyp/process_resources.py (left): https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:480: input_paths.extend(p for p in options.extra_r_text_files if os.path.exists(p)) Pretty sure this is responsible for the webrtc failure?
Message was sent while issue was closed.
https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... File build/android/gyp/process_resources.py (left): https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:480: input_paths.extend(p for p in options.extra_r_text_files if os.path.exists(p)) On 2016/06/23 12:33:00, jbudorick (EMEA til June 30) wrote: > Pretty sure this is responsible for the webrtc failure? likely. Looking now to see if it'll fix it. There's another CL that depends on this one, so if it's a simple fix I'd rather do it than revert both changes.
Message was sent while issue was closed.
On 2016/06/23 12:49:36, agrieve wrote: > https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... > File build/android/gyp/process_resources.py (left): > > https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... > build/android/gyp/process_resources.py:480: input_paths.extend(p for p in > options.extra_r_text_files if os.path.exists(p)) > On 2016/06/23 12:33:00, jbudorick (EMEA til June 30) wrote: > > Pretty sure this is responsible for the webrtc failure? > > likely. Looking now to see if it'll fix it. There's another CL that depends on > this one, so if it's a simple fix I'd rather do it than revert both changes. Looks like for some reason the GYP rules tell process_resources.py to take its own R.txt as an input: https://cs.chromium.org/chromium/src/build/java_apk.gypi?q=java_apk.gypi&sq=p... Bizarre... But I'll just revert for now since I'm not able to gclient sync to make a fix atm.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2090303003/ by agrieve@chromium.org. The reason for reverting is: Broke webrtc GYP rules..
Message was sent while issue was closed.
On 2016/06/23 13:08:40, agrieve wrote: > On 2016/06/23 12:49:36, agrieve wrote: > > > https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... > > File build/android/gyp/process_resources.py (left): > > > > > https://codereview.chromium.org/2083383003/diff/1/build/android/gyp/process_r... > > build/android/gyp/process_resources.py:480: input_paths.extend(p for p in > > options.extra_r_text_files if os.path.exists(p)) > > On 2016/06/23 12:33:00, jbudorick (EMEA til June 30) wrote: > > > Pretty sure this is responsible for the webrtc failure? > > > > likely. Looking now to see if it'll fix it. There's another CL that depends on > > this one, so if it's a simple fix I'd rather do it than revert both changes. > > Looks like for some reason the GYP rules tell process_resources.py to take its > own R.txt as an input: > https://cs.chromium.org/chromium/src/build/java_apk.gypi?q=java_apk.gypi&sq=p... > > Bizarre... But I'll just revert for now since I'm not able to gclient sync to > make a fix atm. That's too bad. I'll be around on Monday in case there's any way I can help (OOO tomorrow and rest of next week). kjellander@ is going OOO as well. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
