Add a helper process to Chrome Frame to allow for non-administrative installs...

Add a helper process to Chrome Frame to allow for non-administrative installs. The helper process registers a hook dll that performs the necessary BHO injection instead of registering it in HKLM.

BUG=53127
TEST=Non-admin CF installs work.


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

[tommi (sloooow) - chröme, 2010-08-26 19:52:51]

http://codereview.chromium.org/3158036/diff/1/2
File chrome_frame/bho_loader.cc (right):

http://codereview.chromium.org/3158036/diff/1/2#newcode19
chrome_frame/bho_loader.cc:19: hooker_ = new EventHooker();
put the hooker in an initializer list?

http://codereview.chromium.org/3158036/diff/1/2#newcode52
chrome_frame/bho_loader.cc:52: IObjectWithSite* bho_object = NULL;
not using CComPtr?
if not, then you might want to look at using ScopedComPtr, I don't think you'd
need to link against base for that.

http://codereview.chromium.org/3158036/diff/1/3
File chrome_frame/bho_loader.h (right):

http://codereview.chromium.org/3158036/diff/1/3#newcode11
chrome_frame/bho_loader.h:11: class EventHooker;
:-|  a forward event hooker, oh my

http://codereview.chromium.org/3158036/diff/1/6
File chrome_frame/chrome_frame_helper.rc (right):

http://codereview.chromium.org/3158036/diff/1/6#newcode57
chrome_frame/chrome_frame_helper.rc:57: 1 TYPELIB "chrome_tab.tlb"
tlb?

http://codereview.chromium.org/3158036/diff/1/7
File chrome_frame/chrome_frame_helper_dll.cc (right):

http://codereview.chromium.org/3158036/diff/1/7#newcode26
chrome_frame/chrome_frame_helper_dll.cc:26: return TRUE;
disable thread notifications?

http://codereview.chromium.org/3158036/diff/1/9
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/3158036/diff/1/9#newcode37
chrome_frame/chrome_frame_helper_main.cc:37: dll_ =
LoadLibrary(L"chrome_frame_helper_dll.dll");
redundancy?
(_dll.dll)

http://codereview.chromium.org/3158036/diff/1/10
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/3158036/diff/1/10#newcode48
chrome_frame/chrome_frame_helper_util.cc:48: static const ULONG
get_browser_message =
not sure, but it feels like this should be kGetBrowserMessage

http://codereview.chromium.org/3158036/diff/1/10#newcode70
chrome_frame/chrome_frame_helper_util.cc:70: DWORD dwRetSize =
GetClassNameW(window_to_check, &buffer[0], buf_size);
ret_size (no hungarian).  Also, &buffer[0] and buffer are equal...
overcomplicated?

[robertshield, 2010-08-26 20:41:00]

Thanks, please take another look.

http://codereview.chromium.org/3158036/diff/1/2
File chrome_frame/bho_loader.cc (right):

http://codereview.chromium.org/3158036/diff/1/2#newcode19
chrome_frame/bho_loader.cc:19: hooker_ = new EventHooker();
On 2010/08/26 19:52:51, tommi wrote:
> put the hooker in an initializer list?

Done.

http://codereview.chromium.org/3158036/diff/1/2#newcode52
chrome_frame/bho_loader.cc:52: IObjectWithSite* bho_object = NULL;
On 2010/08/26 19:52:51, tommi wrote:
> not using CComPtr?
> if not, then you might want to look at using ScopedComPtr, I don't think you'd
> need to link against base for that.

Went back to using CComPtr. Experimented with removing ATL altogether, and saw
negligible size savings.

http://codereview.chromium.org/3158036/diff/1/3
File chrome_frame/bho_loader.h (right):

http://codereview.chromium.org/3158036/diff/1/3#newcode11
chrome_frame/bho_loader.h:11: class EventHooker;
On 2010/08/26 19:52:51, tommi wrote:
> :-|  a forward event hooker, oh my

Done.

http://codereview.chromium.org/3158036/diff/1/6
File chrome_frame/chrome_frame_helper.rc (right):

http://codereview.chromium.org/3158036/diff/1/6#newcode57
chrome_frame/chrome_frame_helper.rc:57: 1 TYPELIB "chrome_tab.tlb"
On 2010/08/26 19:52:51, tommi wrote:
> tlb?

Removed.

http://codereview.chromium.org/3158036/diff/1/7
File chrome_frame/chrome_frame_helper_dll.cc (right):

http://codereview.chromium.org/3158036/diff/1/7#newcode26
chrome_frame/chrome_frame_helper_dll.cc:26: return TRUE;
On 2010/08/26 19:52:51, tommi wrote:
> disable thread notifications?

