Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9570017:
Defer shutting down Chrome after the CF automation connection goes away. (Closed)
|
Created:
8 years, 9 months ago by robertshield Modified:
8 years, 9 months ago CC:
chromium-reviews, kkania, erikwright (departed) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDefer shutting down Chrome after the CF automation connection goes away.
BUG=98506
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124942
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review feedback. #Patch Set 3 : Making the shutdown delay command-line configurable to allow for field experiments. #
Total comments: 7
Patch Set 4 : grt's feedback #
Total comments: 2
Patch Set 5 : Addressing grt's latest nit. #
Total comments: 8
Patch Set 6 : #
Messages
Total messages: 18 (0 generated)
For initial discussion, this appears to work like a charm. Looking at writing a test for this as well (although it will be hard to get that test to run in less than kShutdownDelaySeconds :-))
LGTM http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.cc:14: const int kShutdownDelaySeconds = 30; Would be great to make this controllable from the registry so that we can experiment with values easily. http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.cc:34: extra newline?
That's really straightforward. Even better that it actually works! https://chromiumcodereview.appspot.com/9570017/diff/1/chrome/browser/automati... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9570017/diff/1/chrome/browser/automati... chrome/browser/automation/chrome_frame_automation_provider.cc:34: remove extra newline https://chromiumcodereview.appspot.com/9570017/diff/1/chrome/browser/automati... chrome/browser/automation/chrome_frame_automation_provider.cc:102: if (g_browser_process) { remove extra braces https://chromiumcodereview.appspot.com/9570017/diff/1/chrome/browser/automati... File chrome/browser/automation/chrome_frame_automation_provider.h (right): https://chromiumcodereview.appspot.com/9570017/diff/1/chrome/browser/automati... chrome/browser/automation/chrome_frame_automation_provider.h:37: // Called to release this an instance's ref count on the global BrowserProcess nix "an "
lgtm
http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.cc:14: const int kShutdownDelaySeconds = 30; On 2012/03/01 16:57:12, slightlyoff wrote: > Would be great to make this controllable from the registry so that we can > experiment with values easily. True, but this runs on the UI thread which (I think) needs to defer off to another thread for registry access. Also, a registry value won't do for in-the-field experiments without a lot of extra plumbing through the installer. Mind if I mark this as a TODO() for now? http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.cc:34: On 2012/03/01 16:57:12, slightlyoff wrote: > extra newline? Done. http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.cc:34: On 2012/03/01 16:59:31, grt wrote: > remove extra newline Done. http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.cc:102: if (g_browser_process) { On 2012/03/01 16:59:31, grt wrote: > remove extra braces Done. http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... File chrome/browser/automation/chrome_frame_automation_provider.h (right): http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... chrome/browser/automation/chrome_frame_automation_provider.h:37: // Called to release this an instance's ref count on the global BrowserProcess On 2012/03/01 16:59:31, grt wrote: > nix "an " Done.
LGTM
On 2012/03/02 15:17:17, robertshield wrote: > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > File chrome/browser/automation/chrome_frame_automation_provider.cc (right): > > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > chrome/browser/automation/chrome_frame_automation_provider.cc:14: const int > kShutdownDelaySeconds = 30; > On 2012/03/01 16:57:12, slightlyoff wrote: > > Would be great to make this controllable from the registry so that we can > > experiment with values easily. > > True, but this runs on the UI thread which (I think) needs to defer off to > another thread for registry access. Also, a registry value won't do for > in-the-field experiments without a lot of extra plumbing through the installer. > Mind if I mark this as a TODO() for now? Actually scratch that. After speaking with our local "is it really faster" guru, I need to plumb this through a command line arg and add some field trial stuff to our histograms to figure out exactly how much this helps. I'll make this setting configurable while I'm at it. > > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > chrome/browser/automation/chrome_frame_automation_provider.cc:34: > On 2012/03/01 16:57:12, slightlyoff wrote: > > extra newline? > > Done. > > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > chrome/browser/automation/chrome_frame_automation_provider.cc:34: > On 2012/03/01 16:59:31, grt wrote: > > remove extra newline > > Done. > > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > chrome/browser/automation/chrome_frame_automation_provider.cc:102: if > (g_browser_process) { > On 2012/03/01 16:59:31, grt wrote: > > remove extra braces > > Done. > > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > File chrome/browser/automation/chrome_frame_automation_provider.h (right): > > http://codereview.chromium.org/9570017/diff/1/chrome/browser/automation/chrom... > chrome/browser/automation/chrome_frame_automation_provider.h:37: // Called to > release this an instance's ref count on the global BrowserProcess > On 2012/03/01 16:59:31, grt wrote: > > nix "an " > > Done.
ptal - will add the Chrome Frame end of this in another imminent CL.
http://codereview.chromium.org/9570017/diff/7002/chrome/browser/automation/ch... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/7002/chrome/browser/automation/ch... chrome/browser/automation/chrome_frame_automation_provider.cc:29: if (cmd_line.HasSwitch(switches::kChromeFrameShutdownDelay)) { please rework this block to: CommandLine::StringType shutdown_delay( cmd_line.GetSwitchValueNative(switches::kChromeFrameShutdownDelay)); if (!shutdown_delay.empty()) { // delay } else { // no delay } this avoids a string conversion and an extra lookup into cmd_line. http://codereview.chromium.org/9570017/diff/7002/chrome/browser/automation/ch... chrome/browser/automation/chrome_frame_automation_provider.cc:37: base::StringToInt(shutdown_delay, &shutdown_delay_seconds); in worst-case conditions, shutdown_delay_seconds could be set to INT_MAX or INT_MIN. how about erring on the safe side and ensuring that shutdown_delay_seconds is positive and less than some reasonable enormous upper-bound (e.g., 60*60*24)? http://codereview.chromium.org/9570017/diff/7002/chrome/common/chrome_switches.h File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9570017/diff/7002/chrome/common/chrome_switche... chrome/common/chrome_switches.h:54: extern const char kChromeFrameShutdownDelay[]; consider removing "ChromeFrame" from the name and making this general-purpose.
http://codereview.chromium.org/9570017/diff/7002/chrome/browser/automation/ch... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/7002/chrome/browser/automation/ch... chrome/browser/automation/chrome_frame_automation_provider.cc:29: if (cmd_line.HasSwitch(switches::kChromeFrameShutdownDelay)) { On 2012/03/02 20:13:37, grt wrote: > please rework this block to: > > CommandLine::StringType shutdown_delay( > cmd_line.GetSwitchValueNative(switches::kChromeFrameShutdownDelay)); > if (!shutdown_delay.empty()) { > // delay > } else { > // no delay > } > > this avoids a string conversion and an extra lookup into cmd_line. Done. http://codereview.chromium.org/9570017/diff/7002/chrome/browser/automation/ch... chrome/browser/automation/chrome_frame_automation_provider.cc:37: base::StringToInt(shutdown_delay, &shutdown_delay_seconds); On 2012/03/02 20:13:37, grt wrote: > in worst-case conditions, shutdown_delay_seconds could be set to INT_MAX or > INT_MIN. how about erring on the safe side and ensuring that > shutdown_delay_seconds is positive and less than some reasonable enormous > upper-bound (e.g., 60*60*24)? Done. http://codereview.chromium.org/9570017/diff/7002/chrome/common/chrome_switches.h File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9570017/diff/7002/chrome/common/chrome_switche... chrome/common/chrome_switches.h:54: extern const char kChromeFrameShutdownDelay[]; On 2012/03/02 20:13:37, grt wrote: > consider removing "ChromeFrame" from the name and making this general-purpose. This is used specifically in the ChromeFrameAutomationProvider. I don't want to move it up to the general AutomationProvider without a use case.
http://codereview.chromium.org/9570017/diff/7002/chrome/common/chrome_switches.h File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9570017/diff/7002/chrome/common/chrome_switche... chrome/common/chrome_switches.h:54: extern const char kChromeFrameShutdownDelay[]; On 2012/03/02 20:33:37, robertshield wrote: > On 2012/03/02 20:13:37, grt wrote: > > consider removing "ChromeFrame" from the name and making this general-purpose. > > This is used specifically in the ChromeFrameAutomationProvider. I don't want to > move it up to the general AutomationProvider without a use case. Duh. Somehow I missed the "chrome_frame" part of the filename. :-( http://codereview.chromium.org/9570017/diff/7003/chrome/browser/automation/ch... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/7003/chrome/browser/automation/ch... chrome/browser/automation/chrome_frame_automation_provider.cc:44: if (shutdown_delay_seconds > kMaxChromeShutdownDelaySeconds) nit: else if
http://codereview.chromium.org/9570017/diff/7003/chrome/browser/automation/ch... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/7003/chrome/browser/automation/ch... chrome/browser/automation/chrome_frame_automation_provider.cc:44: if (shutdown_delay_seconds > kMaxChromeShutdownDelaySeconds) On 2012/03/02 20:43:23, grt wrote: > nit: else if Done.
Nice! LGTM.
http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... chrome/browser/automation/chrome_frame_automation_provider.cc:34: VLOG(1) << "ChromeFrameAutomationProvider: " this is going to use streamio which, IIRC, once had a static initializer. Do we still do that in Chrome? http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... chrome/browser/automation/chrome_frame_automation_provider.cc:45: shutdown_delay_seconds = kMaxChromeShutdownDelaySeconds; Are there min/max functions somewhere for this? http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... chrome/browser/automation/chrome_frame_automation_provider.cc:48: // intra-page load times. Add a bug # for tracking perf here? http://codereview.chromium.org/9570017/diff/11009/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9570017/diff/11009/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:140: const char kChromeFrameShutdownDelay[] = "chrome-frame-shutdown-delay"; Why is "chrome frame" in the name? Why not just "shutdown-delay"?
Thanks, PTAL! http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... File chrome/browser/automation/chrome_frame_automation_provider.cc (right): http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... chrome/browser/automation/chrome_frame_automation_provider.cc:34: VLOG(1) << "ChromeFrameAutomationProvider: " On 2012/03/05 12:35:53, slightlyoff wrote: > this is going to use streamio which, IIRC, once had a static initializer. Do we > still do that in Chrome? We use this pattern to log all throughout the code base. What do you suggest as an alternative? http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... chrome/browser/automation/chrome_frame_automation_provider.cc:45: shutdown_delay_seconds = kMaxChromeShutdownDelaySeconds; On 2012/03/05 12:35:53, slightlyoff wrote: > Are there min/max functions somewhere for this? Done. http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... chrome/browser/automation/chrome_frame_automation_provider.cc:48: // intra-page load times. On 2012/03/05 12:35:53, slightlyoff wrote: > Add a bug # for tracking perf here? Good idea, I'll track it under the original bug number if that's alright. http://codereview.chromium.org/9570017/diff/11009/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9570017/diff/11009/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:140: const char kChromeFrameShutdownDelay[] = "chrome-frame-shutdown-delay"; On 2012/03/05 12:35:53, slightlyoff wrote: > Why is "chrome frame" in the name? Why not just "shutdown-delay"? Because this applies only to Chrome Frame. It is used in the ChromeFrameAutomationProvider, which is only used when Chrome Frame is in the picture (see earlier review comment). I considered making it generic and adding it to AutomationProvider, but that would require a little more plumbing and I don't have a use case for it.
On 2012/03/05 13:33:25, robertshield wrote: > Thanks, PTAL! > > http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... > File chrome/browser/automation/chrome_frame_automation_provider.cc (right): > > http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... > chrome/browser/automation/chrome_frame_automation_provider.cc:34: VLOG(1) << > "ChromeFrameAutomationProvider: " > On 2012/03/05 12:35:53, slightlyoff wrote: > > this is going to use streamio which, IIRC, once had a static initializer. Do > we > > still do that in Chrome? > > We use this pattern to log all throughout the code base. What do you suggest as > an alternative? As long as it's what we're doing elsewhere, LGTM. > http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... > chrome/browser/automation/chrome_frame_automation_provider.cc:45: > shutdown_delay_seconds = kMaxChromeShutdownDelaySeconds; > On 2012/03/05 12:35:53, slightlyoff wrote: > > Are there min/max functions somewhere for this? > > Done. > > http://codereview.chromium.org/9570017/diff/11009/chrome/browser/automation/c... > chrome/browser/automation/chrome_frame_automation_provider.cc:48: // intra-page > load times. > On 2012/03/05 12:35:53, slightlyoff wrote: > > Add a bug # for tracking perf here? > > Good idea, I'll track it under the original bug number if that's alright. SGTM. > http://codereview.chromium.org/9570017/diff/11009/chrome/common/chrome_switch... > File chrome/common/chrome_switches.cc (right): > > http://codereview.chromium.org/9570017/diff/11009/chrome/common/chrome_switch... > chrome/common/chrome_switches.cc:140: const char kChromeFrameShutdownDelay[] > = "chrome-frame-shutdown-delay"; > On 2012/03/05 12:35:53, slightlyoff wrote: > > Why is "chrome frame" in the name? Why not just "shutdown-delay"? > > Because this applies only to Chrome Frame. It is used in the > ChromeFrameAutomationProvider, which is only used when Chrome Frame is in the > picture (see earlier review comment). Great. > I considered making it generic and adding it to AutomationProvider, but that > would require a little more plumbing and I don't have a use case for it. No worries. LGTM++!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertshield@chromium.org/9570017/19002
Change committed as 124942 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
