Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6462024:
Register Application Restart with Windows Restart Manager (Closed)
|
Created:
9 years, 10 months ago by msw Modified:
9 years, 7 months ago CC:
chromium-reviews, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRegister Application Restart with Windows Restart Manager on browser_main startup for Windows Vista / Server 2008 and beyond. This launches Chrome and restores tabs if the computer is restarted as the result of an update. Use GetFunctionPointer/GetProcAddress to avoid XP link and run errors. Make changes to support necessary CommandLine operations. Other minor cleanup and refactoring.
BUG=70824
TEST=Reboot from update or ::ExitWindowsEx(EWX_RESTARTAPPS... Chrome should restart and restore tabs on login. Test launching Chrome with a variety of command-line switches and arguments.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75405
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75516
Patch Set 1 #Patch Set 2 : "Fix comments, Append CommandLine args." #Patch Set 3 : "Update comment." #
Total comments: 8
Patch Set 4 : Update comments. #
Total comments: 16
Patch Set 5 : Address feedback, limit to Vista+, add switch warning. #Patch Set 6 : Address feedback, limit to Vista+, add switch warning. #Patch Set 7 : Sync #
Total comments: 3
Patch Set 8 : Use GetFunctionPointer to avoid XP link and run errors. #
Total comments: 8
Patch Set 9 : Address comments #
Messages
Total messages: 20 (0 generated)
Evan, please review the CommandLine changes. My questions/considerations: - Should we avoid registering incognito, tests, or non-browser processes that might run through browser_main? - Should we strip command-line switches/args between --flag-switches-begin and --flag-switches-end? - Should we strip all command-line arguments (just the initial pages to load) anyway, since we're using switches::kRestoreLastSession? - Should we try to support "envp" environmental command-line parameters? - PerfTimer showed a 0ms execution time on Debug; I have not measured cycles. Should I bother to use PostDelayedTask or do anything to postpone this operation from startup? - Should we register on WM_QUERYENDSESSION, rather than around startup? (This works, but would fire once for each WidgetWin / Chrome window. Repeatedly calling RegisterApplicationRestart doesn't have apparent negative effects, but would possibly need to be avoided/mitigated. Also, registering on WM_QUERYENDSESSION wouldn’t be conducive to extending this functionality to crashes/hangs/patches...) - Should I investigate extending this functionality to crashes/hangs/patches? - Is it possible to write a test that runs across a Windows restart?
http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main.... chrome/browser/browser_main.cc:1521: RegisterApplicationRestart(parsed_command_line); You might add a quick note here saying what this does, like the comment for PrepareRestartOnCrashEnvironment does above it. Given the size of BrowserMain(), you should also note if doing it at this particular point is at all important, or if you just threw it in here because this seemed good enough :-) http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:150: // Rebuild the CommandLine without the program for Windows Restart Manager. Rather than say what the code does, say why, e.g. // The Windows Restart Manager expects a string of command line flags only, without the program. or some such. http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:161: RESTART_NO_CRASH | RESTART_NO_HANG | RESTART_NO_PATCH); I am OK with this if cpu is OK :-) http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... File chrome/browser/browser_main_win.h (right): http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... chrome/browser/browser_main_win.h:24: // Windows Restart Manager launches Chrome and restores tabs if the computer is Registers Chrome with the Windows Restart Manager, which will restore the Chrome session when the computer is restarted after a system update.
Done. Ping: cpu & evan. http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main.... chrome/browser/browser_main.cc:1521: RegisterApplicationRestart(parsed_command_line); On 2011/02/10 16:56:49, Ben Goodger wrote: > You might add a quick note here saying what this does, like the comment for > PrepareRestartOnCrashEnvironment does above it. > > Given the size of BrowserMain(), you should also note if doing it at this > particular point is at all important, or if you just threw it in here because > this seemed good enough :-) Done. Added your same comment from the function definition plus a rationale for calling in browser_main; please let me know of any better code paths to host this call. http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:150: // Rebuild the CommandLine without the program for Windows Restart Manager. On 2011/02/10 16:56:49, Ben Goodger wrote: > Rather than say what the code does, say why, e.g. > > // The Windows Restart Manager expects a string of command line flags only, > without the program. > > or some such. Done. http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:161: RESTART_NO_CRASH | RESTART_NO_HANG | RESTART_NO_PATCH); On 2011/02/10 16:56:49, Ben Goodger wrote: > I am OK with this if cpu is OK :-) Yeah, we may want to handle crashes, hangs, and patches... However, the crash handling is redundant with our custom module and dialog, and I haven't explored hangs or patches. http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... File chrome/browser/browser_main_win.h (right): http://codereview.chromium.org/6462024/diff/6002/chrome/browser/browser_main_... chrome/browser/browser_main_win.h:24: // Windows Restart Manager launches Chrome and restores tabs if the computer is On 2011/02/10 16:56:49, Ben Goodger wrote: > Registers Chrome with the Windows Restart Manager, which will restore the Chrome > session when the computer is restarted after a system update. Done.
My main concern is how this interacts with the restart-on-crash code that we already have in breakpad_win.cc. Look at DumpDoneCallback(). http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main.cc:1526: // but should run on startup if extended to handle crashes/hangs/patches. we already handle this restart on crashes see DumpDoneCallback() in breakpad_win.cc http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:148: why not return bool ? http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:158: PCWSTR command_line_string = command_line.command_line_string().c_str(); we don't use PCWSTR replace with const wchar_t*
http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc File base/command_line.cc (right): http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcode44 base/command_line.cc:44: namespace { Thanks for the cleanup! Can you remove the "static" qualifiers on the functions in this anon namespace? http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcod... base/command_line.cc:475: command_line_string_.append(L" --"); This makes me anxious. Does this mean we do the wrong thing if you mix calls to AppendArgs and other calls? I had always intended to remove command_line_string_ and instead generate it on the fly from the args_ array to avoid these kinds of issues, but I never got around to it... http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:149: void RegisterApplicationRestart(const CommandLine &parsed_command_line) { & on the left of the space http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:154: command_line.AppendArgs(parsed_command_line); I wonder if, rather than adding these extra APIs, it would be sufficient to create a CommandLine where the "program" was the empty string?
Done most; I propose to fix and cleanup CommandLine code in a new prerequisite blocking Issue. I'll restart this review thread when that is ready. http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc File base/command_line.cc (right): http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcode44 base/command_line.cc:44: namespace { On 2011/02/10 19:38:31, Evan Martin wrote: > Thanks for the cleanup! Can you remove the "static" qualifiers on the functions > in this anon namespace? Done. http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcod... base/command_line.cc:475: command_line_string_.append(L" --"); On 2011/02/10 19:38:31, Evan Martin wrote: > This makes me anxious. Does this mean we do the wrong thing if you mix calls to > AppendArgs and other calls? > > I had always intended to remove command_line_string_ and instead generate it on > the fly from the args_ array to avoid these kinds of issues, but I never got > around to it... That's an excellent point, appending switches after args would break the switches. There's a *lot* of room for improvement in this code. I will create a new issue and tackle this prerequisite functionality asap. http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main.cc:1526: // but should run on startup if extended to handle crashes/hangs/patches. On 2011/02/10 18:59:24, cpu wrote: > we already handle this restart on crashes see DumpDoneCallback() in > breakpad_win.cc Yup, I'm aware but thought it's worth considering using the OS mechanism (although ours collects good data, etc.). The two can co-exist, Restart Manager just alters the "Google Chrome has stopped working" dialog to offer "Restart the program" instead of "Close the program" (if Chrome ran for at least a minute). We could potentially leave breakpad in but eliminate our dialog and use the native one (only if chrome was running for more than a minute?)... Newsflash to myself: Restart Manager only exists on Vista+!!! http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:148: On 2011/02/10 18:59:24, cpu wrote: > why not return bool ? Done. http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:149: void RegisterApplicationRestart(const CommandLine &parsed_command_line) { On 2011/02/10 19:38:31, Evan Martin wrote: > & on the left of the space Done. Also fixed other instances in this file. http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:154: command_line.AppendArgs(parsed_command_line); On 2011/02/10 19:38:31, Evan Martin wrote: > I wonder if, rather than adding these extra APIs, it would be sufficient to > create a CommandLine where the "program" was the empty string? Yes, I propose to fix and cleanup CommandLine code in a new prerequisite blocking Issue. http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:158: PCWSTR command_line_string = command_line.command_line_string().c_str(); On 2011/02/10 18:59:24, cpu wrote: > we don't use PCWSTR replace with const wchar_t* Done.
Please review and see new tangentially related Issue 73195. Elliot, please review CommandLine (EvanM is on vacation). http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc File base/command_line.cc (right): http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcod... base/command_line.cc:475: command_line_string_.append(L" --"); On 2011/02/10 19:54:54, msw wrote: > On 2011/02/10 19:38:31, Evan Martin wrote: > > This makes me anxious. Does this mean we do the wrong thing if you mix calls > to > > AppendArgs and other calls? > > > > I had always intended to remove command_line_string_ and instead generate it > on > > the fly from the args_ array to avoid these kinds of issues, but I never got > > around to it... > > That's an excellent point, appending switches after args would break the > switches. There's a *lot* of room for improvement in this code. I will create a > new issue and tackle this prerequisite functionality asap. I added a comment above the AppendSwitch* declarations for now. I'll follow up on the extensive amount of needed CommandLine cleanup work with Issue 73195; this should suffice in the mean time. Please let me know if you disagree. http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:154: command_line.AppendArgs(parsed_command_line); On 2011/02/10 19:54:54, msw wrote: > On 2011/02/10 19:38:31, Evan Martin wrote: > > I wonder if, rather than adding these extra APIs, it would be sufficient to > > create a CommandLine where the "program" was the empty string? > > Yes, I propose to fix and cleanup CommandLine code in a new prerequisite > blocking Issue. I'll follow up on the extensive amount of needed CommandLine cleanup work with Issue 73195; this should suffice in the mean time. Please let me know if you disagree.
LGTM assuming you cleanup 73195 later. On 2011/02/16 23:15:53, msw wrote: > Please review and see new tangentially related Issue 73195. > Elliot, please review CommandLine (EvanM is on vacation). > > http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc > File base/command_line.cc (right): > > http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcod... > base/command_line.cc:475: command_line_string_.append(L" --"); > On 2011/02/10 19:54:54, msw wrote: > > On 2011/02/10 19:38:31, Evan Martin wrote: > > > This makes me anxious. Does this mean we do the wrong thing if you mix > calls > > to > > > AppendArgs and other calls? > > > > > > I had always intended to remove command_line_string_ and instead generate it > > on > > > the fly from the args_ array to avoid these kinds of issues, but I never got > > > around to it... > > > > That's an excellent point, appending switches after args would break the > > switches. There's a *lot* of room for improvement in this code. I will create > a > > new issue and tackle this prerequisite functionality asap. > > I added a comment above the AppendSwitch* declarations for now. I'll follow up > on the extensive amount of needed CommandLine cleanup work with Issue 73195; > this should suffice in the mean time. Please let me know if you disagree. > > http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... > File chrome/browser/browser_main_win.cc (right): > > http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... > chrome/browser/browser_main_win.cc:154: > command_line.AppendArgs(parsed_command_line); > On 2011/02/10 19:54:54, msw wrote: > > On 2011/02/10 19:38:31, Evan Martin wrote: > > > I wonder if, rather than adding these extra APIs, it would be sufficient to > > > create a CommandLine where the "program" was the empty string? > > > > Yes, I propose to fix and cleanup CommandLine code in a new prerequisite > > blocking Issue. > > I'll follow up on the extensive amount of needed CommandLine cleanup work with > Issue 73195; this should suffice in the mean time. Please let me know if you > disagree.
On 2011/02/16 23:21:25, Elliot Glaysher wrote: > LGTM assuming you cleanup 73195 later. > > On 2011/02/16 23:15:53, msw wrote: > > Please review and see new tangentially related Issue 73195. > > Elliot, please review CommandLine (EvanM is on vacation). > > > > http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc > > File base/command_line.cc (right): > > > > > http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcod... > > base/command_line.cc:475: command_line_string_.append(L" --"); > > On 2011/02/10 19:54:54, msw wrote: > > > On 2011/02/10 19:38:31, Evan Martin wrote: > > > > This makes me anxious. Does this mean we do the wrong thing if you mix > > calls > > > to > > > > AppendArgs and other calls? > > > > > > > > I had always intended to remove command_line_string_ and instead generate > it > > > on > > > > the fly from the args_ array to avoid these kinds of issues, but I never > got > > > > around to it... > > > > > > That's an excellent point, appending switches after args would break the > > > switches. There's a *lot* of room for improvement in this code. I will > create > > a > > > new issue and tackle this prerequisite functionality asap. > > > > I added a comment above the AppendSwitch* declarations for now. I'll follow up > > on the extensive amount of needed CommandLine cleanup work with Issue 73195; > > this should suffice in the mean time. Please let me know if you disagree. > > > > > http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... > > File chrome/browser/browser_main_win.cc (right): > > > > > http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... > > chrome/browser/browser_main_win.cc:154: > > command_line.AppendArgs(parsed_command_line); > > On 2011/02/10 19:54:54, msw wrote: > > > On 2011/02/10 19:38:31, Evan Martin wrote: > > > > I wonder if, rather than adding these extra APIs, it would be sufficient > to > > > > create a CommandLine where the "program" was the empty string? > > > > > > Yes, I propose to fix and cleanup CommandLine code in a new prerequisite > > > blocking Issue. > > > > I'll follow up on the extensive amount of needed CommandLine cleanup work with > > Issue 73195; this should suffice in the mean time. Please let me know if you > > disagree. Ben, Carlos: Do I have LGTM from you? The Windows ExtensionApiTest.WebSocket timeout on r75165 looks like a fluke, passing bot at LKGR on the way: http://build.chromium.org/p/tryserver.chromium/builders/win/builds/16347/step...
On 2011/02/17 01:25:52, msw wrote: > On 2011/02/16 23:21:25, Elliot Glaysher wrote: > > LGTM assuming you cleanup 73195 later. > > > > On 2011/02/16 23:15:53, msw wrote: > > > Please review and see new tangentially related Issue 73195. > > > Elliot, please review CommandLine (EvanM is on vacation). > > > > > > http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc > > > File base/command_line.cc (right): > > > > > > > > > http://codereview.chromium.org/6462024/diff/10001/base/command_line.cc#newcod... > > > base/command_line.cc:475: command_line_string_.append(L" --"); > > > On 2011/02/10 19:54:54, msw wrote: > > > > On 2011/02/10 19:38:31, Evan Martin wrote: > > > > > This makes me anxious. Does this mean we do the wrong thing if you mix > > > calls > > > > to > > > > > AppendArgs and other calls? > > > > > > > > > > I had always intended to remove command_line_string_ and instead > generate > > it > > > > on > > > > > the fly from the args_ array to avoid these kinds of issues, but I never > > got > > > > > around to it... > > > > > > > > That's an excellent point, appending switches after args would break the > > > > switches. There's a *lot* of room for improvement in this code. I will > > create > > > a > > > > new issue and tackle this prerequisite functionality asap. > > > > > > I added a comment above the AppendSwitch* declarations for now. I'll follow > up > > > on the extensive amount of needed CommandLine cleanup work with Issue 73195; > > > this should suffice in the mean time. Please let me know if you disagree. > > > > > > > > > http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... > > > File chrome/browser/browser_main_win.cc (right): > > > > > > > > > http://codereview.chromium.org/6462024/diff/10001/chrome/browser/browser_main... > > > chrome/browser/browser_main_win.cc:154: > > > command_line.AppendArgs(parsed_command_line); > > > On 2011/02/10 19:54:54, msw wrote: > > > > On 2011/02/10 19:38:31, Evan Martin wrote: > > > > > I wonder if, rather than adding these extra APIs, it would be sufficient > > to > > > > > create a CommandLine where the "program" was the empty string? > > > > > > > > Yes, I propose to fix and cleanup CommandLine code in a new prerequisite > > > > blocking Issue. > > > > > > I'll follow up on the extensive amount of needed CommandLine cleanup work > with > > > Issue 73195; this should suffice in the mean time. Please let me know if you > > > disagree. > > Ben, Carlos: Do I have LGTM from you? > > The Windows ExtensionApiTest.WebSocket timeout on r75165 looks like a fluke, > passing bot at LKGR on the way: > http://build.chromium.org/p/tryserver.chromium/builders/win/builds/16347/step... Yikes, nvm! Surprise new failure looks related... BrowserTest.CommandCreateAppShortcutHttp
Okay, tests are green after a sync. Ben, Carlos: Do I have LGTM from you?
http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:161: HRESULT hr = ::RegisterApplicationRestart(command_string, One question: does this result in blocking I/O? (e.g. registry write?)
http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:161: HRESULT hr = ::RegisterApplicationRestart(command_string, FileMon shows no File/Registry access while executing this function call. On 2011/02/17 04:12:29, Ben Goodger wrote: > One question: does this result in blocking I/O? (e.g. registry write?)
Sorry, ProcessMon is what I meant. On 2011/02/17 10:30:54, Finnur wrote: > http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... > File chrome/browser/browser_main_win.cc (right): > > http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... > chrome/browser/browser_main_win.cc:161: HRESULT hr = > ::RegisterApplicationRestart(command_string, > FileMon shows no File/Registry access while executing this function call. > > On 2011/02/17 04:12:29, Ben Goodger wrote: > > One question: does this result in blocking I/O? (e.g. registry write?)
I agree with Finnur; see my comment below. http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/21010/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:161: HRESULT hr = ::RegisterApplicationRestart(command_string, On 2011/02/17 04:12:29, Ben Goodger wrote: > One question: does this result in blocking I/O? (e.g. registry write?) Short answer: RegisterApplicationRestart() doesn't appear to perform a synchronous registry/disk operation (although documentation is elusive), and PerfTimer reports 0ms to execute. There are surprisingly thousands of registry/file ops by Chrome on startup! I'll watch the startup perf dashboards closely after landing. Long answer: Using Process Monitor while running Chrome Debug via Visual Studio on Windows 7 shows 0 registry operations attributed to chrome.exe during RegisterApplicationRestart(); the same is true for my test application. I don't see any registry operations run by other processes during RegisterApplicationRestart() that are obviously related to Chrome or Windows Restart Manager. Regardless, Chrome apparently runs thousands of registry/file operations on startup (see below), it's unlikely that several more would be significant. All that said, I don't claim to have tremendous expertise here, and these references allude to the Windows Restart Manager API itself using the registry (msdn.microsoft.com/en-us/library/bb757039.aspx and ERROR_SEM_TIMEOUT from RmGetList/RmShutdown/RmRestart via msdn.microsoft.com/en-us/library/aa373649.aspx), although RegisterApplicationRestart is listed separately as part of the "Supporting API". Are we concerned that thousands of registry/file ops are performed during startup? There are ~363 registry ops and ~900 file ops by Chrome.exe before BrowserMain(). There are ~1968 *additional* registry ops and ~740 *additional* file ops in BrowserMain() before browser_init.Start(). Altogether, I see ~12k registry ops and ~6k file ops by chrome.exe during its first few seconds of runtime.
Did Finnur and I address your blocking I/O question satisfactorily? Is there a better way to investigate this concern? I'm hesitant to land without those precious four letters.
Oh yes. LGTM. On Fri, Feb 18, 2011 at 8:56 AM, <msw@chromium.org> wrote: > Did Finnur and I address your blocking I/O question satisfactorily? > Is there a better way to investigate this concern? > I'm hesitant to land without those precious four letters. > > > http://codereview.chromium.org/6462024/ >
Sorry for the churn; please review. Use GetFunctionPointer/GetProcAddress to avoid XP link and run errors. Should I pursue adding similar functionality to Windows XP with another CL? Perhaps via: HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\RunOnce
OK http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:45: typedef DECLSPEC_IMPORT HRESULT (STDAPICALLTYPE *RAR)( STDAPICALLTYPE* http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:46: const wchar_t *pwzCommandline, we don't use hungarian... const wchar_t* command_line instead. http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:47: DWORD dwFlags); flags http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:162: library.GetFunctionPointer("RegisterApplicationRestart")); 4-space indent
Done. FYI, PerfTimer still shows 0ms, and there's no new registry access via this change. http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:45: typedef DECLSPEC_IMPORT HRESULT (STDAPICALLTYPE *RAR)( On 2011/02/18 23:41:51, Ben Goodger wrote: > STDAPICALLTYPE* Done. http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:46: const wchar_t *pwzCommandline, On 2011/02/18 23:41:51, Ben Goodger wrote: > we don't use hungarian... > > const wchar_t* command_line > > instead. Done. http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:47: DWORD dwFlags); On 2011/02/18 23:41:51, Ben Goodger wrote: > flags Done. http://codereview.chromium.org/6462024/diff/3008/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:162: library.GetFunctionPointer("RegisterApplicationRestart")); On 2011/02/18 23:41:51, Ben Goodger wrote: > 4-space indent Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
