Rewrite of chrome.exe startup code...

Rewrite of chrome.exe startup code

A lot of cruft and repeated code has deposited over the years on chrome's initialization code.
This CL makes it all much more clear and straightforward. There is no fundamental change of
behavior except the order of certain things is different but it should not alter the observed
operation.

- chrome's and chromium load is fundamentally the same but most of the code was repeated
- chrome's way to load the dll was incorrect: using a relative path with LOAD_WITH_ALTERED_SEARCH_PATH
- Use of SearchPath() was dangerous and not needed
- removed google_update_client.cc and .h
- removed bunch of #ifdefs

TEST=all convered by UI tests already except [1]
BUG=none

[1] The only thing I don't see convered by test is the restart dialog ('woa! chrome crashed').



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30934

[cpu_(ooo_6.6-7.5), 2009-11-02 19:42:34]

sorry for the amount of changes. At least the rewrite of the crash handling is
not here. :)

[Nicolas Sylvain, 2009-11-02 21:15:58]

Overall it looks great. Some questions below

the only thing I don't really like is that LoadChromeWithDirectory modifies the
dir to be a file path. this is weird. That wont block the lgtm though.

http://codereview.chromium.org/345036/diff/2001/2002
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/345036/diff/2001/2002#newcode21
Line 21: BYTE v[128 * sizeof(wchar_t)];
why not wchar_t[128] and cast is below? It would make more sense while reading
the code.

"v" does not mean much, what about "value" ?

http://codereview.chromium.org/345036/diff/2001/2002#newcode40
Line 40: new_chrome_exe.append(installer_util::kChromeNewExe);
i guess that's for the auto-update case? Can you add a comment here so we
understand why you would use the new chrome exe and the old version field.

http://codereview.chromium.org/345036/diff/2001/2002#newcode46
Line 46: bool ret = ReadRegistryStr(key, google_update::kRegVersionField,
version);
i would add an empty line about "bool" to separate the blocks

http://codereview.chromium.org/345036/diff/2001/2002#newcode79
Line 79: dir->append(installer_util::kChromeDll);
are you sure you really mean to modify the input param here? 
<edit> I see that you did. It would be nice to make it more explicit. calling it
"dir" might not be the best idea, but i would settle for a comment.

http://codereview.chromium.org/345036/diff/2001/2002#newcode124
Line 124: HMODULE dll = LoadChromeWithDirectory(file);
is it changing the logic here?

I thought we had this problem before, like you have chrome.dll on your desktop,
and you have your chrome link on your desktop, and when you click it, it starts
using the chrome.dll on your desktop instead of taking the one in the current
directory.  Maybe LOAD_WITH_ALTERED_SEARCH_PATH fixes that?

http://codereview.chromium.org/345036/diff/2001/2002#newcode189
Line 189: virtual void OnBeforeLaunch(const std::wstring& version) {
you don't want to make it do nothing for the parent class instead of defining an
empty declaration here?

http://codereview.chromium.org/345036/diff/2001/2004
File chrome/app/client_util.h (right):

http://codereview.chromium.org/345036/diff/2001/2004#newcode21
Line 21: class MainDllLoader {
should we not have a main_dll_loader.h ?

[cpu_(ooo_6.6-7.5), 2009-11-03 02:40:16]

code updated please look again.

http://codereview.chromium.org/345036/diff/2001/2002
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/345036/diff/2001/2002#newcode21
Line 21: BYTE v[128 * sizeof(wchar_t)];
On 2009/11/02 21:15:58, Nicolas Sylvain wrote:
> why not wchar_t[128] and cast is below? It would make more sense while reading
> the code.

Did it for the stupid 80 char limit. I feel it reads well :)

> 
> "v" does not mean much, what about "value" ?

value is taken on that function :) how about 'out' ? it maches EnvQueryStr
usage.

http://codereview.chromium.org/345036/diff/2001/2002#newcode40
Line 40: new_chrome_exe.append(installer_util::kChromeNewExe);
On 2009/11/02 21:15:58, Nicolas Sylvain wrote:
> i guess that's for the auto-update case? Can you add a comment here so we
> understand why you would use the new chrome exe and the old version field.

