Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1551393002:
Move use_webview_internal_framework to android_webview/BUILD.gn (Closed)
|
Created:
4 years, 11 months ago by agrieve Modified:
4 years, 11 months ago Reviewers:
michaelbai CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove use_webview_internal_framework to android_webview/BUILD.gn
BUG=574108
Committed: https://crrev.com/2aceeb64bdf3d8504ad07d6a3a194cd736b4db3e
Cr-Commit-Position: refs/heads/master@{#367830}
Patch Set 1 #
Total comments: 2
Depends on Patchset: Messages
Total messages: 13 (3 generated)
agrieve@chromium.org changed reviewers: + michaelbai@chromium.org
₪₪ ₪₪
https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... File build/config/android/config.gni (left): https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... build/config/android/config.gni:113: use_webview_internal_framework = false Sorry, I still don't understand what caused the error and why this variable need to be removed. From gn help declare_args, "Introduces the given arguments into the current scope. If they are not specified on the command line or in a toolchain's arguments, the default values given in the declare_args block will be used." IIUC, the default value of use_webview_internal_framework should be defined here, then, it could be used every where, did I miss something?
https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... File build/config/android/config.gni (left): https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... build/config/android/config.gni:113: use_webview_internal_framework = false On 2016/01/06 03:53:42, michaelbai wrote: > Sorry, I still don't understand what caused the error and why this variable need > to be removed. > > From gn help declare_args, > > "Introduces the given arguments into the current scope. If they are > not specified on the command line or in a toolchain's arguments, > the default values given in the declare_args block will be used." > > IIUC, the default value of use_webview_internal_framework should be defined > here, then, it could be used every where, did I miss something? > > > > Looks like the problem doesn't happen anymore because 8d2e9c0cb301a83e5e700166e3119ca66bb88b75 was reverted ("Port Monochrome to GN"). Maybe this was the reason for the revert? If you revert the revert, I suspect you'll see the gn gen failure. The problem is just that the order is: 1. GN starts parsing //build/android/config.gni 2. GN hits import("//clank/config.gni") 3. //clank/config.gni references use_webview_internal_framework 4. GN hits the declare_args() block which defines use_webview_internal_framework #3 fails because it needs to happen after #4
On 2016/01/06 04:16:04, agrieve wrote: > https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... > File build/config/android/config.gni (left): > > https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... > build/config/android/config.gni:113: use_webview_internal_framework = false > On 2016/01/06 03:53:42, michaelbai wrote: > > Sorry, I still don't understand what caused the error and why this variable > need > > to be removed. > > > > From gn help declare_args, > > > > "Introduces the given arguments into the current scope. If they are > > not specified on the command line or in a toolchain's arguments, > > the default values given in the declare_args block will be used." > > > > IIUC, the default value of use_webview_internal_framework should be defined > > here, then, it could be used every where, did I miss something? > > > > > > > > > > Looks like the problem doesn't happen anymore because > 8d2e9c0cb301a83e5e700166e3119ca66bb88b75 was reverted ("Port Monochrome to GN"). > Maybe this was the reason for the revert? > > If you revert the revert, I suspect you'll see the gn gen failure. > > The problem is just that the order is: > > 1. GN starts parsing //build/android/config.gni > 2. GN hits import("//clank/config.gni") > 3. //clank/config.gni references use_webview_internal_framework > 4. GN hits the declare_args() block which defines > use_webview_internal_framework > > #3 fails because it needs to happen after #4 It only happened for the new directory of gn args, did it expose GN's issue?
lgtm
On 2016/01/06 06:15:44, michaelbai wrote: > On 2016/01/06 04:16:04, agrieve wrote: > > > https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... > > File build/config/android/config.gni (left): > > > > > https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... > > build/config/android/config.gni:113: use_webview_internal_framework = false > > On 2016/01/06 03:53:42, michaelbai wrote: > > > Sorry, I still don't understand what caused the error and why this variable > > need > > > to be removed. > > > > > > From gn help declare_args, > > > > > > "Introduces the given arguments into the current scope. If they are > > > not specified on the command line or in a toolchain's arguments, > > > the default values given in the declare_args block will be used." > > > > > > IIUC, the default value of use_webview_internal_framework should be defined > > > here, then, it could be used every where, did I miss something? > > > > > > > > > > > > > > > > Looks like the problem doesn't happen anymore because > > 8d2e9c0cb301a83e5e700166e3119ca66bb88b75 was reverted ("Port Monochrome to > GN"). > > Maybe this was the reason for the revert? > > > > If you revert the revert, I suspect you'll see the gn gen failure. > > > > The problem is just that the order is: > > > > 1. GN starts parsing //build/android/config.gni > > 2. GN hits import("//clank/config.gni") > > 3. //clank/config.gni references use_webview_internal_framework > > 4. GN hits the declare_args() block which defines > > use_webview_internal_framework > > > > #3 fails because it needs to happen after #4 > > It only happened for the new directory of gn args, did it expose GN's issue? For me it happens in an existing directory. Could be that it works if you set the arg explicitly in your GN args?
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/1551393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551393002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Move use_webview_internal_framework to android_webview/BUILD.gn BUG=574108 ========== to ========== Move use_webview_internal_framework to android_webview/BUILD.gn BUG=574108 Committed: https://crrev.com/2aceeb64bdf3d8504ad07d6a3a194cd736b4db3e Cr-Commit-Position: refs/heads/master@{#367830} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2aceeb64bdf3d8504ad07d6a3a194cd736b4db3e Cr-Commit-Position: refs/heads/master@{#367830}
Message was sent while issue was closed.
On 2016/01/06 14:55:58, agrieve wrote: > On 2016/01/06 06:15:44, michaelbai wrote: > > On 2016/01/06 04:16:04, agrieve wrote: > > > > > > https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... > > > File build/config/android/config.gni (left): > > > > > > > > > https://codereview.chromium.org/1551393002/diff/1/build/config/android/config... > > > build/config/android/config.gni:113: use_webview_internal_framework = false > > > On 2016/01/06 03:53:42, michaelbai wrote: > > > > Sorry, I still don't understand what caused the error and why this > variable > > > need > > > > to be removed. > > > > > > > > From gn help declare_args, > > > > > > > > "Introduces the given arguments into the current scope. If they are > > > > not specified on the command line or in a toolchain's arguments, > > > > the default values given in the declare_args block will be used." > > > > > > > > IIUC, the default value of use_webview_internal_framework should be > defined > > > > here, then, it could be used every where, did I miss something? > > > > > > > > > > > > > > > > > > > > > > Looks like the problem doesn't happen anymore because > > > 8d2e9c0cb301a83e5e700166e3119ca66bb88b75 was reverted ("Port Monochrome to > > GN"). > > > Maybe this was the reason for the revert? > > > > > > If you revert the revert, I suspect you'll see the gn gen failure. > > > > > > The problem is just that the order is: > > > > > > 1. GN starts parsing //build/android/config.gni > > > 2. GN hits import("//clank/config.gni") > > > 3. //clank/config.gni references use_webview_internal_framework > > > 4. GN hits the declare_args() block which defines > > > use_webview_internal_framework > > > > > > #3 fails because it needs to happen after #4 > > > > It only happened for the new directory of gn args, did it expose GN's issue? > > For me it happens in an existing directory. Could be that it works if you set > the arg explicitly in your GN args? You are right, I did set it in gn args. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
