Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1416513006:
Aura on Android: compile gn target extensions/browser. (Closed)
|
Created:
5 years, 1 month ago by Hadi Modified:
5 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@content_unittests Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAura on Android: build gn target extensions/browser.
Changes necessary to get extensions/browser build on Android when using use_aura=true. Most of these changes are because we don't have USB support for Aura on Android yet.
This is a prerequisite for building the "extensions" target for Aura on Android.
BUG=548797
Committed: https://crrev.com/23ae24f61bdb3945ffb3ef55e26de209d08cecf1
Cr-Commit-Position: refs/heads/master@{#360073}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed feedback from patch set 1. #
Total comments: 2
Patch Set 3 : Addressed feedback from patchset 2. #Patch Set 4 : Reordered imports in BUILD.gn to fix analyze error on android. #Patch Set 5 : rebase. #Patch Set 6 : Fix analyze error #
Total comments: 2
Patch Set 7 : add comments to features.gni. #Patch Set 8 : Fix comment. #
Total comments: 2
Patch Set 9 : addressed feedback. #Patch Set 10 : Revert the changes in features.gni. #
Dependent Patchsets: Messages
Total messages: 35 (11 generated)
Description was changed from ========== Aura on Android: build extensions_unittests_apk Changes necessary to get 'extensions_unittests_apk' target to build. BUG= ========== to ========== Aura on Android: build extensions_unittests_apk Changes necessary to get 'extensions_unittests_apk' target to build. BUG=548797 ==========
moshayedi@chromium.org changed reviewers: + mfomitchev@chromium.org, sievers@chromium.org
I see, so I'm guessing we're doing this because it's easier to compile most of this in for now, rather than untangling things to not depend on it. https://codereview.chromium.org/1416513006/diff/1/extensions/BUILD.gn File extensions/BUILD.gn (right): https://codereview.chromium.org/1416513006/diff/1/extensions/BUILD.gn#newcode262 extensions/BUILD.gn:262: #"//device/usb:mocks", don't the tests use this? https://codereview.chromium.org/1416513006/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1416513006/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_guest.cc:139: case base::TERMINATION_STATUS_OOM_PROTECTED: you can actually move this to _CRASHED. it really means that we *should* not have been killed by the OOM killer and therefore most likely just crashed https://codereview.chromium.org/1416513006/diff/1/extensions/extensions_tests... File extensions/extensions_tests.gypi (right): https://codereview.chromium.org/1416513006/diff/1/extensions/extensions_tests... extensions/extensions_tests.gypi:145: #'renderer/api/serial/data_sender_unittest.cc', ok should properly filter all these out based on is_android
https://codereview.chromium.org/1416513006/diff/1/device/usb/BUILD.gn File device/usb/BUILD.gn (right): https://codereview.chromium.org/1416513006/diff/1/device/usb/BUILD.gn#newcode61 device/usb/BUILD.gn:61: deps -= [ "//third_party/libusb" ] Created crbug.com/549257 https://codereview.chromium.org/1416513006/diff/1/extensions/common/BUILD.gn File extensions/common/BUILD.gn (right): https://codereview.chromium.org/1416513006/diff/1/extensions/common/BUILD.gn#... extensions/common/BUILD.gn:30: # GetTrustedICAPublicKey in extensions/browser which isn"t always linked ? https://codereview.chromium.org/1416513006/diff/1/extensions/common/BUILD.gn#... extensions/common/BUILD.gn:80: "api/printer_provider/usb_printer_manifest_handler.cc", Maybe we should create a stubbed out implementation in usb_printer_manifest_handler_android.cc, so that we don't have to ifdef stuff in common_manifest_handlers.cc and extensions_api_permissions.cc https://codereview.chromium.org/1416513006/diff/1/extensions/extensions_tests... File extensions/extensions_tests.gypi (right): https://codereview.chromium.org/1416513006/diff/1/extensions/extensions_tests... extensions/extensions_tests.gypi:145: #'renderer/api/serial/data_sender_unittest.cc', On 2015/10/28 20:12:23, sievers wrote: > ok should properly filter all these out based on is_android +1, but also: Why are we even making changes here? It's not part of the changes in the branch. Making unit tests compile/pass is probably best done after we upstream all the changes from the branch.
Description was changed from ========== Aura on Android: build extensions_unittests_apk Changes necessary to get 'extensions_unittests_apk' target to build. BUG=548797 ========== to ========== Aura on Android: compile extensions. Changes necessary to get extensions compile on Android when using use_aura=true. BUG=548797 ==========
https://codereview.chromium.org/1416513006/diff/1/device/usb/BUILD.gn File device/usb/BUILD.gn (right): https://codereview.chromium.org/1416513006/diff/1/device/usb/BUILD.gn#newcode61 device/usb/BUILD.gn:61: deps -= [ "//third_party/libusb" ] On 2015/10/29 19:31:55, mfomitchev wrote: > Created crbug.com/549257 Added a TODO comment. https://codereview.chromium.org/1416513006/diff/1/extensions/BUILD.gn File extensions/BUILD.gn (right): https://codereview.chromium.org/1416513006/diff/1/extensions/BUILD.gn#newcode262 extensions/BUILD.gn:262: #"//device/usb:mocks", On 2015/10/28 20:12:23, sievers wrote: > don't the tests use this? removed from this patch. https://codereview.chromium.org/1416513006/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1416513006/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_guest.cc:139: case base::TERMINATION_STATUS_OOM_PROTECTED: On 2015/10/28 20:12:23, sievers wrote: > you can actually move this to _CRASHED. it really means that we *should* not > have been killed by the OOM killer and therefore most likely just crashed Done. https://codereview.chromium.org/1416513006/diff/1/extensions/common/BUILD.gn File extensions/common/BUILD.gn (right): https://codereview.chromium.org/1416513006/diff/1/extensions/common/BUILD.gn#... extensions/common/BUILD.gn:80: "api/printer_provider/usb_printer_manifest_handler.cc", On 2015/10/29 19:31:55, mfomitchev wrote: > Maybe we should create a stubbed out implementation in > usb_printer_manifest_handler_android.cc, so that we don't have to ifdef stuff in > common_manifest_handlers.cc and extensions_api_permissions.cc Landed the stub in https://codereview.chromium.org/1409033007/. Removed the changes to common_manifest_handlers.cc and extensions_api_permissions.cc from this patch. https://codereview.chromium.org/1416513006/diff/1/extensions/extensions_tests... File extensions/extensions_tests.gypi (right): https://codereview.chromium.org/1416513006/diff/1/extensions/extensions_tests... extensions/extensions_tests.gypi:145: #'renderer/api/serial/data_sender_unittest.cc', On 2015/10/29 19:31:55, mfomitchev wrote: > On 2015/10/28 20:12:23, sievers wrote: > > ok should properly filter all these out based on is_android > > +1, but also: Why are we even making changes here? It's not part of the changes > in the branch. Making unit tests compile/pass is probably best done after we > upstream all the changes from the branch. Removed these changes from the CL and changed the title and description of the CL.
Description was changed from ========== Aura on Android: compile extensions. Changes necessary to get extensions compile on Android when using use_aura=true. BUG=548797 ========== to ========== Aura on Android: build extensions. Changes necessary to get extensions build on Android when using use_aura=true. Most of these changes are because we don't have USB support for Aura on Android yet. BUG=548797 ==========
Please clarify in the CL title and/or description which GN target you are building. Otherwise LGTM.
https://codereview.chromium.org/1416513006/diff/20001/extensions/browser/api/... File extensions/browser/api/serial/serial_api.cc (right): https://codereview.chromium.org/1416513006/diff/20001/extensions/browser/api/... extensions/browser/api/serial/serial_api.cc:88: #if !defined(OS_ANDROID) Add a TODO for crbug.com/549257 here as well
Description was changed from ========== Aura on Android: build extensions. Changes necessary to get extensions build on Android when using use_aura=true. Most of these changes are because we don't have USB support for Aura on Android yet. BUG=548797 ========== to ========== Aura on Android: build gn target extensions/browser. Changes necessary to get extensions/browser build on Android when using use_aura=true. Most of these changes are because we don't have USB support for Aura on Android yet. This is a prerequisite for building the "extensions" target for Aura on Android. BUG=548797 ==========
https://codereview.chromium.org/1416513006/diff/20001/extensions/browser/api/... File extensions/browser/api/serial/serial_api.cc (right): https://codereview.chromium.org/1416513006/diff/20001/extensions/browser/api/... extensions/browser/api/serial/serial_api.cc:88: #if !defined(OS_ANDROID) On 2015/11/04 22:06:37, mfomitchev wrote: > Add a TODO for crbug.com/549257 here as well Done.
moshayedi@chromium.org changed reviewers: + brettw@chromium.org, reillyg@chromium.org
reillyg@ can you please review the changes in device/usb and extensions/? brettw@ can you please review the changes in build/config/features.gni? thanks.
lgtm
lgtm
The CQ bit was checked by moshayedi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org Link to the patchset: https://codereview.chromium.org/1416513006/#ps40001 (title: "Addressed feedback from patchset 2.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416513006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416513006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
brettw@ can you please look at the new changes to features.gni? The previous version caused analyze errors in some trybots. The reason is that features.gni is always included before ui.gni, and use_aura gets assigned in ui.gni. We missed this problem our local tests since we always assigned use_aura in our "gn args" file. The solutions I could think of to resolve this problem was: * Change the import order, but to do this I had to touch all the files in which features.gni was imported: https://code.google.com/p/chromium/codesearch#search/&q=features.gni&sq=packa... * Check for defined(use_aura) before checking for use_aura. I thought the 2nd solution might be a bit cleaner. What do you think? If this works, I can add some comments which explains why I added defined(use_aura), and commit this. Thanks.
https://codereview.chromium.org/1416513006/diff/100001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1416513006/diff/100001/build/config/features.... build/config/features.gni:164: enable_extensions = (!is_android && !is_ios) || (defined(use_aura) && use_aura) Can you please add a comment explaining the use of "defined"? Also, this can be simplified a bit: is_mac || (defined(use_aura) && use_aura)
https://codereview.chromium.org/1416513006/diff/100001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1416513006/diff/100001/build/config/features.... build/config/features.gni:164: enable_extensions = (!is_android && !is_ios) || (defined(use_aura) && use_aura) Sorry, ignore the "simplification" part, that clearly won't work here, since use_aura would only have a valid value if it was explicitly defined.
On 2015/11/11 17:00:35, mfomitchev wrote: > https://codereview.chromium.org/1416513006/diff/100001/build/config/features.gni > File build/config/features.gni (right): > > https://codereview.chromium.org/1416513006/diff/100001/build/config/features.... > build/config/features.gni:164: enable_extensions = (!is_android && !is_ios) || > (defined(use_aura) && use_aura) > Can you please add a comment explaining the use of "defined"? > Also, this can be simplified a bit: > is_mac || (defined(use_aura) && use_aura) Done.
On 2015/11/12 17:47:15, Hadi wrote: > On 2015/11/11 17:00:35, mfomitchev wrote: > > > https://codereview.chromium.org/1416513006/diff/100001/build/config/features.gni > > File build/config/features.gni (right): > > > > > https://codereview.chromium.org/1416513006/diff/100001/build/config/features.... > > build/config/features.gni:164: enable_extensions = (!is_android && !is_ios) || > > (defined(use_aura) && use_aura) > > Can you please add a comment explaining the use of "defined"? > > Also, this can be simplified a bit: > > is_mac || (defined(use_aura) && use_aura) > > Done. I mean I added a comment.
https://codereview.chromium.org/1416513006/diff/140001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1416513006/diff/140001/build/config/features.... build/config/features.gni:165: # If use_aura is not explicitly set, defined(use_aura) is false here and it will For the second sentence, maybe just say "use_aura is assigned a value in ui.gni, so it will be undefined here unless it was explicitly provided in the build arguments."
https://codereview.chromium.org/1416513006/diff/140001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1416513006/diff/140001/build/config/features.... build/config/features.gni:165: # If use_aura is not explicitly set, defined(use_aura) is false here and it will On 2015/11/12 18:07:03, mfomitchev wrote: > For the second sentence, maybe just say "use_aura is assigned a value in ui.gni, > so it will be undefined here unless it was explicitly provided in the build > arguments." Done.
Removed the changes to features.gni per internal discussion. Planning to address this change and similar changes to build config in a separate CL.
Still LGTM
The CQ bit was checked by moshayedi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1416513006/#ps180001 (title: "Revert the changes in features.gni.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416513006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416513006/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/23ae24f61bdb3945ffb3ef55e26de209d08cecf1 Cr-Commit-Position: refs/heads/master@{#360073} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