Done.

http://codereview.chromium.org/345036/diff/2001/2002#newcode46
Line 46: bool ret = ReadRegistryStr(key, google_update::kRegVersionField,
version);
On 2009/11/02 21:15:58, Nicolas Sylvain wrote:
> i would add an empty line about "bool" to separate the blocks

Done.

http://codereview.chromium.org/345036/diff/2001/2002#newcode79
Line 79: dir->append(installer_util::kChromeDll);
On 2009/11/02 21:15:58, Nicolas Sylvain wrote:
> are you sure you really mean to modify the input param here? 

It is a bit strange but saves me from keeping a path around. Added comments.

> <edit> I see that you did. It would be nice to make it more explicit. calling
it
> "dir" might not be the best idea, but i would settle for a comment.

http://codereview.chromium.org/345036/diff/2001/2002#newcode124
Line 124: HMODULE dll = LoadChromeWithDirectory(file);
On 2009/11/02 21:15:58, Nicolas Sylvain wrote:
> is it changing the logic here?

Yes, the previous logic did not made any sense. We used a relative path with
LOAD_WITH_ALTERED_SEARCH_PATH which according to msdn makes no sense.

> 
> I thought we had this problem before, like you have chrome.dll on your
desktop,
> and you have your chrome link on your desktop, and when you click it, it
starts
> using the chrome.dll on your desktop instead of taking the one in the current
> directory.  Maybe LOAD_WITH_ALTERED_SEARCH_PATH fixes that?

I will test that scenario and let you know.

http://codereview.chromium.org/345036/diff/2001/2004
File chrome/app/client_util.h (right):

