Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
282813002:
Show logs in english in TracingController for adb_chrome_profiler (Closed)
|
Created:
6 years, 7 months ago by JungJik Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, dmikurube+memory_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionShow logs in english in TracingController for adb_chrome_profiler
if the target is not in english, adb_profiler_chrome can
not know when the profiler is started and finished.
because adb_profiler_chrome uses only english regex for
parsing the logs. So this is a patch for showing the logs
in english, however it still shows toasts by following
the target's locale.
BUG=373505
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273035
Patch Set 1 : please ignore this #Patch Set 2 #Patch Set 3 : please ignore this #Patch Set 4 : #Patch Set 5 : fix in TracingControllerAndroid #
Total comments: 2
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #
Total comments: 1
Patch Set 11 : #Messages
Total messages: 34 (0 generated)
Please PTAL :)
thanks Jung Jik! :) hmm... what part of the script is relying on the device language? I think this warning is fine, but ideally, this script should work regardless of the locale ;) sami, wdyt?
On 2014/05/14 10:30:31, bulach wrote: > thanks Jung Jik! :) > hmm... what part of the script is relying on the device language? I think this > warning is fine, but ideally, this script should work regardless of the locale > ;) > sami, wdyt? Ah, I think it's the regex looking for the toast text, e.g., "Logging performance trace to ...". That text is localized, so this will break on non-english locales. Could we instead change Chrome to always log the English message so this script wouldn't have to care about the locale? We can keep the toast text as is. Note that we can't easily change the text since this script needs to remain compatible with the current stable build.
On 2014/05/14 12:14:34, Sami wrote: > On 2014/05/14 10:30:31, bulach wrote: > > thanks Jung Jik! :) > > hmm... what part of the script is relying on the device language? I think this > > warning is fine, but ideally, this script should work regardless of the locale > > ;) > > sami, wdyt? > > Ah, I think it's the regex looking for the toast text, e.g., "Logging > performance trace to ...". That text is localized, so this will break on > non-english locales. > > Could we instead change Chrome to always log the English message so this script > wouldn't have to care about the locale? We can keep the toast text as is. Note > that we can't easily change the text since this script needs to remain > compatible with the current stable build. hi, so could I change all toast texts to English? or change this patch to support other languages by using regex dict?
On 2014/05/14 15:51:40, JungJik wrote: > hi, so could I change all toast texts to English? or change this patch to > support > other languages by using regex dict? How about a third option where we change TracingControllerAndroid.logAndToastInfo (and logAndToastError) to display the translated text in the toast but log the untranslated text? That way there's no need to change adb_profile_chrome.py and the UI still remains translated. I *think* you can get the original untranslated text through a ResourceBundle.
On 2014/05/15 13:54:17, Sami wrote: > On 2014/05/14 15:51:40, JungJik wrote: > > hi, so could I change all toast texts to English? or change this patch to > > support > > other languages by using regex dict? > > How about a third option where we change > TracingControllerAndroid.logAndToastInfo (and logAndToastError) to display the > translated text in the toast but log the untranslated text? That way there's no > need to change adb_profile_chrome.py and the UI still remains translated. I > *think* you can get the original untranslated text through a ResourceBundle. That's a good option. thank you and I'll file new patch.
On 2014/05/16 04:03:52, JungJik wrote: > On 2014/05/15 13:54:17, Sami wrote: > > On 2014/05/14 15:51:40, JungJik wrote: > > > hi, so could I change all toast texts to English? or change this patch to > > > support > > > other languages by using regex dict? > > > > How about a third option where we change > > TracingControllerAndroid.logAndToastInfo (and logAndToastError) to display the > > translated text in the toast but log the untranslated text? That way there's > no > > need to change adb_profile_chrome.py and the UI still remains translated. I > > *think* you can get the original untranslated text through a ResourceBundle. > > That's a good option. thank you and I'll file new patch. I've tried to use ResourceBundle to get untranslated text, but it seems that we couldn't use properties for resource. I found the way to get "en" text from context.Resources. Please take a look :)
Thanks for the update. I think this otherwise looks good to go, but I have one comment about locale switching. https://codereview.chromium.org/282813002/diff/70001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/70001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:131: mContext.getResources().updateConfiguration(configuration, displayMetrics); Changing the view configuration like this is probably too heavy weight and could cause unintended side-effects. Could you use Context.createConfigurationContext() instead? That's only available from Jellybean onwards, but I think we can just fall back to the current behavior on older versions.
https://codereview.chromium.org/282813002/diff/70001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/70001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:131: mContext.getResources().updateConfiguration(configuration, displayMetrics); On 2014/05/19 15:51:20, Sami wrote: > Changing the view configuration like this is probably too heavy weight and could > cause unintended side-effects. Could you use > Context.createConfigurationContext() instead? That's only available from > Jellybean onwards, but I think we can just fall back to the current behavior on > older versions. thanks for your help. I also worried about the heavy weight of changing view configuration, but I couldn't find the proper way. I will file new patch again with using Context.createConfigurationContext.
On 2014/05/19 16:33:50, JungJik wrote: > https://codereview.chromium.org/282813002/diff/70001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java > (right): > > https://codereview.chromium.org/282813002/diff/70001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:131: > mContext.getResources().updateConfiguration(configuration, displayMetrics); > On 2014/05/19 15:51:20, Sami wrote: > > Changing the view configuration like this is probably too heavy weight and > could > > cause unintended side-effects. Could you use > > Context.createConfigurationContext() instead? That's only available from > > Jellybean onwards, but I think we can just fall back to the current behavior > on > > older versions. > > thanks for your help. I also worried about the heavy weight of changing view > configuration, but I couldn't find the proper way. I will file new patch again > with using Context.createConfigurationContext. Could you review this patch again?
https://codereview.chromium.org/282813002/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:127: Configuration configuration = mContext.getResources().getConfiguration(); Instead of modifying the current configuration I think we should make a copy like this: Configuration configuration = new Configuration(mContext.getResources().getConfiguration()); https://codereview.chromium.org/282813002/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:131: configuration.locale = new Locale(language); Can we move this after line 127 and avoid the call to updateConfiguration completely?
On 2014/05/22 10:37:04, Sami wrote: > https://codereview.chromium.org/282813002/diff/90001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java > (right): > > https://codereview.chromium.org/282813002/diff/90001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:127: > Configuration configuration = mContext.getResources().getConfiguration(); > Instead of modifying the current configuration I think we should make a copy > like this: > > Configuration configuration = new > Configuration(mContext.getResources().getConfiguration()); > > https://codereview.chromium.org/282813002/diff/90001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:131: > configuration.locale = new Locale(language); > Can we move this after line 127 and avoid the call to updateConfiguration > completely? I fixed the patch. thanks for the review.
Thank you! lgtm with one nit. https://codereview.chromium.org/282813002/diff/110001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/110001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:132: else { Nit: should be on the previous line with the closing brace.
On 2014/05/22 15:09:51, Sami wrote: > Thank you! lgtm with one nit. > > https://codereview.chromium.org/282813002/diff/110001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java > (right): > > https://codereview.chromium.org/282813002/diff/110001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:132: > else { > Nit: should be on the previous line with the closing brace. I fixed the patch. I appreciate your review.
Thank you. Still lgtm.
The CQ bit was checked by jungjik.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/282813002/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
On 2014/05/22 21:10:08, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) it's failed during chromium_presumit due to omitting OWNER's lgtm. so I add owners for review. please take a look. thanks.
https://codereview.chromium.org/282813002/diff/130001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/130001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:129: Context context = mContext.createConfigurationContext(configuration); Any idea how long these calls can take? I have a feeling it's doing a lot of work in the background to initialize these resources. Personally, I would just vote that we hard code these strings as constants in this file and not worry about this. As these strings need to match other hardcoded strings in the profiler, it seems risky in general to read them from resources as people might not understand the implications of changing them in one place breaking another.
On 2014/05/23 20:12:54, Ted C wrote: > https://codereview.chromium.org/282813002/diff/130001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java > (right): > > https://codereview.chromium.org/282813002/diff/130001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:129: > Context context = mContext.createConfigurationContext(configuration); > Any idea how long these calls can take? I have a feeling it's doing a lot of > work in the background to initialize these resources. > > Personally, I would just vote that we hard code these strings as constants in > this file and not worry about this. As these strings need to match other > hardcoded strings in the profiler, it seems risky in general to read them from > resources as people might not understand the implications of changing them in > one place breaking another. Thanks for comment. I've check createConfigurationContext function (http://goo.gl/cJFtzi). It looks just creating context object and returning the object it seems the function does not include loading resources work. resources are already loaded so I think it does not work heavily. However if you think it is good to use constant string for this, I could change this patch with constant string. it's more simple. PTAL again. this patch makes me not to change the locale for profiling. :)
thanks JungJik! I think I'm with ted here.. :) we could just have a tiny function, "logForProfiler(String msg)", and two hardcoded strings here, with a nice comment saying "it needs to match the ones that adb_chrome_profiler look for".. it'd keep this code nice, simple and safe, wdyt?
On 2014/05/27 11:07:19, bulach wrote: > thanks JungJik! > I think I'm with ted here.. :) > we could just have a tiny function, "logForProfiler(String msg)", and two > hardcoded strings here, with a nice comment saying "it needs to match the ones > that adb_chrome_profiler look for".. > > it'd keep this code nice, simple and safe, wdyt? thanks bulach for the comment. I fixed it and it became much simpler. Please take a look again.
lgtm % small suggestions below, but please ensure the other folks are happy as well :) thanks, it looks indeed much simpler! https://codereview.chromium.org/282813002/diff/150001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/150001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:194: logForProfiler("Profiler started: " + categories); nit: how about put up above: // These strings must match the ones expected by adb_profile_chrome private final String PROFILER_STARTED_FMT = "Profiler started: %s"; private final String PROFILER_FINISHED_FMT = "Profiler finished. Results are in %s."; then use String.format(PROFILER_STARTED_FMT, categories) here, and String.format(PROFILER_FINISHED, mFileName) below? https://codereview.chromium.org/282813002/diff/150001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:252: void logForProfiler(String str) { nit: I think this two functions can be made private..
On 2014/05/27 15:04:52, bulach wrote: > lgtm % small suggestions below, but please ensure the other folks are happy as > well :) thanks, it looks indeed much simpler! > > https://codereview.chromium.org/282813002/diff/150001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java > (right): > > https://codereview.chromium.org/282813002/diff/150001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:194: > logForProfiler("Profiler started: " + categories); > nit: how about put up above: > > // These strings must match the ones expected by adb_profile_chrome > private final String PROFILER_STARTED_FMT = "Profiler started: %s"; > private final String PROFILER_FINISHED_FMT = "Profiler finished. Results are in > %s."; > > then use String.format(PROFILER_STARTED_FMT, categories) here, and > String.format(PROFILER_FINISHED, mFileName) below? > > https://codereview.chromium.org/282813002/diff/150001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:252: > void logForProfiler(String str) { > nit: I think this two functions can be made private.. thanks bulach for lgtm and suggestions, now I don't need to change the locale for profiler! :)
lgtm, happy with this way too. https://codereview.chromium.org/282813002/diff/170001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java (right): https://codereview.chromium.org/282813002/diff/170001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:256: // it needs to match the ones that adb_chrome_profiler looks for. Nit: Replace "it" with "The |str| string".
On 2014/05/27 16:01:42, Sami wrote: > lgtm, happy with this way too. > > https://codereview.chromium.org/282813002/diff/170001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java > (right): > > https://codereview.chromium.org/282813002/diff/170001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:256: > // it needs to match the ones that adb_chrome_profiler looks for. > Nit: Replace "it" with "The |str| string". I fixed it and thanks for the notation. now I learned it. :)
lgtm
The CQ bit was checked by jungjik.lee@samsung.com
On 2014/05/27 17:09:50, Ted C wrote: > lgtm Thanks for lgtm :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/282813002/180001
Message was sent while issue was closed.
Change committed as 273035 |
