Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14617003:
Make chrome.exe rendezvous with existing chrome process earlier. (Closed)
|
Created:
7 years, 7 months ago by koz (OOO until 15th September) Modified:
7 years, 6 months ago Reviewers:
Shrikant Kelkar, sky, gab, Mark Mentovai, commit-bot: I haz the power, brettw, pastarmovj, cpu_(ooo_6.6-7.5), grt (UTC plus 2) CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, grt (UTC plus 2), huangs (corp), Shrikant Kelkar Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake chrome.exe rendezvous with existing chrome process earlier.
This change causes chrome to attempt to locate and defer to an already running
chrome process earlier in startup (before chrome.dll is loaded).
Some benchmarks* (all times are in ms):
ASUS S200E (low-end netbook):
Unpatched: 1109, 547, 2656, 381
Patched: 125, 125, 78, 78
z620 (high-end dev machine)
Unpatched: 124, 125, 125, 219
Patched: 47, 63, 63, 46
chrome.exe size:
Unpatched: 748K
Patched: 768K
* The period measured was that between chrome.exe being executed and the existing chrome process receiving WM_COPYDATA.
BUG=171588
TBR=cpu@chromium.org, gab@chromium.org, grt@chromium.org, pastarmovj@chromium.org, shrikant@chromium.org, sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204099
Patch Set 1 #
Total comments: 11
Patch Set 2 : respond to comments, add whitelist, add policy check for udd #
Total comments: 4
Patch Set 3 : add profile dir to whitelist #
Total comments: 21
Patch Set 4 : respond to comments #
Total comments: 2
Patch Set 5 : respond to comments / rebase #
Total comments: 28
Patch Set 6 : respond to comments #Patch Set 7 : rebase #
Total comments: 18
Patch Set 8 : respond to comments #
Total comments: 18
Patch Set 9 : respond to comments #
Total comments: 7
Patch Set 10 : . #
Total comments: 2
Patch Set 11 : respond to comments #Patch Set 12 : fix if/enum #
Total comments: 10
Patch Set 13 : respond to comments #Patch Set 14 : respond to comments #
Total comments: 4
Patch Set 15 : respond to comments #
Total comments: 4
Patch Set 16 : add process_id and thread_id check, rebase #Patch Set 17 : fix compile errors #Patch Set 18 : remove comment, add missing #include #Patch Set 19 : rebase #Messages
Total messages: 56 (0 generated)
gab: process_singleton stuff cpu: everything else
drive-by. https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:3073: 'browser/chrome_process_finder_win.cc', chrome_process_finder_win.cc includes chrome/common/chrome_constants.h, so this target needs to depend on ../chrome/common_constants.gyp:common_constants. https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2908: 'browser/ui/metro_chrome_win.cc', browser/ui/metro_chrome_win.cc includes pieces of chrome/installer/util, so this target needs a dependency on installer_util. https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2908: 'browser/ui/metro_chrome_win.cc', i think these files don't belong in chrome/browser/ui if they're linked into their own target that doesn't depend on anything in the browser.
Let me ponder this, we now execute things in different order. Adding Julian to the review. https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... chrome/app/chrome_exe_main_win.cc:46: base::FilePath udd; udd -> data_dir
I'm mostly worried about all "short-lived" command-lines for which chrome.exe is meant to do something quick and exit before even trying to rendez-vous, it would now result in a rendez-vous. (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...) Cheers, Gab
On 2013/04/30 20:26:14, gab wrote: > I'm mostly worried about all "short-lived" command-lines for which chrome.exe is > meant to do something quick and exit before even trying to rendez-vous, it would > now result in a rendez-vous. > > (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...) > > Cheers, > Gab Would an acceptable solution to that problem be to only allow certain flag combinations (eg: none or only --show-app-list)?
Please check the comment inline. Only verifying the user-data-dir cmd flag is not enough since policy can specify the path as well and will override the location. Ping me if you need some info about how to do that. But you can check the code that creates/sets the Win7 launcher bar icons which had the same issue. See this CL: https://codereview.chromium.org/12277002/ https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... chrome/app/chrome_exe_main_win.cc:43: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir)) You have to check that the UserDataDir policy is not set either because it can override the path as well and even has priority over the command line switch.
On May 1, 2013 3:35 AM, <koz@chromium.org> wrote: > > On 2013/04/30 20:26:14, gab wrote: >> >> I'm mostly worried about all "short-lived" command-lines for which chrome.exe > > is >> >> meant to do something quick and exit before even trying to rendez-vous, it > > would >> >> now result in a rendez-vous. > > >> (e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... ) > >> Cheers, >> Gab > > > Would an acceptable solution to that problem be to only allow certain flag > combinations (eg: none or only --show-app-list)? That could work although it might be hard to maintain, i.e., the recent addition of profile shortcuts makes it so most entry points will now have the profile dir flags (it's not hard to add that flag to the list, but how do we make sure that such default flags are added to this list in chrome_exe_main in the future?) > > https://codereview.chromium.org/14617003/
On 2013/05/01 13:17:41, gab wrote: > On May 1, 2013 3:35 AM, <mailto:koz@chromium.org> wrote: > > > > On 2013/04/30 20:26:14, gab wrote: > >> > >> I'm mostly worried about all "short-lived" command-lines for which > chrome.exe > > > > is > >> > >> meant to do something quick and exit before even trying to rendez-vous, > it > > > > would > >> > >> now result in a rendez-vous. > > > > > >> (e.g. > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > ) > > > >> Cheers, > >> Gab > > > > > > Would an acceptable solution to that problem be to only allow certain flag > > combinations (eg: none or only --show-app-list)? > > That could work although it might be hard to maintain, i.e., the recent > addition of profile shortcuts makes it so most entry points will now have > the profile dir flags (it's not hard to add that flag to the list, but how > do we make sure that such default flags are added to this list in > chrome_exe_main in the future?) That's a good question and I have no way of solving it apart from vigilance and code reviews, but the worst case here is the fast path doesn't get taken, which is not a catastrophic failure. I think it's quite rare to modify the default command line flags for all chrome instances, too; the Chrome shortcut on my desktop currently has no extra flags.
Thanks for the comments, all! https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... chrome/app/chrome_exe_main_win.cc:43: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir)) On 2013/05/01 10:54:09, pastarmovj wrote: > You have to check that the UserDataDir policy is not set either because it can > override the path as well and even has priority over the command line switch. Done. https://codereview.chromium.org/14617003/diff/1/chrome/app/chrome_exe_main_wi... chrome/app/chrome_exe_main_win.cc:46: base::FilePath udd; On 2013/04/30 19:33:37, cpu wrote: > udd -> data_dir Done. https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:3073: 'browser/chrome_process_finder_win.cc', On 2013/04/30 14:54:23, grt wrote: > chrome_process_finder_win.cc includes chrome/common/chrome_constants.h, so this > target needs to depend on ../chrome/common_constants.gyp:common_constants. Done. https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2908: 'browser/ui/metro_chrome_win.cc', On 2013/04/30 14:54:23, grt wrote: > i think these files don't belong in chrome/browser/ui if they're linked into > their own target that doesn't depend on anything in the browser. Yeah, it only depends on base/ and chrome/installer/util. Do you have any suggestions for a better home? https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2908: 'browser/ui/metro_chrome_win.cc', On 2013/04/30 14:54:23, grt wrote: > browser/ui/metro_chrome_win.cc includes pieces of chrome/installer/util, so this > target needs a dependency on installer_util. Done.
LGTM with two small nits (I put the nits in revision 2 not 3 but they apply just the same, sorry about that) https://codereview.chromium.org/14617003/diff/12001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/12001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:50: for (size_t i = 0; i < arraysize(kFastStartSwitches); i++) nit: You can use std::find here. E.g: std::string* last = kFastStartSwitches+arraysize(kFastStartSwitches); return std::find(kFastStartSwitches, last, command_line_switch) != last; https://codereview.chromium.org/14617003/diff/12001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/12001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:33: "http://www.google.com/search?q=%s&sourceid=chrome&ie=UTF-8"; nit: Two more spaces.
https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/14617003/diff/1/chrome/chrome_browser_ui.gypi... chrome/chrome_browser_ui.gypi:2908: 'browser/ui/metro_chrome_win.cc', On 2013/05/09 01:47:42, koz wrote: > On 2013/04/30 14:54:23, grt wrote: > > i think these files don't belong in chrome/browser/ui if they're linked into > > their own target that doesn't depend on anything in the browser. > > Yeah, it only depends on base/ and chrome/installer/util. Do you have any > suggestions for a better home? chrome/browser/metro_utils with this gyp stuff in chrome/chrome_metro_utils.gypi and a chrome/browser/metro_utils/DEPS file that contains proper include_rules sounds good to me. i suggest you bounce this and my other gyp-related suggestions off of someone in chrome/OWNERS to see what they think. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:24: int RunChrome(HINSTANCE instance) { put this and all of your new code into an unnamed namespace. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:45: const std::string kFastStartSwitches[] = { this will create exit-time dtors, which is banned. see my suggestion in ContainsNonFastStartFlag for a better way. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:58: const CommandLine::SwitchMap& switches = command_line->GetSwitches(); i think you're better off with: static const char* const kFastStartSwitches[] = { ... }; for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) { if (command_line->HasSwitch(kFastStartSwitches[i])) return true; } return false; which gives you O(NlogM) rather than O(NM) and only creates/destroys one string for each item in kFastStartSwitches. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:77: if (chrome && chrome::AttemptToNotifyRunningChrome(chrome)) return chrome && chrome::AttemptToNotifyRunningChrome(chrome); https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:32: const char kSearchUrl[] = static const char https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:34: DWORD process_id = 0; how about: DCHECK(remote_window); https://codereview.chromium.org/14617003/diff/15001/chrome/browser/policy/pol... File chrome/browser/policy/policy.gyp (right): https://codereview.chromium.org/14617003/diff/15001/chrome/browser/policy/pol... chrome/browser/policy/policy.gyp:1: { missing copyright also, i think this should be a .gypi in the chrome/ directory rather than a .gyp down here. https://codereview.chromium.org/14617003/diff/15001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/14617003/diff/15001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:3152: ['OS=="win"', { my gut tells me this belongs in its own .gypi file
This looks much better https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:17: #include "net/base/escape.h" grt, can we make chrome exe not depend on net. Or is this a lost cause. At least in the gypi I don't see net. https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:58: ::ShellExecuteExA(&sei); Mix of :: For windows api calls see for example line 58 vs line 92. My preference is for making them all have ::
https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:17: #include "net/base/escape.h" On 2013/05/09 18:15:47, cpu wrote: > grt, can we make chrome exe not depend on net. Or is this a lost cause. At least > in the gypi I don't see net. That's probably no good. chrome.exe doesn't currently depend on net.
On 2013/05/09 18:29:24, grt wrote: > https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... > File chrome/browser/chrome_process_finder_win.cc (right): > > https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... > chrome/browser/chrome_process_finder_win.cc:17: #include "net/base/escape.h" > On 2013/05/09 18:15:47, cpu wrote: > > grt, can we make chrome exe not depend on net. Or is this a lost cause. At > least > > in the gypi I don't see net. > > That's probably no good. chrome.exe doesn't currently depend on net. Yes, the dependency on net was very disappointing for me. It's only needed for a call to net::EscapeQueryParamValue(), too, but it seemed like quite a large effort to try and split it out.
sky: Could you make sure my gyp changes are okay? https://codereview.chromium.org/14617003/diff/12001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/12001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:50: for (size_t i = 0; i < arraysize(kFastStartSwitches); i++) On 2013/05/09 09:53:32, pastarmovj wrote: > nit: You can use std::find here. E.g: > std::string* last = kFastStartSwitches+arraysize(kFastStartSwitches); > return std::find(kFastStartSwitches, last, command_line_switch) != last; Done. https://codereview.chromium.org/14617003/diff/12001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/12001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:33: "http://www.google.com/search?q=%s&sourceid=chrome&ie=UTF-8"; On 2013/05/09 09:53:32, pastarmovj wrote: > nit: Two more spaces. Done. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:24: int RunChrome(HINSTANCE instance) { On 2013/05/09 16:10:44, grt wrote: > put this and all of your new code into an unnamed namespace. Done. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:45: const std::string kFastStartSwitches[] = { On 2013/05/09 16:10:44, grt wrote: > this will create exit-time dtors, which is banned. see my suggestion in > ContainsNonFastStartFlag for a better way. Done. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:58: const CommandLine::SwitchMap& switches = command_line->GetSwitches(); On 2013/05/09 16:10:44, grt wrote: > i think you're better off with: > static const char* const kFastStartSwitches[] = { ... }; > for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) { > if (command_line->HasSwitch(kFastStartSwitches[i])) > return true; > } > return false; > which gives you O(NlogM) rather than O(NM) and only creates/destroys one string > for each item in kFastStartSwitches. I think the code you've given will return true iff the command line has at least one switch in it from kFastStartSwitches, but the intention of that list is that only command lines consisting solely of those switches are able to be fast-started, ie: we need to check each flag in the command line against all the flags in kFastStartSwitches. I've expanded on the comment to make this clearer. https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:77: if (chrome && chrome::AttemptToNotifyRunningChrome(chrome)) On 2013/05/09 16:10:44, grt wrote: > return chrome && chrome::AttemptToNotifyRunningChrome(chrome); Done. https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:32: const char kSearchUrl[] = On 2013/05/09 16:10:44, grt wrote: > static const char Done. https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:34: DWORD process_id = 0; On 2013/05/09 16:10:44, grt wrote: > how about: > DCHECK(remote_window); Done. https://codereview.chromium.org/14617003/diff/15001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:58: ::ShellExecuteExA(&sei); On 2013/05/09 18:15:47, cpu wrote: > Mix of :: For windows api calls see for example line 58 vs line 92. My > preference is for making them all have :: Done. https://codereview.chromium.org/14617003/diff/15001/chrome/browser/policy/pol... File chrome/browser/policy/policy.gyp (right): https://codereview.chromium.org/14617003/diff/15001/chrome/browser/policy/pol... chrome/browser/policy/policy.gyp:1: { On 2013/05/09 16:10:44, grt wrote: > missing copyright > also, i think this should be a .gypi in the chrome/ directory rather than a .gyp > down here. Done. https://codereview.chromium.org/14617003/diff/15001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/14617003/diff/15001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:3152: ['OS=="win"', { On 2013/05/09 16:10:44, grt wrote: > my gut tells me this belongs in its own .gypi file Done.
sky->mentovai
https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/15001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:58: const CommandLine::SwitchMap& switches = command_line->GetSwitches(); On 2013/05/10 01:46:48, koz wrote: > On 2013/05/09 16:10:44, grt wrote: > > i think you're better off with: > > static const char* const kFastStartSwitches[] = { ... }; > > for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) { > > if (command_line->HasSwitch(kFastStartSwitches[i])) > > return true; > > } > > return false; > > which gives you O(NlogM) rather than O(NM) and only creates/destroys one > string > > for each item in kFastStartSwitches. > > I think the code you've given will return true iff the command line has at least > one switch in it from kFastStartSwitches, but the intention of that list is that > only command lines consisting solely of those switches are able to be > fast-started, ie: we need to check each flag in the command line against all the > flags in kFastStartSwitches. I've expanded on the comment to make this clearer. oops. i see. apologies for the bad suggestion. https://codereview.chromium.org/14617003/diff/25001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/25001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:51: static const std::string kFastStartSwitches[] = { you cannot use std::string here. go with: static const char* const kFastStartSwitches[] =
EscapeQueryParamValue seems very contained, just copy it over here. What do the other reviews think, heresy?
@cpu, sounds good to me :-) https://codereview.chromium.org/14617003/diff/25001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/25001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:51: static const std::string kFastStartSwitches[] = { On 2013/05/10 16:53:04, grt wrote: > you cannot use std::string here. go with: > static const char* const kFastStartSwitches[] = Oh, of course. Done.
Comments below. Cheers! Gab https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:55: for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) nit: wrap for loop in {} since content is multiple lines https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:61: bool ContainsNonFastStartFlag(CommandLine* command_line) { This algorithm is O(m*n), where: m is the number of fast-start-switches. n is the number of switches on the command-line. This could easily be made O(n) by putting the fast-start-switches in a hash table; I agree however that since m is 2 in this case it's probably faster to avoid building a hash table just for this... Perhaps this should be emphasized by a comment (or even by removing the for loop in IsFastStartSwitch() and just explicitly adding if{}else if{} blocks to check for both switches explicitly..? You could also add a quick check at the beginning of ContainsNonFastStartFlag() to check if the command-line has more switches than there are fast-start-switches to quickly return instead of starting lookups in the SwitchMap (although I guess you could prove that you are guaranteed to return in m+1 lookups (not n) so that the algo is really O(m^2)). https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:62: const CommandLine::SwitchMap& switches = command_line->GetSwitches(); Hmmm... GetSwitches is confusing, it says "// Get a copy of all switches, along with their values."... yet it doesn't return a copy, but a reference to the internal structure... either way, it returns a const& so GetSwitches() can probably be made a const-method, allowing this method to use a const& command_line? https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:64: it != switches.end(); it++) { nit: ++it https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:24: namespace { nit: Add an empty line after start of namespace. https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:29: // fast-rendezvous (see https://codereview.chromium.org/14617003/). I haven't followed the discussion with regards to restructuring includes and dependencies, but can't you move this to an acceptable common-base target (or simply make a mini-target like you did for policy_path_parser)? https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:137: return true; Why add a return here? There was none in the original code, the rest of the rendez-vous still needs to happen after the activation. https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:140: // Send the command line to the remote chrome window. Why move this code after the above activation instead of keeping the order in the original code? https://codereview.chromium.org/14617003/diff/31001/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (left): https://codereview.chromium.org/14617003/diff/31001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:369: remote_window_ = NULL; The moved code no longer resets |remote_window_| to NULL upon failing to send the message. https://codereview.chromium.org/14617003/diff/31001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/14617003/diff/31001/chrome/chrome.gyp#newcode159 chrome/chrome.gyp:159: 'browser/metro_utils/metro_utils.gypi', This is Windows only and should go in the OS=="win" section below. https://codereview.chromium.org/14617003/diff/31001/chrome/chrome_browser_ui.... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/14617003/diff/31001/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:29: 'metro_utils', Win-only
https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:51: static const char* kFastStartSwitches[] = { Use the extra const to keep the whole structure in the RO data segment: static const char* const kFastStartSwitches https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:55: for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) braces on the "for" loop
https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:51: static const char* kFastStartSwitches[] = { On 2013/05/13 13:32:07, grt wrote: > Use the extra const to keep the whole structure in the RO data segment: > static const char* const kFastStartSwitches Done. https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:55: for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) On 2013/05/13 13:27:43, gab wrote: > nit: wrap for loop in {} since content is multiple lines Done. https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:55: for (size_t i = 0; i < arraysize(kFastStartSwitches); ++i) On 2013/05/13 13:32:07, grt wrote: > braces on the "for" loop Done. https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:61: bool ContainsNonFastStartFlag(CommandLine* command_line) { On 2013/05/13 13:27:43, gab wrote: > This algorithm is O(m*n), where: > m is the number of fast-start-switches. > n is the number of switches on the command-line. > > This could easily be made O(n) by putting the fast-start-switches in a hash > table; I agree however that since m is 2 in this case it's probably faster to > avoid building a hash table just for this... > > Perhaps this should be emphasized by a comment (or even by removing the for loop > in IsFastStartSwitch() and just explicitly adding if{}else if{} blocks to check > for both switches explicitly..? > > You could also add a quick check at the beginning of ContainsNonFastStartFlag() > to check if the command-line has more switches than there are > fast-start-switches to quickly return instead of starting lookups in the > SwitchMap (although I guess you could prove that you are guaranteed to return in > m+1 lookups (not n) so that the algo is really O(m^2)). I added the comment and the size check. https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:62: const CommandLine::SwitchMap& switches = command_line->GetSwitches(); On 2013/05/13 13:27:43, gab wrote: > Hmmm... GetSwitches is confusing, it says "// Get a copy of all switches, along > with their values."... yet it doesn't return a copy, but a reference to the > internal structure... either way, it returns a const& so GetSwitches() can > probably be made a const-method, allowing this method to use a const& > command_line? Done. https://codereview.chromium.org/14617003/diff/31001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:64: it != switches.end(); it++) { On 2013/05/13 13:27:43, gab wrote: > nit: ++it Done. https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:24: namespace { On 2013/05/13 13:27:43, gab wrote: > nit: Add an empty line after start of namespace. Done. https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:29: // fast-rendezvous (see https://codereview.chromium.org/14617003/). On 2013/05/13 13:27:43, gab wrote: > I haven't followed the discussion with regards to restructuring includes and > dependencies, but can't you move this to an acceptable common-base target (or > simply make a mini-target like you did for policy_path_parser)? It should be possible to move net/base/escape* to base/. How about we have chrome.exe depend on net (it doesn't bloat the binary due to link-time pruning) for now and I'll put a TODO to move net/base/escape* into base? https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:137: return true; On 2013/05/13 13:27:43, gab wrote: > Why add a return here? There was none in the original code, the rest of the > rendez-vous still needs to happen after the activation. Good catch, thanks. This was an artifact of when I used a delegate interface for the metro activation and so bailing out here needed to happen because in the fast-start case the delegate was NULL. https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:140: // Send the command line to the remote chrome window. On 2013/05/13 13:27:43, gab wrote: > Why move this code after the above activation instead of keeping the order in > the original code? Just because there's no reason to have it come before and this way the definition of to_send occurs closer to where it's used. https://codereview.chromium.org/14617003/diff/31001/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (left): https://codereview.chromium.org/14617003/diff/31001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:369: remote_window_ = NULL; On 2013/05/13 13:27:43, gab wrote: > The moved code no longer resets |remote_window_| to NULL upon failing to send > the message. Done. https://codereview.chromium.org/14617003/diff/31001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/14617003/diff/31001/chrome/chrome.gyp#newcode159 chrome/chrome.gyp:159: 'browser/metro_utils/metro_utils.gypi', On 2013/05/13 13:27:43, gab wrote: > This is Windows only and should go in the OS=="win" section below. Done. https://codereview.chromium.org/14617003/diff/31001/chrome/chrome_browser_ui.... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/14617003/diff/31001/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:29: 'metro_utils', On 2013/05/13 13:27:43, gab wrote: > Win-only Done.
https://codereview.chromium.org/14617003/diff/47001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/47001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:8: #include <algorithm> nit: unused https://codereview.chromium.org/14617003/diff/47001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:52: static const char* const kFastStartSwitches[] = { nit: no "static" at namespace scope in the unnamed namespace. https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:32: // BEGIN COPY from net/base/escape.cc i'm ignoring everything between here and END COPY. https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:156: // Allow the current running browser window making itself the foreground "making" -> "to make" https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.h:10: #include "base/files/file_path.h" move the #include into the .cc file and instead forward decl here with: namespace base { class FilePath; } https://codereview.chromium.org/14617003/diff/47001/chrome/browser/metro_util... File chrome/browser/metro_utils/metro_utils.gypi (right): https://codereview.chromium.org/14617003/diff/47001/chrome/browser/metro_util... chrome/browser/metro_utils/metro_utils.gypi:7: 'installer_util', this also needs base https://codereview.chromium.org/14617003/diff/47001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/14617003/diff/47001/chrome/chrome.gyp#newcode159 chrome/chrome.gyp:159: 'policy.gypi', i think this belongs in the OS!="ios" section below. https://codereview.chromium.org/14617003/diff/47001/chrome/chrome_process_fin... File chrome/chrome_process_finder.gypi (right): https://codereview.chromium.org/14617003/diff/47001/chrome/chrome_process_fin... chrome/chrome_process_finder.gypi:10: 'policy_path_parser', chrome_process_finder depends on base https://codereview.chromium.org/14617003/diff/47001/chrome/policy.gypi File chrome/policy.gypi (right): https://codereview.chromium.org/14617003/diff/47001/chrome/policy.gypi#newcode11 chrome/policy.gypi:11: '<(DEPTH)/chrome/common_constants.gyp:common_constants', policy_path_parser depends on base, too
Thanks! https://codereview.chromium.org/14617003/diff/47001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/47001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:8: #include <algorithm> On 2013/05/14 13:49:32, grt wrote: > nit: unused Done. https://codereview.chromium.org/14617003/diff/47001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:52: static const char* const kFastStartSwitches[] = { On 2013/05/14 13:49:32, grt wrote: > nit: no "static" at namespace scope in the unnamed namespace. Done. https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:32: // BEGIN COPY from net/base/escape.cc On 2013/05/14 13:49:32, grt wrote: > i'm ignoring everything between here and END COPY. Yep. https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:156: // Allow the current running browser window making itself the foreground On 2013/05/14 13:49:32, grt wrote: > "making" -> "to make" Done. https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/14617003/diff/47001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.h:10: #include "base/files/file_path.h" On 2013/05/14 13:49:32, grt wrote: > move the #include into the .cc file and instead forward decl here with: > namespace base { > class FilePath; > } Done. https://codereview.chromium.org/14617003/diff/47001/chrome/browser/metro_util... File chrome/browser/metro_utils/metro_utils.gypi (right): https://codereview.chromium.org/14617003/diff/47001/chrome/browser/metro_util... chrome/browser/metro_utils/metro_utils.gypi:7: 'installer_util', On 2013/05/14 13:49:32, grt wrote: > this also needs base Done. https://codereview.chromium.org/14617003/diff/47001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/14617003/diff/47001/chrome/chrome.gyp#newcode159 chrome/chrome.gyp:159: 'policy.gypi', On 2013/05/14 13:49:32, grt wrote: > i think this belongs in the OS!="ios" section below. Done. https://codereview.chromium.org/14617003/diff/47001/chrome/chrome_process_fin... File chrome/chrome_process_finder.gypi (right): https://codereview.chromium.org/14617003/diff/47001/chrome/chrome_process_fin... chrome/chrome_process_finder.gypi:10: 'policy_path_parser', On 2013/05/14 13:49:32, grt wrote: > chrome_process_finder depends on base Done. https://codereview.chromium.org/14617003/diff/47001/chrome/policy.gypi File chrome/policy.gypi (right): https://codereview.chromium.org/14617003/diff/47001/chrome/policy.gypi#newcode11 chrome/policy.gypi:11: '<(DEPTH)/chrome/common_constants.gyp:common_constants', On 2013/05/14 13:49:32, grt wrote: > policy_path_parser depends on base, too Done.
lg https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:29: // fast-rendezvous (see https://codereview.chromium.org/14617003/). On 2013/05/14 08:39:54, koz wrote: > On 2013/05/13 13:27:43, gab wrote: > > I haven't followed the discussion with regards to restructuring includes and > > dependencies, but can't you move this to an acceptable common-base target (or > > simply make a mini-target like you did for policy_path_parser)? > > It should be possible to move net/base/escape* to base/. How about we have > chrome.exe depend on net (it doesn't bloat the binary due to link-time pruning) > for now and I'll put a TODO to move net/base/escape* into base? I'm not an expert with dependencies and link time optimizations, but if you can confirm this doesn't bloat chrome.exe (and others agree with the dependency), that sgtm. https://codereview.chromium.org/14617003/diff/55001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/55001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:76: bool AttemptFastNotify(CommandLine* command_line) { nit: This can also take a const& https://codereview.chromium.org/14617003/diff/55001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/55001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:117: LOG(WARNING) << "In metro mode, but and launch is " << launch; #include "base/logging.h" https://codereview.chromium.org/14617003/diff/55001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:145: file_util::GetCurrentDirectory(&cur_dir); return false if this fails. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:290: DWORD process_id = 0; This code is already in AttemptToNotifyRunningChrome(). https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:301: remote_window_ = NULL; Some of these cases should return PROCESS_NONE here to keep the same behavior. It's probably simpler to just move the enum and have chrome::AttemptToNotifyRunningChrome return an enum (e.g., something like: PROCESS_NOTIFY_SUCCESS (success cases -- we should return early), PROCESS_NOTIFY_FAILED (failed cases -- we should launch), and PROCESS_NOTIFY_WINDOW_HUNG (when we should keep going below -- or, in the chrome.exe case, abandon the early rendez-vous (since we can't pop a dialog from chrome.exe; as it is currently though, we will wait for the 20s timeout again when trying to rdv a second time in process_singleton, although we "know" from chrome.exe that the window is hung... could we signal that to process_singleton from chrome.exe somehow?)). https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:304: if (!::IsWindow(remote_window_)) { You can also move this block in chrome::AttemptToNotifyRunningChrome().
GYP review https://codereview.chromium.org/14617003/diff/55001/chrome/browser/metro_util... File chrome/browser/metro_utils/metro_utils.gypi (right): https://codereview.chromium.org/14617003/diff/55001/chrome/browser/metro_util... chrome/browser/metro_utils/metro_utils.gypi:1: { New files should get the boilerplate garbage. https://codereview.chromium.org/14617003/diff/55001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/14617003/diff/55001/chrome/chrome.gyp#newcode... chrome/chrome.gyp:1055: 'browser/metro_utils/metro_utils.gypi', Included gypi files should be in the same directory as the gyp that includes them. Otherwise things get very confusing. Once you move the gypi to this directory, you can expunge <(DEPTH) from it. https://codereview.chromium.org/14617003/diff/55001/chrome/chrome_process_fin... File chrome/chrome_process_finder.gypi (right): https://codereview.chromium.org/14617003/diff/55001/chrome/chrome_process_fin... chrome/chrome_process_finder.gypi:1: { Boilerplate.
https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/31001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:29: // fast-rendezvous (see https://codereview.chromium.org/14617003/). On 2013/05/15 13:28:03, gab wrote: > On 2013/05/14 08:39:54, koz wrote: > > On 2013/05/13 13:27:43, gab wrote: > > > I haven't followed the discussion with regards to restructuring includes and > > > dependencies, but can't you move this to an acceptable common-base target > (or > > > simply make a mini-target like you did for policy_path_parser)? > > > > It should be possible to move net/base/escape* to base/. How about we have > > chrome.exe depend on net (it doesn't bloat the binary due to link-time > pruning) > > for now and I'll put a TODO to move net/base/escape* into base? > > I'm not an expert with dependencies and link time optimizations, but if you can > confirm this doesn't bloat chrome.exe (and others agree with the dependency), > that sgtm. As the other reviewers have expressed a dislike of depending on net above, are you okay with this copied code being here instead for the time being? As the code will be moved in a follow-up CL I think it's probably moot which way we go here. https://codereview.chromium.org/14617003/diff/55001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/55001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:76: bool AttemptFastNotify(CommandLine* command_line) { On 2013/05/15 13:28:03, gab wrote: > nit: This can also take a const& Done. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/55001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:117: LOG(WARNING) << "In metro mode, but and launch is " << launch; On 2013/05/15 13:28:03, gab wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:145: file_util::GetCurrentDirectory(&cur_dir); On 2013/05/15 13:28:03, gab wrote: > return false if this fails. Done. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/metro_util... File chrome/browser/metro_utils/metro_utils.gypi (right): https://codereview.chromium.org/14617003/diff/55001/chrome/browser/metro_util... chrome/browser/metro_utils/metro_utils.gypi:1: { On 2013/05/15 13:37:06, Mark Mentovai wrote: > New files should get the boilerplate garbage. Done. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:290: DWORD process_id = 0; On 2013/05/15 13:28:03, gab wrote: > This code is already in AttemptToNotifyRunningChrome(). These variables are used further down. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:301: remote_window_ = NULL; On 2013/05/15 13:28:03, gab wrote: > Some of these cases should return PROCESS_NONE here to keep the same behavior. > > It's probably simpler to just move the enum and have > chrome::AttemptToNotifyRunningChrome return an enum (e.g., something like: > PROCESS_NOTIFY_SUCCESS (success cases -- we should return early), > PROCESS_NOTIFY_FAILED (failed cases -- we should launch), and > PROCESS_NOTIFY_WINDOW_HUNG (when we should keep going below -- or, in the > chrome.exe case, abandon the early rendez-vous (since we can't pop a dialog from > chrome.exe; as it is currently though, we will wait for the 20s timeout again > when trying to rdv a second time in process_singleton, although we "know" from > chrome.exe that the window is hung... could we signal that to process_singleton > from chrome.exe somehow?)). Done. I don't know if it's worth trying to signal to chrome.dll that the window has hung. https://codereview.chromium.org/14617003/diff/55001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:304: if (!::IsWindow(remote_window_)) { On 2013/05/15 13:28:03, gab wrote: > You can also move this block in chrome::AttemptToNotifyRunningChrome(). Done. https://codereview.chromium.org/14617003/diff/55001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/14617003/diff/55001/chrome/chrome.gyp#newcode... chrome/chrome.gyp:1055: 'browser/metro_utils/metro_utils.gypi', On 2013/05/15 13:37:06, Mark Mentovai wrote: > Included gypi files should be in the same directory as the gyp that includes > them. Otherwise things get very confusing. > > Once you move the gypi to this directory, you can expunge <(DEPTH) from it. Done. https://codereview.chromium.org/14617003/diff/55001/chrome/chrome_process_fin... File chrome/chrome_process_finder.gypi (right): https://codereview.chromium.org/14617003/diff/55001/chrome/chrome_process_fin... chrome/chrome_process_finder.gypi:1: { On 2013/05/15 13:37:06, Mark Mentovai wrote: > Boilerplate. Done.
final nits from me, i think. https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:183: if (!::IsWindow(remote_window)) { nit: no braces for single-line conditional and body https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.h:21: nit: remove extra blank line https://codereview.chromium.org/14617003/diff/63001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/14617003/diff/63001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:3201: 'chrome_process_finder.gypi', since the target defined in chrome_process_finder.gypi is used both by chrome_browser.gypi and chrome_exe.gypi, i think the actual inclusion should take place up in chrome.gyp.
https://codereview.chromium.org/14617003/diff/63001/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/63001/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:98: return 0; Not sure if we should call AttemptFastNotify before we know we are a metro process, I am guessing this will break aura. let me check on monday what would be the outcome in aura.
https://codereview.chromium.org/14617003/diff/78001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/78001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:186: return NOTIFY_WINDOW_HUNG; NOTIFY_SUCCESS here?
https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:183: if (!::IsWindow(remote_window)) { On 2013/05/17 03:30:58, grt wrote: > nit: no braces for single-line conditional and body Done. https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/14617003/diff/63001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.h:21: On 2013/05/17 03:30:58, grt wrote: > nit: remove extra blank line Done. https://codereview.chromium.org/14617003/diff/63001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/14617003/diff/63001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:3201: 'chrome_process_finder.gypi', On 2013/05/17 03:30:58, grt wrote: > since the target defined in chrome_process_finder.gypi is used both by > chrome_browser.gypi and chrome_exe.gypi, i think the actual inclusion should > take place up in chrome.gyp. Done. https://codereview.chromium.org/14617003/diff/78001/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/78001/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:186: return NOTIFY_WINDOW_HUNG; On 2013/05/20 20:11:26, Shrikant Kelkar wrote: > NOTIFY_SUCCESS here? Actually NOTIFY_WINDOW_HUNG is correct here, but I accidentally dropped the NOTIFY_SUCCESS from the if (SendMessageTimeout()) {...}, cheers.
Process singleton stuff lgtm (I'll let other reviewers review dependencies/gyp changes) w/ comments below. Cheers, Gab https://codereview.chromium.org/14617003/diff/83018/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/83018/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:87: (chrome::AttemptToNotifyRunningChrome(chrome) == chrome::NOTIFY_SUCCESS); nit: I don't think this needs extra brackets around the equality check https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:178: if (result) optional nit: I find using the ternary operator cleaner here, e.g.: return result ? NOTIFY_SUCCESS : NOTIFY_FAILED; https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:187: return NOTIFY_WINDOW_HUNG; Add a comment like: // If the window couldn't be notified, but still exists, assume it is hung. https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:297: DWORD thread_id = ::GetWindowThreadProcessId(remote_window_, &process_id); This variable is now only needed in the NOTIFY_WINDOW_HUNG case, move it after the switch statement. https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:325: chrome::MESSAGE_BOX_TYPE_QUESTION) == chrome::MESSAGE_BOX_RESULT_NO) { optional: While you're here would you mind fixing this wrapping/indent, I suggest something like: if (visible_window && chrome::ShowMessageBox( NULL, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME), l10n_util::GetStringUTF16(IDS_BROWSER_HUNGBROWSER_MESSAGE), chrome::MESSAGE_BOX_TYPE_QUESTION) == chrome::MESSAGE_BOX_RESULT_NO) {
gyp* and other bits i looked at lgtm. mark@: lgty?
it looks ok to me but I defer to Shrikant for final thoughts.
https://codereview.chromium.org/14617003/diff/83018/chrome/app/chrome_exe_mai... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/14617003/diff/83018/chrome/app/chrome_exe_mai... chrome/app/chrome_exe_main_win.cc:87: (chrome::AttemptToNotifyRunningChrome(chrome) == chrome::NOTIFY_SUCCESS); On 2013/05/21 19:48:40, gab wrote: > nit: I don't think this needs extra brackets around the equality check Done. https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:178: if (result) On 2013/05/21 19:48:40, gab wrote: > optional nit: I find using the ternary operator cleaner here, e.g.: > > return result ? NOTIFY_SUCCESS : NOTIFY_FAILED; Done. https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... chrome/browser/chrome_process_finder_win.cc:187: return NOTIFY_WINDOW_HUNG; On 2013/05/21 19:48:40, gab wrote: > Add a comment like: > > // If the window couldn't be notified, but still exists, assume it is hung. Done. https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:297: DWORD thread_id = ::GetWindowThreadProcessId(remote_window_, &process_id); On 2013/05/21 19:48:40, gab wrote: > This variable is now only needed in the NOTIFY_WINDOW_HUNG case, move it after > the switch statement. Done, and removed the 0 checks below as they occur in AttemptToNotifyRunningChrome(). https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:325: chrome::MESSAGE_BOX_TYPE_QUESTION) == chrome::MESSAGE_BOX_RESULT_NO) { On 2013/05/21 19:48:40, gab wrote: > optional: While you're here would you mind fixing this wrapping/indent, I > suggest something like: > if (visible_window && > chrome::ShowMessageBox( > NULL, > l10n_util::GetStringUTF16(IDS_PRODUCT_NAME), > l10n_util::GetStringUTF16(IDS_BROWSER_HUNGBROWSER_MESSAGE), > chrome::MESSAGE_BOX_TYPE_QUESTION) == chrome::MESSAGE_BOX_RESULT_NO) { Done.
Hmm, did you forget to upload? The latest patch set is still the one I commented on 2 days ago :). Cheers, Gab On 2013/05/23 06:11:48, koz wrote: > https://codereview.chromium.org/14617003/diff/83018/chrome/app/chrome_exe_mai... > File chrome/app/chrome_exe_main_win.cc (right): > > https://codereview.chromium.org/14617003/diff/83018/chrome/app/chrome_exe_mai... > chrome/app/chrome_exe_main_win.cc:87: > (chrome::AttemptToNotifyRunningChrome(chrome) == chrome::NOTIFY_SUCCESS); > On 2013/05/21 19:48:40, gab wrote: > > nit: I don't think this needs extra brackets around the equality check > > Done. > > https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... > File chrome/browser/chrome_process_finder_win.cc (right): > > https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... > chrome/browser/chrome_process_finder_win.cc:178: if (result) > On 2013/05/21 19:48:40, gab wrote: > > optional nit: I find using the ternary operator cleaner here, e.g.: > > > > return result ? NOTIFY_SUCCESS : NOTIFY_FAILED; > > Done. > > https://codereview.chromium.org/14617003/diff/83018/chrome/browser/chrome_pro... > chrome/browser/chrome_process_finder_win.cc:187: return NOTIFY_WINDOW_HUNG; > On 2013/05/21 19:48:40, gab wrote: > > Add a comment like: > > > > // If the window couldn't be notified, but still exists, assume it is hung. > > Done. > > https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... > File chrome/browser/process_singleton_win.cc (right): > > https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... > chrome/browser/process_singleton_win.cc:297: DWORD thread_id = > ::GetWindowThreadProcessId(remote_window_, &process_id); > On 2013/05/21 19:48:40, gab wrote: > > This variable is now only needed in the NOTIFY_WINDOW_HUNG case, move it after > > the switch statement. > > Done, and removed the 0 checks below as they occur in > AttemptToNotifyRunningChrome(). > > https://codereview.chromium.org/14617003/diff/83018/chrome/browser/process_si... > chrome/browser/process_singleton_win.cc:325: chrome::MESSAGE_BOX_TYPE_QUESTION) > == chrome::MESSAGE_BOX_RESULT_NO) { > On 2013/05/21 19:48:40, gab wrote: > > optional: While you're here would you mind fixing this wrapping/indent, I > > suggest something like: > > if (visible_window && > > chrome::ShowMessageBox( > > NULL, > > l10n_util::GetStringUTF16(IDS_PRODUCT_NAME), > > l10n_util::GetStringUTF16(IDS_BROWSER_HUNGBROWSER_MESSAGE), > > chrome::MESSAGE_BOX_TYPE_QUESTION) == chrome::MESSAGE_BOX_RESULT_NO) { > > Done.
lgtm For Metro activation issues.
@gab yes, I did forget to upload. Thanks for the reminder :-) @cpu, LGTY? +sky for owners of pretty much everything
Thanks, one last nit from me. Note: You can upload with --similarity=90 or something like that so that it stops thinking the new gypi files are modified copies of the auto_login_parser gypi. Cheers, Gab https://codereview.chromium.org/14617003/diff/108001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/108001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:302: DWORD process_id = 0; Please add a comment here explaining why you're skipping the 0 checks.
LGTM https://codereview.chromium.org/14617003/diff/108001/chrome/browser/chrome_pr... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/14617003/diff/108001/chrome/browser/chrome_pr... chrome/browser/chrome_process_finder_win.h:16: enum NotifyResult { This name is too generic for the chrome namespace. Go with something more specific, maybe NotifyChromeResult.
Great! Thanks all for the assistance in getting this ready to land. https://codereview.chromium.org/14617003/diff/108001/chrome/browser/chrome_pr... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/14617003/diff/108001/chrome/browser/chrome_pr... chrome/browser/chrome_process_finder_win.h:16: enum NotifyResult { On 2013/05/24 16:13:31, sky wrote: > This name is too generic for the chrome namespace. Go with something more > specific, maybe NotifyChromeResult. Done. https://codereview.chromium.org/14617003/diff/108001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/108001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:302: DWORD process_id = 0; On 2013/05/24 12:22:22, gab wrote: > Please add a comment here explaining why you're skipping the 0 checks. Done.
https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:303: // running, so no need to check again here. Actually, I disagree that it's fine to not check for 0 for the reason stated above. Since AttemptToNotifyRunningChrome() can pause for a while (in particular if the window is hung) while trying to send the message, even if the process/thread for remote_window_ existed before, it doesn't mean they still exist (in fact, even without a long wait this can also be true since there is no concurrency mechanism between this process and the other Chrome process; i.e., this is inherently racy). It is technically fine to not check for 0 since if |thread_id| is 0, EnumThreadWindows will return false and KillProcessById will fail (but we don't check that it succeeded so that's "ok"); however it feels weird to assume the rest of the method will do the right thing, I'd prefer checking for 0 again I think, it makes the code more explicit and isn't very costly at all (it doesn't completely prevent a race however since the process/thread could be killed immediately after the 0 check, but at least we don't explicitly call into the methods below with 0s).
https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:303: // running, so no need to check again here. On 2013/05/27 13:20:19, gab wrote: > Actually, I disagree that it's fine to not check for 0 for the reason stated > above. > > Since AttemptToNotifyRunningChrome() can pause for a while (in particular if the > window is hung) while trying to send the message, even if the process/thread for > remote_window_ existed before, it doesn't mean they still exist (in fact, even > without a long wait this can also be true since there is no concurrency > mechanism between this process and the other Chrome process; i.e., this is > inherently racy). > > It is technically fine to not check for 0 since if |thread_id| is 0, > EnumThreadWindows will return false and KillProcessById will fail (but we don't > check that it succeeded so that's "ok"); however it feels weird to assume the > rest of the method will do the right thing, I'd prefer checking for 0 again I > think, it makes the code more explicit and isn't very costly at all (it doesn't > completely prevent a race however since the process/thread could be killed > immediately after the 0 check, but at least we don't explicitly call into the > methods below with 0s). Done.
https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:303: // running, so no need to check again here. On 2013/05/28 00:54:01, koz wrote: > On 2013/05/27 13:20:19, gab wrote: > > Actually, I disagree that it's fine to not check for 0 for the reason stated > > above. > > > > Since AttemptToNotifyRunningChrome() can pause for a while (in particular if > the > > window is hung) while trying to send the message, even if the process/thread > for > > remote_window_ existed before, it doesn't mean they still exist (in fact, even > > without a long wait this can also be true since there is no concurrency > > mechanism between this process and the other Chrome process; i.e., this is > > inherently racy). > > > > It is technically fine to not check for 0 since if |thread_id| is 0, > > EnumThreadWindows will return false and KillProcessById will fail (but we > don't > > check that it succeeded so that's "ok"); however it feels weird to assume the > > rest of the method will do the right thing, I'd prefer checking for 0 again I > > think, it makes the code more explicit and isn't very costly at all (it > doesn't > > completely prevent a race however since the process/thread could be killed > > immediately after the 0 check, but at least we don't explicitly call into the > > methods below with 0s). > > Done. Thanks, you can remove this comment now.
https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/14617003/diff/116001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:303: // running, so no need to check again here. On 2013/05/28 13:55:34, gab wrote: > On 2013/05/28 00:54:01, koz wrote: > > On 2013/05/27 13:20:19, gab wrote: > > > Actually, I disagree that it's fine to not check for 0 for the reason stated > > > above. > > > > > > Since AttemptToNotifyRunningChrome() can pause for a while (in particular if > > the > > > window is hung) while trying to send the message, even if the process/thread > > for > > > remote_window_ existed before, it doesn't mean they still exist (in fact, > even > > > without a long wait this can also be true since there is no concurrency > > > mechanism between this process and the other Chrome process; i.e., this is > > > inherently racy). > > > > > > It is technically fine to not check for 0 since if |thread_id| is 0, > > > EnumThreadWindows will return false and KillProcessById will fail (but we > > don't > > > check that it succeeded so that's "ok"); however it feels weird to assume > the > > > rest of the method will do the right thing, I'd prefer checking for 0 again > I > > > think, it makes the code more explicit and isn't very costly at all (it > > doesn't > > > completely prevent a race however since the process/thread could be killed > > > immediately after the 0 check, but at least we don't explicitly call into > the > > > methods below with 0s). > > > > Done. > > Thanks, you can remove this comment now. Oops, trying to do too many things at once :-P
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/14617003/147001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/14617003/147001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+brettw for base/ owners
Thanks, lgtm++
lgtm
TBRing brettw as the owners error is triggered because I have +base in a DEPS file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/14617003/172001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Message was sent while issue was closed.
Committed patchset #19 manually as r204099 (presubmit successful). |
