Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137893002:
Warm up EnumSystemLocalesEx() for Flash Player. (Closed)
|
Created:
6 years, 11 months ago by Ye Liu Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWarm up EnumSystemLocalesEx() for Flash Player.
Because EnumSystemLocalesEx() is blocked by sandbox on Vista and Win8
for its accessing registry.
R=piman@chromium.org, jschuh@google.com, yzshen@google.com
BUG=335438
Patch Set 1 #Patch Set 2 : Use GetModuleHandleW/GetProcAddress to warm up the API as Windows XP doesn't have it. #Patch Set 3 : Fix Lint Errors #
Total comments: 18
Patch Set 4 : Fix issues per commends. Add warm-up for XP. #Patch Set 5 : Fix lint error. #
Total comments: 1
Patch Set 6 : Per comment, combine the warm-ups. #
Total comments: 2
Patch Set 7 : Shrink to normal call. Justin you made the code look damn cool! Thanks! #
Total comments: 3
Patch Set 8 : Fix format issues per yszhen1's comments. #Patch Set 9 : Fix format issues per yszhen1's comments. #
Total comments: 1
Patch Set 10 : Put a trailing '.' at the end of comment. #Patch Set 11 : Replace the email in AUTHOR file. #Patch Set 12 : Rebase the patch for AUTHOR file. #Patch Set 13 : Update AUTHOR file #Messages
Total messages: 41 (0 generated)
I noticed some minor style issues. Also, I wanted to verify that this works for repeated locale calls, rather than just initializing it for a single enumeration. https://codereview.chromium.org/137893002/diff/90001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/137893002/diff/90001/AUTHORS#newcode319 AUTHORS:319: Ye Liu <yeli@adobe.com> Make sure you submit a signed copy of the contributor agreement. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:59: #if defined(OS_WIN) Move this function definition into the "#if defined(OS_WIN)" block above, below the g_target_services declaration. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:62: (LPWSTR lpLocaleString, DWORD dwFlags, LPARAM lParam) { Fix the indenting to comply with the style guide. Open parenth should be on the first line and you can have the args start up there as lon as you don't exceed 80 cols. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:66: typedef BOOL (WINAPI *PfnEnumSystemLocalesEx) Move this typedef down to where you use it. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:303: // GetModuleHandleW doesn't require FreeLibrary() per MSDN. You don't need the comment about GetModuleHandleW. Probably better to just have the comment read "Warm up system locales." https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:306: if (hKernel32Dll) { There's no need need to check this. All Windows processes must have kernel32.dll loaded. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:308: lfpEnumSystemLocalesEx = (PfnEnumSystemLocalesEx) GetProcAddress( assign in the initializer, rather than on a new line. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:309: hKernel32Dll, "EnumSystemLocalesEx"); indent at least four spaces for a continued line. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:311: if (lfpEnumSystemLocalesEx) { No braces needed for single line if.
On 2014/01/15 00:15:45, Justin Schuh wrote: > I noticed some minor style issues. Also, I wanted to verify that this works for > repeated locale calls, rather than just initializing it for a single > enumeration. > > https://codereview.chromium.org/137893002/diff/90001/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/137893002/diff/90001/AUTHORS#newcode319 > AUTHORS:319: Ye Liu <mailto:yeli@adobe.com> > Make sure you submit a signed copy of the contributor agreement. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > File content/ppapi_plugin/ppapi_thread.cc (right): > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:59: #if defined(OS_WIN) > Move this function definition into the "#if defined(OS_WIN)" block above, below > the g_target_services declaration. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:62: (LPWSTR lpLocaleString, DWORD dwFlags, > LPARAM lParam) { > Fix the indenting to comply with the style guide. Open parenth should be on the > first line and you can have the args start up there as lon as you don't exceed > 80 cols. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:66: typedef BOOL (WINAPI > *PfnEnumSystemLocalesEx) > Move this typedef down to where you use it. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:303: // GetModuleHandleW doesn't require > FreeLibrary() per MSDN. > You don't need the comment about GetModuleHandleW. Probably better to just have > the comment read "Warm up system locales." > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:306: if (hKernel32Dll) { > There's no need need to check this. All Windows processes must have kernel32.dll > loaded. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:308: lfpEnumSystemLocalesEx = > (PfnEnumSystemLocalesEx) GetProcAddress( > assign in the initializer, rather than on a new line. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:309: hKernel32Dll, "EnumSystemLocalesEx"); > indent at least four spaces for a continued line. > > https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... > content/ppapi_plugin/ppapi_thread.cc:311: if (lfpEnumSystemLocalesEx) { > No braces needed for single line if. Thanks for your review! All minor issues are fixed. Because no one knows which locale will be used in Flash Player, it's not possible to warm up the specific one before locking. Beside, I also add warming-up for XP, and limit the enumeration to system installed locales only.
Hi I just fixed the issues per comments. Also add a warm-up for XP. Please review it. Thank you! -ye https://codereview.chromium.org/137893002/diff/90001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/137893002/diff/90001/AUTHORS#newcode319 AUTHORS:319: Ye Liu <yeli@adobe.com> On 2014/01/15 00:15:45, Justin Schuh wrote: > Make sure you submit a signed copy of the contributor agreement. Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:59: #if defined(OS_WIN) On 2014/01/15 00:15:45, Justin Schuh wrote: > Move this function definition into the "#if defined(OS_WIN)" block above, below > the g_target_services declaration. Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:62: (LPWSTR lpLocaleString, DWORD dwFlags, LPARAM lParam) { On 2014/01/15 00:15:45, Justin Schuh wrote: > Fix the indenting to comply with the style guide. Open parenth should be on the > first line and you can have the args start up there as lon as you don't exceed > 80 cols. I change it to static BOOL CALLBACK EnumLocalesProcEx(LPWSTR lpLocaleString, DWORD dwFlags, LPARAM lParam) { return true; } Is it ok? https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:66: typedef BOOL (WINAPI *PfnEnumSystemLocalesEx) On 2014/01/15 00:15:45, Justin Schuh wrote: > Move this typedef down to where you use it. Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:303: // GetModuleHandleW doesn't require FreeLibrary() per MSDN. On 2014/01/15 00:15:45, Justin Schuh wrote: > You don't need the comment about GetModuleHandleW. Probably better to just have > the comment read "Warm up system locales." Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:306: if (hKernel32Dll) { On 2014/01/15 00:15:45, Justin Schuh wrote: > There's no need need to check this. All Windows processes must have kernel32.dll > loaded. Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:308: lfpEnumSystemLocalesEx = (PfnEnumSystemLocalesEx) GetProcAddress( On 2014/01/15 00:15:45, Justin Schuh wrote: > assign in the initializer, rather than on a new line. Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:309: hKernel32Dll, "EnumSystemLocalesEx"); On 2014/01/15 00:15:45, Justin Schuh wrote: > indent at least four spaces for a continued line. Done. https://codereview.chromium.org/137893002/diff/90001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:311: if (lfpEnumSystemLocalesEx) { On 2014/01/15 00:15:45, Justin Schuh wrote: > No braces needed for single line if. Done.
https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:317: GetProcAddress(hKernel32Dll, "EnumSystemLocalesW"); EnumSystemLocales is supported in Win2k+, so let the loader handle it rather than making the explicit GetProcAddress call. Also, if this works to warm up the system locales, then why can't you call it on the later versions of Windows rather than explicitly loading and calling EnumSystemLocalesEx? And just to verify, is the bug present on all supported versions of Windows (WinXP-Win8.1)?
On 2014/01/15 21:16:01, Justin Schuh wrote: > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > File content/ppapi_plugin/ppapi_thread.cc (right): > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:317: GetProcAddress(hKernel32Dll, > "EnumSystemLocalesW"); > EnumSystemLocales is supported in Win2k+, so let the loader handle it rather > than making the explicit GetProcAddress call. Also, if this works to warm up the > system locales, then why can't you call it on the later versions of Windows > rather than explicitly loading and calling EnumSystemLocalesEx? And just to > verify, is the bug present on all supported versions of Windows (WinXP-Win8.1)? 1 The bug is found on XP, Vista, Win 8. 2 Without explicit call, it fails on XP with sandbox enabled. 3 Per MSDN, http://msdn.microsoft.com/en-us/library/windows/desktop/dd317828(v=vs.85).aspx "Note For interoperability reasons, the application should prefer the EnumSystemLocalesEx function to EnumSystemLocales because Microsoft is migrating toward the use of locale names instead of locale identifiers for new locales. Any application that will be run only on Windows Vista and later should use EnumSystemLocalesEx." The older version will fail to enumerate some locales on Vista and above. That's the reason we warm-up two instead of using the older one. Thank you!
On 2014/01/16 01:55:45, Ye Liu wrote: > On 2014/01/15 21:16:01, Justin Schuh wrote: > > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > > File content/ppapi_plugin/ppapi_thread.cc (right): > > > > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > > content/ppapi_plugin/ppapi_thread.cc:317: GetProcAddress(hKernel32Dll, > > "EnumSystemLocalesW"); > > EnumSystemLocales is supported in Win2k+, so let the loader handle it rather > > than making the explicit GetProcAddress call. Also, if this works to warm up > the > > system locales, then why can't you call it on the later versions of Windows > > rather than explicitly loading and calling EnumSystemLocalesEx? And just to > > verify, is the bug present on all supported versions of Windows > (WinXP-Win8.1)? > > 1 The bug is found on XP, Vista, Win 8. > 2 Without explicit call, it fails on XP with sandbox enabled. > 3 Per MSDN, > http://msdn.microsoft.com/en-us/library/windows/desktop/dd317828%28v=vs.85%29... > "Note For interoperability reasons, the application should prefer the > EnumSystemLocalesEx function to EnumSystemLocales because Microsoft is migrating > toward the use of locale names instead of locale identifiers for new locales. > Any application that will be run only on Windows Vista and later should use > EnumSystemLocalesEx." > The older version will fail to enumerate some locales on Vista and above. > That's the reason we warm-up two instead of using the older one. > Thank you! I think you misunderstood my question. When you call a function to warm up a subsystem, you're causing the subsystem to initialize and load any resources it needs before lockdown (after which point it won't be able to load anything). Since legacy compatibility is usually implemented on top of the newer APIs, calling the legacy version is usually sufficient for warming up the subsystem. So, calling EnumSystemLocales is likely to still initialize the subsystem and make it load the resources it needs, regardless of whether you actually use EnumSystemLocalesEx later. You just need to test and see if it works that way on Windows versions that provide both EnumSystemLocales and EnumSystemLocalesEx.
On 2014/01/16 04:44:05, Justin Schuh wrote: > On 2014/01/16 01:55:45, Ye Liu wrote: > > On 2014/01/15 21:16:01, Justin Schuh wrote: > > > > > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > > > File content/ppapi_plugin/ppapi_thread.cc (right): > > > > > > > > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > > > content/ppapi_plugin/ppapi_thread.cc:317: GetProcAddress(hKernel32Dll, > > > "EnumSystemLocalesW"); > > > EnumSystemLocales is supported in Win2k+, so let the loader handle it rather > > > than making the explicit GetProcAddress call. Also, if this works to warm up > > the > > > system locales, then why can't you call it on the later versions of Windows > > > rather than explicitly loading and calling EnumSystemLocalesEx? And just to > > > verify, is the bug present on all supported versions of Windows > > (WinXP-Win8.1)? > > > > 1 The bug is found on XP, Vista, Win 8. > > 2 Without explicit call, it fails on XP with sandbox enabled. > > 3 Per MSDN, > > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd317828%2528v=vs.85%... > > "Note For interoperability reasons, the application should prefer the > > EnumSystemLocalesEx function to EnumSystemLocales because Microsoft is > migrating > > toward the use of locale names instead of locale identifiers for new locales. > > Any application that will be run only on Windows Vista and later should use > > EnumSystemLocalesEx." > > The older version will fail to enumerate some locales on Vista and above. > > That's the reason we warm-up two instead of using the older one. > > Thank you! > > I think you misunderstood my question. When you call a function to warm up a > subsystem, you're causing the subsystem to initialize and load any resources it > needs before lockdown (after which point it won't be able to load anything). > Since legacy compatibility is usually implemented on top of the newer APIs, > calling the legacy version is usually sufficient for warming up the subsystem. > > So, calling EnumSystemLocales is likely to still initialize the subsystem and > make it load the resources it needs, regardless of whether you actually use > EnumSystemLocalesEx later. You just need to test and see if it works that way on > Windows versions that provide both EnumSystemLocales and EnumSystemLocalesEx. Oh, sure. I will test it immediately, and let you know soon.
On 2014/01/16 04:56:02, Ye Liu wrote: > On 2014/01/16 04:44:05, Justin Schuh wrote: > > On 2014/01/16 01:55:45, Ye Liu wrote: > > > On 2014/01/15 21:16:01, Justin Schuh wrote: > > > > > > > > > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > > > > File content/ppapi_plugin/ppapi_thread.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/137893002/diff/210001/content/ppapi_plugin/pp... > > > > content/ppapi_plugin/ppapi_thread.cc:317: GetProcAddress(hKernel32Dll, > > > > "EnumSystemLocalesW"); > > > > EnumSystemLocales is supported in Win2k+, so let the loader handle it > rather > > > > than making the explicit GetProcAddress call. Also, if this works to warm > up > > > the > > > > system locales, then why can't you call it on the later versions of > Windows > > > > rather than explicitly loading and calling EnumSystemLocalesEx? And just > to > > > > verify, is the bug present on all supported versions of Windows > > > (WinXP-Win8.1)? > > > > > > 1 The bug is found on XP, Vista, Win 8. > > > 2 Without explicit call, it fails on XP with sandbox enabled. > > > 3 Per MSDN, > > > > > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd317828%252528v=vs.8... > > > "Note For interoperability reasons, the application should prefer the > > > EnumSystemLocalesEx function to EnumSystemLocales because Microsoft is > > migrating > > > toward the use of locale names instead of locale identifiers for new > locales. > > > Any application that will be run only on Windows Vista and later should use > > > EnumSystemLocalesEx." > > > The older version will fail to enumerate some locales on Vista and above. > > > That's the reason we warm-up two instead of using the older one. > > > Thank you! > > > > I think you misunderstood my question. When you call a function to warm up a > > subsystem, you're causing the subsystem to initialize and load any resources > it > > needs before lockdown (after which point it won't be able to load anything). > > Since legacy compatibility is usually implemented on top of the newer APIs, > > calling the legacy version is usually sufficient for warming up the subsystem. > > > > So, calling EnumSystemLocales is likely to still initialize the subsystem and > > make it load the resources it needs, regardless of whether you actually use > > EnumSystemLocalesEx later. You just need to test and see if it works that way > on > > Windows versions that provide both EnumSystemLocales and EnumSystemLocalesEx. > > Oh, sure. I will test it immediately, and let you know soon. Test passed. New patch uploaded. Thank you.
https://codereview.chromium.org/137893002/diff/290001/content/ppapi_plugin/pp... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/137893002/diff/290001/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:55: // Used by EnumSystemLocales for warming up Add a blank line above this comment. https://codereview.chromium.org/137893002/diff/290001/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:305: // Warm up system locales. You don't need to manually load EnumSystemLocales. It's present on all supported Windows versions, so the loader will handle it for you. You just need to call it directly.
On 2014/01/16 07:54:34, Justin Schuh wrote: > https://codereview.chromium.org/137893002/diff/290001/content/ppapi_plugin/pp... > File content/ppapi_plugin/ppapi_thread.cc (right): > > https://codereview.chromium.org/137893002/diff/290001/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:55: // Used by EnumSystemLocales for > warming up > Add a blank line above this comment. > > https://codereview.chromium.org/137893002/diff/290001/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:305: // Warm up system locales. > You don't need to manually load EnumSystemLocales. It's present on all supported > Windows versions, so the loader will handle it for you. You just need to call it > directly. Thanks! You made the code look damn cool!
https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:55: // Used by EnumSystemLocales for warming up nit: - please add a blank line above this comment. - have a trailing '.' at the end of the comment. https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:57: return true; For consistency, it is better to use TRUE since the return type is BOOL. https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:294: LoadLibrary(L"dxva2.dll"); Wrong indent. it should be two spaces less. (The previous format was correct.)
Also, please create a bug at crbug.com and add the number to the BUG= line in the CL description.
On 2014/01/16 17:26:28, yzshen1 wrote: > https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... > File content/ppapi_plugin/ppapi_thread.cc (right): > > https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:55: // Used by EnumSystemLocales for > warming up > nit: > - please add a blank line above this comment. > - have a trailing '.' at the end of the comment. > > https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:57: return true; > For consistency, it is better to use TRUE since the return type is BOOL. > > https://codereview.chromium.org/137893002/diff/200002/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:294: LoadLibrary(L"dxva2.dll"); > Wrong indent. it should be two spaces less. (The previous format was correct.) yzshen, done. Thank you!
On 2014/01/16 18:06:16, Justin Schuh wrote: > Also, please create a bug at http://crbug.com and add the number to the BUG= line in > the CL description. Issue 335438 was filed at https://code.google.com/p/chromium/issues/detail?id=335438&thanks=335438&ts=1... Thank you.
lgtm from me. once one of the owners signs off we can click the commit queue.
LGTM Please fix the following nit. Thanks! https://codereview.chromium.org/137893002/diff/410001/content/ppapi_plugin/pp... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/137893002/diff/410001/content/ppapi_plugin/pp... content/ppapi_plugin/ppapi_thread.cc:56: // Used by EnumSystemLocales for warming up Please put a trailing '.' at the end of the comment, making it a sentence.
On 2014/01/17 17:36:10, yzshen1 wrote: > LGTM > > Please fix the following nit. Thanks! > > https://codereview.chromium.org/137893002/diff/410001/content/ppapi_plugin/pp... > File content/ppapi_plugin/ppapi_thread.cc (right): > > https://codereview.chromium.org/137893002/diff/410001/content/ppapi_plugin/pp... > content/ppapi_plugin/ppapi_thread.cc:56: // Used by EnumSystemLocales for > warming up > Please put a trailing '.' at the end of the comment, making it a sentence. Sure! Thank you!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbakgly@gmail.com/137893002/480001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
@piman - This still needs you to sign off as an owner.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbakgly@gmail.com/137893002/480001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbakgly@gmail.com/137893002/480001
Failed to apply patch for AUTHORS: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 316. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS diff --git a/AUTHORS b/AUTHORS index 1b9c2e0571296c766bd278affd364948054e736b..0698d6eb332e94f9694cc4196c2dc8ababfa33dd 100644 --- a/AUTHORS +++ b/AUTHORS @@ -316,6 +316,7 @@ Yael Aharon <yael.aharon@intel.com> Yair Yogev <progame@chromium.org> Yang Gu <yang.gu@intel.com> Yarin Kaul <yarin.kaul@gmail.com> +Ye Liu <yeli@adobe.com> Yoav Weiss <yoav@yoav.ws> Yoav Zilberberg <yoav.zilberberg@gmail.com> Yong Shin <sy3620@gmail.com>
On 2014/01/24 22:51:41, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Hello, I feel the CQ fails. What can I do for it?
On 2014/01/28 02:48:01, Ye Liu wrote: > On 2014/01/24 22:51:41, I haz the power (commit-bot) wrote: > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > Hello, I feel the CQ fails. What can I do for it? The author of this patch (cbakgly@gmail.com) doesn't match the one that you added to the AUTHORS file (yeli@adobe.com). You probably need to create a new issue, using the correct account. (I assume yeli@adobe.com is preferable.)
On 2014/01/29 17:17:39, yzshen1 wrote: > On 2014/01/28 02:48:01, Ye Liu wrote: > > On 2014/01/24 22:51:41, I haz the power (commit-bot) wrote: > > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > > > Hello, I feel the CQ fails. What can I do for it? > > The author of this patch (mailto:cbakgly@gmail.com) doesn't match the one that you > added to the AUTHORS file (mailto:yeli@adobe.com). > > You probably need to create a new issue, using the correct account. (I assume > mailto:yeli@adobe.com is preferable.) Hi That's quite strange! I only used that mail to log in google when I uploaded the patch! If CQ only accepts the login account, I have no chance to use yeli@adobe.com. Please point me to the solution. Thank you!
On 2014/01/30 03:54:21, Ye Liu wrote: > On 2014/01/29 17:17:39, yzshen1 wrote: > > On 2014/01/28 02:48:01, Ye Liu wrote: > > > On 2014/01/24 22:51:41, I haz the power (commit-bot) wrote: > > > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > > > > > Hello, I feel the CQ fails. What can I do for it? > > > > The author of this patch (mailto:cbakgly@gmail.com) doesn't match the one that > you > > added to the AUTHORS file (mailto:yeli@adobe.com). > > > > You probably need to create a new issue, using the correct account. (I assume > > mailto:yeli@adobe.com is preferable.) > > Hi > That's quite strange! > I only used that mail to log in google when I uploaded the patch! > If CQ only accepts the login account, I have no chance to use mailto:yeli@adobe.com. > Please point me to the solution. > Thank you! (I assume you are using git: https://sites.google.com/a/chromium.org/dev/developers/how-tos/get-the-code) Have you tried git config --global user.email "my@email"
On 2014/01/30 19:22:48, yzshen1 wrote: > On 2014/01/30 03:54:21, Ye Liu wrote: > > On 2014/01/29 17:17:39, yzshen1 wrote: > > > On 2014/01/28 02:48:01, Ye Liu wrote: > > > > On 2014/01/24 22:51:41, I haz the power (commit-bot) wrote: > > > > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > > > > > > > Hello, I feel the CQ fails. What can I do for it? > > > > > > The author of this patch (mailto:cbakgly@gmail.com) doesn't match the one > that > > you > > > added to the AUTHORS file (mailto:yeli@adobe.com). > > > > > > You probably need to create a new issue, using the correct account. (I > assume > > > mailto:yeli@adobe.com is preferable.) > > > > Hi > > That's quite strange! > > I only used that mail to log in google when I uploaded the patch! > > If CQ only accepts the login account, I have no chance to use > mailto:yeli@adobe.com. > > Please point me to the solution. > > Thank you! > > (I assume you are using git: > https://sites.google.com/a/chromium.org/dev/developers/how-tos/get-the-code) > > Have you tried > git config --global user.email "my@email" Yes, I've set the email.
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbakgly@gmail.com/137893002/730001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for AUTHORS: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 316. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS diff --git a/AUTHORS b/AUTHORS index 1b9c2e0571296c766bd278affd364948054e736b..3b179d2ca3d60912a72bd2af8853df733ed9d8c7 100644 --- a/AUTHORS +++ b/AUTHORS @@ -316,6 +316,7 @@ Yael Aharon <yael.aharon@intel.com> Yair Yogev <progame@chromium.org> Yang Gu <yang.gu@intel.com> Yarin Kaul <yarin.kaul@gmail.com> +Ye Liu <cbakgly@gmail.com> Yoav Weiss <yoav@yoav.ws> Yoav Zilberberg <yoav.zilberberg@gmail.com> Yong Shin <sy3620@gmail.com>
You still haven't rebased to the lastest version. You need to sync past the following revision: http://src.chromium.org/viewvc/chrome/trunk/src/AUTHORS?revision=247093 You can notice there is a name got added at the same position as your name in that revision. That is why your patch got a conflict.
The CQ bit was checked by cbakgly@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbakgly@gmail.com/137893002/880001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for AUTHORS: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 14. Hunk #2 FAILED at 53. Hunk #3 FAILED at 91. Hunk #4 FAILED at 131. Hunk #5 FAILED at 174. Hunk #6 FAILED at 185. Hunk #7 FAILED at 234. Hunk #8 FAILED at 243. Hunk #9 FAILED at 278. Hunk #10 FAILED at 316. 10 out of 10 hunks FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS diff --git a/AUTHORS b/AUTHORS index 1b9c2e0571296c766bd278affd364948054e736b..3ac47bd44546056e213eea459cd8747b37053910 100644 --- a/AUTHORS +++ b/AUTHORS @@ -14,6 +14,7 @@ Aaron Randolph <aaron.randolph@gmail.com> Adam Treat <adam.treat@samsung.com> Adenilson Cavalcanti <a.cavalcanti@partner.samsung.com> Aditya Bhargava <heuristicist@gmail.com> +Ajay Berwal <ajay.berwal@samsung.com> Alex Gartrell <agartrell@cmu.edu> Alex Scheele <alexscheele@gmail.com> Alexander Sulfrian <alexander@sulfrian.net> @@ -53,6 +54,7 @@ Catalin Badea <badea@adobe.com> Cem Kocagil <cem.kocagil@gmail.com> Chamal De Silva <chamalsl@yahoo.com> Chandra Shekar Vallala <brk376@motorola.com> +Chang Shu <c.shu@samsung.com> Changbin Shao <changbin.shao@intel.com> Chaobin Zhang <zhchbin@gmail.com> Chris Harrelson <chrishtr@gmail.com> @@ -91,8 +93,10 @@ Evan Wallace <evan.exe@gmail.com> Evangelos Foutras <evangelos@foutrelis.com> Fabien Tassin <fta@sofaraway.org> Felix H. Dahlke <fhd@ubercode.de> +Fernando Jiménez Moreno <ferjmoreno@gmail.com> François Beaufort <beaufort.francois@gmail.com> Francois Kritzinger <francoisk777@gmail.com> +Frédéric Wang <fred.wang@free.fr> Gaetano Mendola <mendola@gmail.com> Gajendra Singh <wxjg68@motorola.com> Gao Chun <gaochun.dev@gmail.com> @@ -131,6 +135,7 @@ Jesse Miller <jesse@jmiller.biz> Jesus Sanchez-Palencia <jesus.sanchez-palencia.fernandez.fil@intel.com> Jin Yang <jin.a.yang@intel.com> Jingwei Liu <kingweiliu@gmail.com> +Jinho Bang <jinho.bang@samsung.com> Jinwoo Song <jinwoo7.song@samsung.com> Joe Knoll <joe.knoll@workday.com> Joe Thomas <mhx348@motorola.com> @@ -174,6 +179,7 @@ Luke Inman-Semerau <luke.semerau@gmail.com> Luke Zarko <lukezarko@gmail.com> Maarten Lankhorst <m.b.lankhorst@gmail.com> Magnus Danielsson <fuzzac@gmail.com> +Mahesh Kulkarni <mahesh.kk@samsung.com> Mao Yujie <maojie0924@gmail.com> Mao Yujie <yujie.mao@intel.com> Marco Rodrigues <gothicx@gmail.com> @@ -185,6 +191,7 @@ Matheus Bratfisch <matheusbrat@gmail.com> Mathias Bynens <mathias@qiwi.be> Matt Arpidone <mma.public@gmail.com> Matthew Robertson <matthewrobertson03@gmail.com> +Matthew Turk <matthewturk@gmail.com> Matthew Willis <appamatto@gmail.com> Matthias Reitinger <reimarvin@gmail.com> Max Perepelitsyn <pph34r@gmail.com> @@ -234,6 +241,7 @@ Pierre-Antoine LaFayette <pierre.lafayette@gmail.com> Po-Chun Chang <pochang0403@gmail.com> Prashant Nevase <prashant.n@samsung.com> Qiankun Miao <qiankun.miao@intel.com> +Qing Zhang <qing.zhang@intel.com> Radu Stavila <stavila@adobe.com> Raman Tenneti <raman.tenneti@gmail.com> Ramkumar Gokarnesan <ramkumar.gokarnesan@gmail.com> @@ -243,11 +251,13 @@ Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Ravi Phaneendra Kasibhatla <r.kasibhatla@samsung.com> Ravi Phaneendra Kasibhatla <ravi.kasibhatla@motorola.com> Rene Bolldorf <rb@radix.io> +Rene Ladan <r.c.ladan@gmail.com> Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Rob Buis <rob.buis@samsung.com> Robert Bear Travis <bear.travis@gmail.com> Robert Bear Travis <betravis@adobe.com> Robert Goldberg <goldberg@adobe.com> +Robert Hogan <robhogan@gmail.com> Robert Nagy <robert.nagy@gmail.com> Robert Sesek <rsesek@bluestatic.org> Rosen Dash <nqk836@motorola.com> @@ -278,6 +288,8 @@ Shiliu Wang <aofdwsl@gmail.com> Shouqun Liu <shouqun.liu@intel.com> Shreyas VA <v.a.shreyas@gmail.com> Simon Arlott <simon.arlott@gmail.com> +Siva Kumar Gunturi <siva.gunturi@samsung.com> +Sohan Jyoti Ghosh <sohan.jyoti@samsung.com> Song YeWen <ffmpeg@gmail.com> Soren Dreijer <dreijerbit@gmail.com> Stephen Searles <stephen.searles@gmail.com> @@ -316,6 +328,8 @@ Yael Aharon <yael.aharon@intel.com> Yair Yogev <progame@chromium.org> Yang Gu <yang.gu@intel.com> Yarin Kaul <yarin.kaul@gmail.com> +Ye Liu <cbakgly@gmail.com> +Yi Shen <yi.shen@samsung.com> Yoav Weiss <yoav@yoav.ws> Yoav Zilberberg <yoav.zilberberg@gmail.com> Yong Shin <sy3620@gmail.com>
I have landed the change in https://codereview.chromium.org/161263002/ We can close this issue now. |
