Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
414823002:
[android_webview] Don't unconditionally add pak files to system image. (Closed)
|
Created:
6 years, 5 months ago by mkosiba (inactive) Modified:
6 years, 4 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[android_webview] Don't unconditionally add pak files to system image.
The make rules we have make it impossible to not include the
android_webview pak files in the system image.
BUG=396741
R=primiano@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285893
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 15 (0 generated)
PTAL
this should work now
The CQ bit was checked by mkosiba@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/414823002/20001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional 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.
https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... File android_webview/build/resources_config.mk (right): https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... android_webview/build/resources_config.mk:21: android_webview_asset_dirs := \ This seems to be unused, or am I missing something? https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... android_webview/build/resources_config.mk:30: android_webview_system_pak_targets := \ Do you plan to remove at this point the similar set of dependencies we have in the Android tree (in frameworks/webview/chromium/Android.mk)? https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... android_webview/build/resources_config.mk:91: android_webview_intermediates_pak_additional_deps := \ Is this for enforcing a dependency onto the shortname.pak? Can you add a comment to explain who will be depending on those? https://codereview.chromium.org/414823002/diff/20001/android_webview/webview_... File android_webview/webview_pak.mk (right): https://codereview.chromium.org/414823002/diff/20001/android_webview/webview_... android_webview/webview_pak.mk:22: include $(CLEAR_VARS) Can you please add a comment before lines 8 and 22, just to explain what these two rules are doing and where are they copying to? something like: # Copies the ???.pak into out/target/product/foo/system/frameworks/webview/paks/en.pak
lgtm https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... File android_webview/build/resources_config.mk (right): https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... android_webview/build/resources_config.mk:21: android_webview_asset_dirs := \ On 2014/07/28 08:01:02, Primiano Tucci wrote: > This seems to be unused, or am I missing something? Ok, we just had a discussion offline. Can you just add a comment to this variable and _additional_deps, to clarify that these are used both by the upstream Android.mk and the one in the glue layer?
The CQ bit was checked by mkosiba@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/414823002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34007) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/39021) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34011) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... File android_webview/build/resources_config.mk (right): https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... android_webview/build/resources_config.mk:30: android_webview_system_pak_targets := \ On 2014/07/28 08:01:02, Primiano Tucci wrote: > Do you plan to remove at this point the similar set of dependencies we have in > the Android tree (in frameworks/webview/chromium/Android.mk)? yes. https://codereview.chromium.org/414823002/diff/20001/android_webview/build/re... android_webview/build/resources_config.mk:91: android_webview_intermediates_pak_additional_deps := \ On 2014/07/28 08:01:02, Primiano Tucci wrote: > Is this for enforcing a dependency onto the shortname.pak? > Can you add a comment to explain who will be depending on those? Done. https://codereview.chromium.org/414823002/diff/20001/android_webview/webview_... File android_webview/webview_pak.mk (right): https://codereview.chromium.org/414823002/diff/20001/android_webview/webview_... android_webview/webview_pak.mk:22: include $(CLEAR_VARS) On 2014/07/28 08:01:02, Primiano Tucci wrote: > Can you please add a comment before lines 8 and 22, just to explain what these > two rules are doing and where are they copying to? something like: > # Copies the ???.pak into > out/target/product/foo/system/frameworks/webview/paks/en.pak Done.
Message was sent while issue was closed.
Committed patchset #4 manually as r285893 (presubmit successful). |
