Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26358012:
Use CategoryFilter::kDefaultCategoryFilterString instead of "*" as the default category (Closed)
|
Created:
7 years, 2 months ago by Xianzhu Modified:
7 years, 2 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, jam, joi+watch-content_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, darin-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUse CategoryFilter::kDefaultCategoryFilterString instead of "*" as the default category
NOTRY=true (passed presubmit and android try bots)
R=skyostil@chromium.org, yfriedman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229195
Patch Set 1 #
Total comments: 2
Patch Set 2 : DEFAULT_CHROME_CATEGORIES #
Total comments: 2
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/26358012/diff/1/build/android/adb_profile_chr... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/26358012/diff/1/build/android/adb_profile_chr... build/android/adb_profile_chrome.py:166: '"*", "cat1*,-cat1a".') Having an emptry string imply default categories is a little problematic since there's then no way to specify that you want no categories at all. Doing that will be more useful after https://codereview.chromium.org/25686006/ when you might only want to trace systrace categories. How about we add a new constant, say, _DEFAULT_CHROME_CATEGORIES, that this option will be set to by default. The _StartTracing then knows to avoid specifying any categories in the intent if that constant is passed in.
https://codereview.chromium.org/26358012/diff/1/build/android/adb_profile_chr... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/26358012/diff/1/build/android/adb_profile_chr... build/android/adb_profile_chrome.py:166: '"*", "cat1*,-cat1a".') On 2013/10/14 11:22:19, Sami wrote: > Having an emptry string imply default categories is a little problematic since > there's then no way to specify that you want no categories at all. Doing that > will be more useful after https://codereview.chromium.org/25686006/ when you > might only want to trace systrace categories. > > How about we add a new constant, say, _DEFAULT_CHROME_CATEGORIES, that this > option will be set to by default. The _StartTracing then knows to avoid > specifying any categories in the intent if that constant is passed in. Do you mean '--categories ""' to opt-out chromium tracing? This seems inconsistent with CategoryFilter which treats empty filter string as the default categories. How about telling user here to pass "-*" for "no categories" (in your patch)?
On 2013/10/14 16:44:39, Xianzhu wrote: > Do you mean '--categories ""' to opt-out chromium tracing? This seems > inconsistent with CategoryFilter which treats empty filter string as the default > categories. How about telling user here to pass "-*" for "no categories" (in > your patch)? Ah, I didn't know that. IMO passing an empty string to mean "no categories" is easier to understand for the user and also more consistent with how systrace categories work. I think we should internally translate that to "-*". WDYT?
On 2013/10/14 16:47:50, Sami wrote: > On 2013/10/14 16:44:39, Xianzhu wrote: > > Do you mean '--categories ""' to opt-out chromium tracing? This seems > > inconsistent with CategoryFilter which treats empty filter string as the > default > > categories. How about telling user here to pass "-*" for "no categories" (in > > your patch)? > > Ah, I didn't know that. IMO passing an empty string to mean "no categories" is > easier to understand for the user and also more consistent with how systrace > categories work. I think we should internally translate that to "-*". WDYT? SGTM. I'll add _DEFAULT_CHROME_CATEGORIES in this CL. It seems that we don't need to translate "" to "-*" because we should not enable chromium tracing at all with "--categores ''", right?
On 2013/10/14 16:53:57, Xianzhu wrote: > It seems that we don't need to translate "" to "-*" because we should not enable > chromium tracing at all with "--categores ''", right? True. Thanks!
Seems better to make this change based on yours. Will upload a new patch after yours landed.
Rebased on Sami's combined tracing change. PTAL.
https://codereview.chromium.org/26358012/diff/15001/build/android/adb_profile... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/26358012/diff/15001/build/android/adb_profile... build/android/adb_profile_chrome.py:24: _DEFAULT_CHROME_CATEGORIES = '_DEFAULT_CHROME_CATEGORIES' Instead of sending this string to Chrome, could we check for it below in ChromeTracingController and not set any categories in the intent if it matches?
https://codereview.chromium.org/26358012/diff/15001/build/android/adb_profile... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/26358012/diff/15001/build/android/adb_profile... build/android/adb_profile_chrome.py:24: _DEFAULT_CHROME_CATEGORIES = '_DEFAULT_CHROME_CATEGORIES' On 2013/10/16 09:43:34, Sami wrote: > Instead of sending this string to Chrome, could we check for it below in > ChromeTracingController and not set any categories in the intent if it matches? I had thought so but later found when --trace-cc or --trace-gpu is used, we need the string to tell chrome not just the cc/gpu disabled-by-default categories are enabled.
On 2013/10/16 16:47:48, Xianzhu wrote: > I had thought so but later found when --trace-cc or --trace-gpu is used, we need > the string to tell chrome not just the cc/gpu disabled-by-default categories are > enabled. Great point, I didn't think of that. lgtm.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26358012/15001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26358012/15001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26358012/15001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26358012/15001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26358012/15001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26358012/15001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Message was sent while issue was closed.
Committed patchset #2 manually as r229195 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