Can't while linking to the CRT.

http://codereview.chromium.org/3158036/diff/1/9
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/3158036/diff/1/9#newcode37
chrome_frame/chrome_frame_helper_main.cc:37: dll_ =
LoadLibrary(L"chrome_frame_helper_dll.dll");
On 2010/08/26 19:52:51, tommi wrote:
> redundancy?
> (_dll.dll)

Who are you calling redundant and needlessly repetitive? Renamed.

http://codereview.chromium.org/3158036/diff/1/10
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/3158036/diff/1/10#newcode48
chrome_frame/chrome_frame_helper_util.cc:48: static const ULONG
get_browser_message =
On 2010/08/26 19:52:51, tommi wrote:
> not sure, but it feels like this should be kGetBrowserMessage

ok.

http://codereview.chromium.org/3158036/diff/1/10#newcode70
chrome_frame/chrome_frame_helper_util.cc:70: DWORD dwRetSize =
GetClassNameW(window_to_check, &buffer[0], buf_size);
On 2010/08/26 19:52:51, tommi wrote:
> ret_size (no hungarian).  Also, &buffer[0] and buffer are equal...
> overcomplicated?

Ah, yes, 'twas a std::string once upon a time, but no more. Fixed.

[tommi (sloooow) - chröme, 2010-08-26 20:45:26]

lgtm