http://codereview.chromium.org/345036/diff/2001/2004#newcode21
Line 21: class MainDllLoader {
If you feel strongly I'll do it.

On 2009/11/02 21:15:58, Nicolas Sylvain wrote:
> should we not have a main_dll_loader.h ?

[kuchhal, 2009-11-03 18:21:49]

http://codereview.chromium.org/345036/diff/4001/4002
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/345036/diff/4001/4002#newcode24
Line 24: return false;
Probably should do a sanity check on size too and return false if that fails.

http://codereview.chromium.org/345036/diff/4001/4002#newcode45
Line 45: if (::PathFileExistsW(new_chrome_exe.c_str()) &&
chrome.exe seems to depend on base project now. I wonder why we are not using
registry and file utility methods from base.

http://codereview.chromium.org/345036/diff/4001/4002#newcode59
Line 59: ::GetModuleFileNameW(NULL, path, MAX_PATH);
May be you can use CommandLine::ForCurrentProcess here.

http://codereview.chromium.org/345036/diff/4001/4002#newcode66
Line 66: bool EnvQueryStr(const wchar_t* key_name, std::wstring* value) {
This method probably should force the 128 size limitation as well and can be
simplified.

http://codereview.chromium.org/345036/diff/4001/4002#newcode69
Line 69: return false;
no need for braces

http://codereview.chromium.org/345036/diff/4001/4002#newcode74
Line 74: return true;
shouldn't this be return false and return true on line 78?

http://codereview.chromium.org/345036/diff/4001/4002#newcode148
Line 148: // environment variable and just calling the entry point. Dervived
classes
Derived

http://codereview.chromium.org/345036/diff/4001/4004
File chrome/app/client_util.h (right):

http://codereview.chromium.org/345036/diff/4001/4004#newcode25
Line 25: // Loads and calls the entry point of chrome.dll. |instance| is the exe
Empty line above this line and other places in this file.

[cpu_(ooo_6.6-7.5), 2009-11-03 22:51:32]

code updated, please look again.

http://codereview.chromium.org/345036/diff/4001/4002
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/345036/diff/4001/4002#newcode24
Line 24: return false;
On 2009/11/03 18:21:50, kuchhal wrote:
> Probably should do a sanity check on size too and return false if that fails.

Added checking on type. size issues should return ERROR_MORE_DATA

http://codereview.chromium.org/345036/diff/4001/4002#newcode45
Line 45: if (::PathFileExistsW(new_chrome_exe.c_str()) &&
On 2009/11/03 18:21:50, kuchhal wrote:
> chrome.exe seems to depend on base project now. I wonder why we are not using
> registry and file utility methods from base.

Yeah, I considered that and even started, I want to first evaluate the size
reduction if we go the other way, no dependency with base. I can't see why
chrome.exe official build is 897Kb. I think it can be made ~300KB. Also remove
CommandLine.

http://codereview.chromium.org/345036/diff/4001/4002#newcode66
Line 66: bool EnvQueryStr(const wchar_t* key_name, std::wstring* value) {
On 2009/11/03 18:21:50, kuchhal wrote:
> This method probably should force the 128 size limitation as well and can be
> simplified.

Done.

http://codereview.chromium.org/345036/diff/4001/4002#newcode69
Line 69: return false;
On 2009/11/03 18:21:50, kuchhal wrote:
> no need for braces

Done.

http://codereview.chromium.org/345036/diff/4001/4002#newcode74
Line 74: return true;
On 2009/11/03 18:21:50, kuchhal wrote:
> shouldn't this be return false and return true on line 78? 

Done.

http://codereview.chromium.org/345036/diff/4001/4002#newcode148
Line 148: // environment variable and just calling the entry point. Dervived
classes
On 2009/11/03 18:21:50, kuchhal wrote:
> Derived

Done.

[kuchhal, 2009-11-03 23:33:20]

lgtm.

On Tue, Nov 3, 2009 at 2:51 PM, <cpu@chromium.org> wrote:

> code updated, please look again.
>
>
>
> http://codereview.chromium.org/345036/diff/4001/4002
> File chrome/app/client_util.cc (right):
>
> http://codereview.chromium.org/345036/diff/4001/4002#newcode24
> Line 24: return false;
> On 2009/11/03 18:21:50, kuchhal wrote:
>
>> Probably should do a sanity check on size too and return false if that
>>
> fails.
>
> Added checking on type. size issues should return ERROR_MORE_DATA
>
>
> http://codereview.chromium.org/345036/diff/4001/4002#newcode45
> Line 45: if (::PathFileExistsW(new_chrome_exe.c_str()) &&
> On 2009/11/03 18:21:50, kuchhal wrote:
>
>> chrome.exe seems to depend on base project now. I wonder why we are
>>
> not using
>
>> registry and file utility methods from base.
>>
>
> Yeah, I considered that and even started, I want to first evaluate the
> size reduction if we go the other way, no dependency with base. I can't
> see why chrome.exe official build is 897Kb. I think it can be made
> ~300KB. Also remove CommandLine.
>
>
> http://codereview.chromium.org/345036/diff/4001/4002#newcode66
> Line 66: bool EnvQueryStr(const wchar_t* key_name, std::wstring* value)
> {
> On 2009/11/03 18:21:50, kuchhal wrote:
>
>> This method probably should force the 128 size limitation as well and
>>
> can be
>
>> simplified.
>>
>
> Done.
>
>
> http://codereview.chromium.org/345036/diff/4001/4002#newcode69
> Line 69: return false;
> On 2009/11/03 18:21:50, kuchhal wrote:
>
>> no need for braces
>>
>
> Done.
>
>
> http://codereview.chromium.org/345036/diff/4001/4002#newcode74
> Line 74: return true;
> On 2009/11/03 18:21:50, kuchhal wrote:
>
>> shouldn't this be return false and return true on line 78?
>>
>
> Done.
>
>
> http://codereview.chromium.org/345036/diff/4001/4002#newcode148
> Line 148: // environment variable and just calling the entry point.
> Dervived classes
> On 2009/11/03 18:21:50, kuchhal wrote:
>
>> Derived
>>
>
> Done.
>
>
> http://codereview.chromium.org/345036
>

[Nicolas Sylvain, 2009-11-03 23:35:54]

LGTM, make sure to watch out for startup regression and size regression.