Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
879853002:
Add a --startup option to generate combined traces for startup. (Closed)
DescriptionAdd profile_chrome_startup.py to generate combined traces for startup.
This is done following a suggestion in
https://codereview.chromium.org/686413002/.
BUG=361548
Committed: https://crrev.com/477e91f0a79fef8e766d07f228e67b645509e0c4
Cr-Commit-Position: refs/heads/master@{#313499}
Patch Set 1 #Patch Set 2 : Make the tracing work with chrome_shell. #
Total comments: 16
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Make startup tracing into a separate tool. #
Total comments: 21
Patch Set 5 : Address comments and add a test. #Patch Set 6 : Typo. #
Total comments: 9
Patch Set 7 : Address comments. #
Messages
Total messages: 27 (4 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
Hello, This is a small patch to have combined startup traces properly integrated into the rest of the tracing scripts.
pasko@google.com changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:82: data='http://www.google.com/', so we are starting chrome right after we drop caches? why? will it only be able to trace the startup of only this page? https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... File tools/profile_chrome/main.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:78: 'are "warm" and "cold". Cold start is done with the page ' nit: maybe rephrase: The "warm" does not perform special steps, while the "cold" flushes the OS page cache before start. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:236: # Systrace need to be put first, because when we profile startup, we want to nit: we usually try to avoid 'we' in the comments. nit: the comment would be more readable if you explain the final state rather than the change you are making. Like: # Enable the systrace and chrome controller. The systrace controller should go # first because it provides early events used in the chrome controller.
Thanks for the patch. I'm wondering if we should separate this out a little more from the main tool, because startup tracing is a pretty special case and it doesn't work with all the other options that the tool supports. For example, any categories that you enable are silently ignored. How about doing what Egor suggested in the linked code review and adding a new executable script next to profile_chrome.py that only supports startup tracing and implements it by importing the necessary bits from the other modules. WDYT? https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:23: startup=False): Could you call this something like startup_tracing_mode and make it take either None or one of the predefined strings. I think mixing bools and strings looks weird. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:115: self._is_tracing = True This should still be set depending on whether we saw the log match or not. Otherwise StopTracing will end up sitting around for 120 seconds of the user told us to use a wrong browser. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... File tools/profile_chrome/main.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:188: if options.startup and options.startup not in ['cold', 'warm']: Could you use optparser's "choices" mechanism to let it validate this automatically? https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:266: options.time if options.time else 0, Why this change?
Thank you both for the reviews. I can separate it into another executable. However, I feel that would lead to more duplication, or slightly more code. I see two options: 1. Add a comment in the --startup flag explaining that this is incompatible with some options (but not all, the systrace categories are still useful here) 2. Separate it into a different binary. I don't have a strong opinion on this, so let you decide. I've addressed most of your other comments in the CL. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:23: startup=False): On 2015/01/27 13:15:47, Sami wrote: > Could you call this something like startup_tracing_mode and make it take either > None or one of the predefined strings. I think mixing bools and strings looks > weird. Oops, forgot to update this. You are right, sorry. Done. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:23: startup=False): On 2015/01/27 13:15:47, Sami wrote: > Could you call this something like startup_tracing_mode and make it take either > None or one of the predefined strings. I think mixing bools and strings looks > weird. Oh, I missed this one. Sorry about that. Done. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:115: self._is_tracing = True On 2015/01/27 13:15:46, Sami wrote: > This should still be set depending on whether we saw the log match or not. > Otherwise StopTracing will end up sitting around for 120 seconds of the user > told us to use a wrong browser. If I understand the code correctly, when we don't have a match in 5s, an exception is raised, and _is_tracing will not be set. For startup tracing, there is no startup marker. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... File tools/profile_chrome/main.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:78: 'are "warm" and "cold". Cold start is done with the page ' On 2015/01/27 12:59:00, pasko wrote: > nit: maybe rephrase: > The "warm" does not perform special steps, while the "cold" flushes the OS page > cache before start. Done. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:188: if options.startup and options.startup not in ['cold', 'warm']: On 2015/01/27 13:15:47, Sami wrote: > Could you use optparser's "choices" mechanism to let it validate this > automatically? Didn't know about this mechanism. Thank you ! Done. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:236: # Systrace need to be put first, because when we profile startup, we want to On 2015/01/27 12:59:00, pasko wrote: > nit: we usually try to avoid 'we' in the comments. > nit: the comment would be more readable if you explain the final state rather > than the change you are making. Like: > # Enable the systrace and chrome controller. The systrace controller should go > # first because it provides early events used in the chrome controller. Done. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/mai... tools/profile_chrome/main.py:266: options.time if options.time else 0, On 2015/01/27 13:15:47, Sami wrote: > Why this change? Because options.time is None when options.continuous or options.startup is specified.
Thanks. On 2015/01/27 13:50:26, lizeb wrote: > Thank you both for the reviews. > > I can separate it into another executable. However, I feel that would lead to > more duplication, or slightly more code. > I see two options: > > 1. Add a comment in the --startup flag explaining that this is incompatible with > some options (but not all, the systrace categories are still useful here) > 2. Separate it into a different binary. > > I don't have a strong opinion on this, so let you decide. I've addressed most of > your other comments in the CL. I vote having a separate binary, because the use cases are pretty different and the list of incompatible modes will quickly get out of date. A dedicated binary will also make it easier to add more startup-related options. For example, maybe you want to measure startup with something other than google.com. If you're worried about the duplication, feel free to refactor main.py to make it more reuseable if you want. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:115: self._is_tracing = True On 2015/01/27 13:50:25, lizeb wrote: > On 2015/01/27 13:15:46, Sami wrote: > > This should still be set depending on whether we saw the log match or not. > > Otherwise StopTracing will end up sitting around for 120 seconds of the user > > told us to use a wrong browser. > > If I understand the code correctly, when we don't have a match in 5s, an > exception is raised, and _is_tracing will not be set. > For startup tracing, there is no startup marker. Ah, sorry, you're right.
I am in slight preference of adb_profile_chrome --startup if all flag incompatibilities are checked in the code and produce correct error messages (and somewhat explained in the --help output). However, I'd also like to leave the decision for Sami to make :)
On 2015/01/27 14:25:32, pasko wrote: > I am in slight preference of adb_profile_chrome --startup if all flag > incompatibilities are checked in the code and produce correct error messages > (and somewhat explained in the --help output). > > However, I'd also like to leave the decision for Sami to make :) One more factor here is that we want to enable tracing all Android apps at the NDK level. This means startup tracing will eventually amount to turning on tracing in the system and launching chrome -- no need to twiddle with command line files etc. At that point it won't make much sense for adb_profile_chrome to know about startup tracing since the two are mostly orthogonal. That would suggest we should have a separate tool for this. What do you guys think?
On 2015/01/27 15:07:09, Sami wrote: > On 2015/01/27 14:25:32, pasko wrote: > > I am in slight preference of adb_profile_chrome --startup if all flag > > incompatibilities are checked in the code and produce correct error messages > > (and somewhat explained in the --help output). > > > > However, I'd also like to leave the decision for Sami to make :) > > One more factor here is that we want to enable tracing all Android apps at the > NDK level. This means startup tracing will eventually amount to turning on > tracing in the system and launching chrome -- no need to twiddle with command > line files etc. At that point it won't make much sense for adb_profile_chrome to > know about startup tracing since the two are mostly orthogonal. That would > suggest we should have a separate tool for this. What do you guys think? Yes, global telemetry state of tracing is the plan. So what I think what it means for adb_profile_chrome is: it just enables/disables tracing. (A 'disable' makes the trace available). But then we would launch chrome manually (or from another script) I guess.
On 2015/01/27 15:19:33, pasko-google - do not use wrote: > > One more factor here is that we want to enable tracing all Android apps at the > > NDK level. This means startup tracing will eventually amount to turning on > > tracing in the system and launching chrome -- no need to twiddle with command > > line files etc. At that point it won't make much sense for adb_profile_chrome > to > > know about startup tracing since the two are mostly orthogonal. That would > > suggest we should have a separate tool for this. What do you guys think? > > Yes, global telemetry state of tracing is the plan. So what I think what it > means for adb_profile_chrome is: it just enables/disables tracing. (A 'disable' > makes the trace available). But then we would launch chrome manually (or from > another script) I guess. So, right, a separate script for _startup would be somewhat closer to that world of global tracing state.
pasko@google.com changed reviewers: + pasko@google.com
https://codereview.chromium.org/879853002/diff/40001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/40001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:78: adb.RunShellCommand('echo 1 > /proc/vm/drop_caches') There is cache_control.DropRamCaches, which also drops inodes and directory entries, maybe that is more representative of a cold start?
Hello, As agreed, I have separated the tool into a separate adb_profile_chrome_startup.py. Also, it is now possible to choose the initial web page for startup, as per pasko's comment. https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/20001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:82: data='http://www.google.com/', On 2015/01/27 12:59:00, pasko wrote: > so we are starting chrome right after we drop caches? why? will it only be able > to trace the startup of only this page? Acknowledged. https://codereview.chromium.org/879853002/diff/40001/tools/profile_chrome/chr... File tools/profile_chrome/chrome_controller.py (right): https://codereview.chromium.org/879853002/diff/40001/tools/profile_chrome/chr... tools/profile_chrome/chrome_controller.py:78: adb.RunShellCommand('echo 1 > /proc/vm/drop_caches') On 2015/01/27 15:20:52, pasko-google - do not use wrote: > There is cache_control.DropRamCaches, which also drops inodes and directory > entries, maybe that is more representative of a cold start? Done.
https://codereview.chromium.org/879853002/diff/60001/build/android/adb_profil... File build/android/adb_profile_chrome_startup (right): https://codereview.chromium.org/879853002/diff/60001/build/android/adb_profil... build/android/adb_profile_chrome_startup:8: exec $(dirname $0)/../../tools/profile_chrome_startup.py $@ to properly handle spaces in directory names and arguments: exec "$(dirname "$0")/../../tools/profile_chrome_startup.py" "$@" https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome/mai... File tools/profile_chrome/main.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome/mai... tools/profile_chrome/main.py:220: if systrace_categories: wouldn't it still be useful to put systrace earlier for non-startup cases? https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:38: def __repr__(self): is this necessary? https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:81: 'steps, while the "cols" flushes the OS page cache before ' s/cols/cold/ this option seems to be ignored and replaced with --cold? https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:94: parser.add_option('-b', '--browser', help='Select among installed browsers. ' maybe these options could also be factored out?
I mean, overall looks good, of course
Thanks, I think this looks much better this way. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:28: class ChromeStartupTracingController(controllers.BaseController): Please add a test for this like we do with the other controllers. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:38: def __repr__(self): On 2015/01/27 17:00:24, pasko wrote: > is this necessary? CaptureProfile uses it to print out the type of trace we are recording. Please use single quotes though. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:59: def StopTracing(self): Can you make sure to remove the --trace-startup flag from the command line file? It'll be pretty surprising for the user if it persists.
All done. I've added a test (so code has moved again), and it passes (as well as the other ones). Thank you. https://codereview.chromium.org/879853002/diff/60001/build/android/adb_profil... File build/android/adb_profile_chrome_startup (right): https://codereview.chromium.org/879853002/diff/60001/build/android/adb_profil... build/android/adb_profile_chrome_startup:8: exec $(dirname $0)/../../tools/profile_chrome_startup.py $@ On 2015/01/27 17:00:24, pasko wrote: > to properly handle spaces in directory names and arguments: > exec "$(dirname "$0")/../../tools/profile_chrome_startup.py" "$@" Done. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome/mai... File tools/profile_chrome/main.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome/mai... tools/profile_chrome/main.py:220: if systrace_categories: On 2015/01/27 17:00:24, pasko wrote: > wouldn't it still be useful to put systrace earlier for non-startup cases? Maybe, but this would be another CL. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:28: class ChromeStartupTracingController(controllers.BaseController): On 2015/01/27 17:08:33, Sami wrote: > Please add a test for this like we do with the other controllers. Done. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:38: def __repr__(self): On 2015/01/27 17:00:24, pasko wrote: > is this necessary? No, but prettier. In profiler.py, this gets printed: trace_type = ' + '.join(map(str, controllers)) So having a nice representation helps (and this is what is done by the other controllers). https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:38: def __repr__(self): On 2015/01/27 17:08:33, Sami wrote: > On 2015/01/27 17:00:24, pasko wrote: > > is this necessary? > > CaptureProfile uses it to print out the type of trace we are recording. Please > use single quotes though. Done. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:59: def StopTracing(self): On 2015/01/27 17:08:32, Sami wrote: > Can you make sure to remove the --trace-startup flag from the command line file? > It'll be pretty surprising for the user if it persists. Indeed, thank you for pointing this out. Done. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:81: 'steps, while the "cols" flushes the OS page cache before ' On 2015/01/27 17:00:24, pasko wrote: > s/cols/cold/ > > this option seems to be ignored and replaced with --cold? You're right, oops. Done. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:94: parser.add_option('-b', '--browser', help='Select among installed browsers. ' On 2015/01/27 17:00:24, pasko wrote: > maybe these options could also be factored out? It seemed to be more trouble than necessary, but I can change it.
Thanks, lgtm with a handful of nits. https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/ch... File tools/profile_chrome/chrome_startup_controller_unittest.py (right): https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/ch... tools/profile_chrome/chrome_startup_controller_unittest.py:14: categories = '*' These two seem to be unused? https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/ch... tools/profile_chrome/chrome_startup_controller_unittest.py:17: self.device, self.package_info, False, "https://www.google.com") nit: single quotes please. https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/fl... File tools/profile_chrome/flags.py (right): https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/fl... tools/profile_chrome/flags.py:12: 'enable both types of categories. Use "list" to see ' s/enable/enabling/ https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome_st... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome_st... tools/profile_chrome_startup.py:34: 'https://wwww.google.com', default='https://www.google.com', s/wwww/www/
lgtm with a few TODOs and a tiny bash fix thanks!! https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome/mai... File tools/profile_chrome/main.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome/mai... tools/profile_chrome/main.py:220: if systrace_categories: On 2015/01/27 18:41:41, lizeb wrote: > On 2015/01/27 17:00:24, pasko wrote: > > wouldn't it still be useful to put systrace earlier for non-startup cases? > > Maybe, but this would be another CL. I assumed it is only about swapping a few lines here, is it more involved? Can please you put a TODO? This would reduce the likelihood of it being forgotten. https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:59: def StopTracing(self): On 2015/01/27 18:41:41, lizeb wrote: > On 2015/01/27 17:08:32, Sami wrote: > > Can you make sure to remove the --trace-startup flag from the command line > file? > > It'll be pretty surprising for the user if it persists. > > Indeed, thank you for pointing this out. > Done. What happens if the tracing is not stopped properly? Say, a device would disconnect, we throw an exception and exit without going through StopTracing. It seems reasonable to require all performance-related scripts to push their own flags file on the start to make the performance results more reproducible. It is OK to perform a 'best effort restore' of course. There is an alternative of always handling exceptions properly in telemetry, but I don't think we are ready to go that path at the moment. So, how about a TODO for the next CL to push the 'canonical' perf flags to the device before running any benchmarks? https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:94: parser.add_option('-b', '--browser', help='Select among installed browsers. ' On 2015/01/27 18:41:41, lizeb wrote: > On 2015/01/27 17:00:24, pasko wrote: > > maybe these options could also be factored out? > > It seemed to be more trouble than necessary, but I can change it. Ack, good enough for now. https://codereview.chromium.org/879853002/diff/100001/build/android/adb_profi... File build/android/adb_profile_chrome_startup (right): https://codereview.chromium.org/879853002/diff/100001/build/android/adb_profi... build/android/adb_profile_chrome_startup:8: exec "$(dirname $0)/../../tools/profile_chrome_startup.py" "$@" suggested quotes around $0 was not a typo, without it having spaces in the path to src/ would be broken.
https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:59: def StopTracing(self): On 2015/01/28 12:26:03, pasko wrote: > So, how about a TODO for the next CL to push the 'canonical' perf flags to the > device before running any benchmarks? adb_profile_chrome isn't just used for running benchmarks, so I don't think it should touch the command line unnecessarily. Before this patch it doesn't event launch the activity. Telemetry seems like the better place for that sort of stuff, and we already do a lot of provisioning there.
https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/60001/tools/profile_chrome_sta... tools/profile_chrome_startup.py:59: def StopTracing(self): On 2015/01/28 12:44:14, Sami (OoO) wrote: > On 2015/01/28 12:26:03, pasko wrote: > > So, how about a TODO for the next CL to push the 'canonical' perf flags to the > > device before running any benchmarks? > > adb_profile_chrome isn't just used for running benchmarks, so I don't think it > should touch the command line unnecessarily. Before this patch it doesn't event > launch the activity. Telemetry seems like the better place for that sort of > stuff, and we already do a lot of provisioning there. Agreed, when startup is not affected, pushing the flags is unnecessary and may harm, but for adb_profile_chrome_startup having results offset by incorrect startup settings may be unpleasant too. Oh, by the way, Benoit, please change the issue description to mention adb_profile_chrome_startup instead of --startup.
All done. Thank you ! https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/ch... File tools/profile_chrome/chrome_startup_controller_unittest.py (right): https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/ch... tools/profile_chrome/chrome_startup_controller_unittest.py:14: categories = '*' On 2015/01/28 09:21:49, Sami (OoO) wrote: > These two seem to be unused? Done. https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/ch... tools/profile_chrome/chrome_startup_controller_unittest.py:17: self.device, self.package_info, False, "https://www.google.com") On 2015/01/28 09:21:49, Sami (OoO) wrote: > nit: single quotes please. Done. https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/fl... File tools/profile_chrome/flags.py (right): https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome/fl... tools/profile_chrome/flags.py:12: 'enable both types of categories. Use "list" to see ' On 2015/01/28 09:21:49, Sami (OoO) wrote: > s/enable/enabling/ Done. https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome_st... File tools/profile_chrome_startup.py (right): https://codereview.chromium.org/879853002/diff/100001/tools/profile_chrome_st... tools/profile_chrome_startup.py:34: 'https://wwww.google.com', default='https://www.google.com', On 2015/01/28 09:21:49, Sami (OoO) wrote: > s/wwww/www/ Done.
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/879853002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/477e91f0a79fef8e766d07f228e67b645509e0c4 Cr-Commit-Position: refs/heads/master@{#313499} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