http://codereview.chromium.org/3158036/diff/15001/16006
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/3158036/diff/15001/16006#newcode116
chrome_frame/chrome_frame_helper_main.cc:116: while (GetMessage(&msg, NULL, 0,
0)) {
fyi - GetMessage can return -1, so MS recommends something like this:

while( (bRet = GetMessage( &msg, hWnd, 0, 0 )) != 0)
{ 
    if (bRet == -1)
    {
        // handle the error and possibly exit
    }
    else
    {
        TranslateMessage(&msg); 
        DispatchMessage(&msg); 
    }
}

[amit, 2010-08-26 21:48:13]

Looks very nice and simple.  Now given that this is a single critical point of
failure, I think having breakpad integration for crash dumps is important.

http://codereview.chromium.org/3158036/diff/15001/16006
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/3158036/diff/15001/16006#newcode102
chrome_frame/chrome_frame_helper_main.cc:102: int APIENTRY wWinMain(HINSTANCE
hinstance, HINSTANCE, wchar_t*, int show_cmd) {
First thing we should do is register with breakpad?

http://codereview.chromium.org/3158036/diff/15001/16011
File chrome_frame/event_hooker.cc (right):

http://codereview.chromium.org/3158036/diff/15001/16011#newcode35
chrome_frame/event_hooker.cc:35: window_reparent_hook_ =
SetWinEventHook(EVENT_OBJECT_PARENTCHANGE,
do we need this?

[tommi (sloooow) - chröme, 2010-08-26 22:28:13]

I agree but maybe we could submit this as is and do breakpad next?

On Thu, Aug 26, 2010 at 5:48 PM, <amit@chromium.org> wrote:

> Looks very nice and simple.  Now given that this is a single critical point
> of
> failure, I think having breakpad integration for crash dumps is important.
>
>
>
> http://codereview.chromium.org/3158036/diff/15001/16006
> File chrome_frame/chrome_frame_helper_main.cc (right):
>
> http://codereview.chromium.org/3158036/diff/15001/16006#newcode102
> chrome_frame/chrome_frame_helper_main.cc:102: int APIENTRY
> wWinMain(HINSTANCE hinstance, HINSTANCE, wchar_t*, int show_cmd) {
> First thing we should do is register with breakpad?
>
> http://codereview.chromium.org/3158036/diff/15001/16011
> File chrome_frame/event_hooker.cc (right):
>
> http://codereview.chromium.org/3158036/diff/15001/16011#newcode35
> chrome_frame/event_hooker.cc:35: window_reparent_hook_ =
> SetWinEventHook(EVENT_OBJECT_PARENTCHANGE,
> do we need this?
>
>
> http://codereview.chromium.org/3158036/show
>

[amit, 2010-08-26 22:42:08]

On Thu, Aug 26, 2010 at 3:27 PM, Tommi <tommi@chromium.org> wrote:

> I agree but maybe we could submit this as is and do breakpad next?

That's fine as long as it's  done before we enable it on the dev channel
build :)


>
>
> On Thu, Aug 26, 2010 at 5:48 PM, <amit@chromium.org> wrote:
>
>> Looks very nice and simple.  Now given that this is a single critical
>> point of
>> failure, I think having breakpad integration for crash dumps is important.
>>
>>
>>
>> http://codereview.chromium.org/3158036/diff/15001/16006
>> File chrome_frame/chrome_frame_helper_main.cc (right):
>>
>> http://codereview.chromium.org/3158036/diff/15001/16006#newcode102
>> chrome_frame/chrome_frame_helper_main.cc:102: int APIENTRY
>> wWinMain(HINSTANCE hinstance, HINSTANCE, wchar_t*, int show_cmd) {
>> First thing we should do is register with breakpad?
>>
>> http://codereview.chromium.org/3158036/diff/15001/16011
>> File chrome_frame/event_hooker.cc (right):
>>
>> http://codereview.chromium.org/3158036/diff/15001/16011#newcode35
>> chrome_frame/event_hooker.cc:35: window_reparent_hook_ =
>> SetWinEventHook(EVENT_OBJECT_PARENTCHANGE,
>> do we need this?
>>
>>
>> http://codereview.chromium.org/3158036/show
>>
>
>

[amit, 2010-08-26 22:42:51]

On Thu, Aug 26, 2010 at 3:42 PM, Amit Joshi <amit@chromium.org> wrote:

>
> On Thu, Aug 26, 2010 at 3:27 PM, Tommi <tommi@chromium.org> wrote:
>
>> I agree but maybe we could submit this as is and do breakpad next?
>
> That's fine as long as it's  done before we enable it on the dev channel
> build :)
>
In that case please consider this as LATM.  Looks Awesome To Me

>
>
>>
>>
>> On Thu, Aug 26, 2010 at 5:48 PM, <amit@chromium.org> wrote:
>>
>>> Looks very nice and simple.  Now given that this is a single critical
>>> point of
>>> failure, I think having breakpad integration for crash dumps is
>>> important.
>>>
>>>
>>>
>>> http://codereview.chromium.org/3158036/diff/15001/16006
>>> File chrome_frame/chrome_frame_helper_main.cc (right):
>>>
>>> http://codereview.chromium.org/3158036/diff/15001/16006#newcode102
>>> chrome_frame/chrome_frame_helper_main.cc:102: int APIENTRY
>>> wWinMain(HINSTANCE hinstance, HINSTANCE, wchar_t*, int show_cmd) {
>>> First thing we should do is register with breakpad?
>>>
>>> http://codereview.chromium.org/3158036/diff/15001/16011
>>> File chrome_frame/event_hooker.cc (right):
>>>
>>> http://codereview.chromium.org/3158036/diff/15001/16011#newcode35
>>> chrome_frame/event_hooker.cc:35: window_reparent_hook_ =
>>> SetWinEventHook(EVENT_OBJECT_PARENTCHANGE,
>>> do we need this?
>>>
>>>
>>> http://codereview.chromium.org/3158036/show
>>>
>>
>>
>

[robertshield, 2010-08-30 14:06:37]

Thanks!

http://codereview.chromium.org/3158036/diff/15001/16006
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/3158036/diff/15001/16006#newcode102
chrome_frame/chrome_frame_helper_main.cc:102: int APIENTRY wWinMain(HINSTANCE
hinstance, HINSTANCE, wchar_t*, int show_cmd) {
On 2010/08/26 21:48:13, amit wrote:
> First thing we should do is register with breakpad?

Added a todo, I'll add that in a quickly upcoming CL.

http://codereview.chromium.org/3158036/diff/15001/16006#newcode116
chrome_frame/chrome_frame_helper_main.cc:116: while (GetMessage(&msg, NULL, 0,
0)) {
On 2010/08/26 20:45:26, tommi wrote:
> fyi - GetMessage can return -1, so MS recommends something like this:
> 
> while( (bRet = GetMessage( &msg, hWnd, 0, 0 )) != 0)
> { 
>     if (bRet == -1)
>     {
>         // handle the error and possibly exit
>     }
>     else
>     {
>         TranslateMessage(&msg); 
>         DispatchMessage(&msg); 
>     }
> }

Done.

http://codereview.chromium.org/3158036/diff/15001/16011
File chrome_frame/event_hooker.cc (right):

http://codereview.chromium.org/3158036/diff/15001/16011#newcode35
chrome_frame/event_hooker.cc:35: window_reparent_hook_ =
SetWinEventHook(EVENT_OBJECT_PARENTCHANGE,
On 2010/08/26 21:48:13, amit wrote:
> do we need this? 

Previous readings suggested it was necessary, if looking for a specific window
hierarchy. Since we're looking for any window of a certain class, I can take
this out.