Change to a simpler upgrade scheme for Chrome Frame whereby we register a win...

Change to a simpler upgrade scheme for Chrome Frame whereby we register a window class in DllMain on attach and tag it with our module handle, then unregister said window class on detach.

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

[robertshield, 2010-05-07 20:29:12]

Here is the implementation of what we decided on doing. I have a todo to
rejigger the tests for this, I don't think I will have time to write those
today.

Note that I couldn't find a way to use the extra wndclass info without creating
a window.

[tommi (sloooow) - chröme, 2010-05-07 20:43:06]

http://codereview.chromium.org/1994007/diff/3002/11002
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/1994007/diff/3002/11002#newcode178
chrome_frame/chrome_tab.cc:178: HMODULE original_module =
DllRedirector::GetHModuleFromWndclass();
should we dcheck that this is never NULL?

http://codereview.chromium.org/1994007/diff/3002/11002#newcode184
chrome_frame/chrome_tab.cc:184:
DllRedirector::GetDllGetClassObjectPtr(original_module);
DCHECK that this is never NULL?

http://codereview.chromium.org/1994007/diff/3002/11003
File chrome_frame/module_utils.cc (right):

http://codereview.chromium.org/1994007/diff/3002/11003#newcode43
chrome_frame/module_utils.cc:43: if (hwnd) {
DCHECK IsWindow?

http://codereview.chromium.org/1994007/diff/3002/11003#newcode49
chrome_frame/module_utils.cc:49: if (!GetLastError()) {
prefer using ERROR_SUCCESS instead of !GetLastError() since GetLastError does
not return bool.

if (GetLastError() == ERROR_SUCCESS) {
}

http://codereview.chromium.org/1994007/diff/3002/11003#newcode70
chrome_frame/module_utils.cc:70: 0, 0, 0, NULL, NULL, NULL, NULL);
DCHECK IsWindow?

http://codereview.chromium.org/1994007/diff/3002/11004
File chrome_frame/module_utils.h (right):

http://codereview.chromium.org/1994007/diff/3002/11004#newcode17
chrome_frame/module_utils.h:17: static bool
DllRedirector::RegisterHModuleWndclass();
This is a total nit, but should we give the functions more logical names than
implementation specific ones?
E.g. RegisterAsFirstCfModule(), GetFirstCfModule(), etc?

[tommi (sloooow) - chröme, 2010-05-07 20:44:21]

lgtm.  (my comments are mostly nits, so feel free to submit if you agree or
don't think they're important)

On Fri, May 7, 2010 at 4:43 PM, <tommi@chromium.org> wrote:

>
> http://codereview.chromium.org/1994007/diff/3002/11002
> File chrome_frame/chrome_tab.cc (right):
>
> http://codereview.chromium.org/1994007/diff/3002/11002#newcode178
> chrome_frame/chrome_tab.cc:178: HMODULE original_module =
> DllRedirector::GetHModuleFromWndclass();
> should we dcheck that this is never NULL?
>
> http://codereview.chromium.org/1994007/diff/3002/11002#newcode184
> chrome_frame/chrome_tab.cc:184:
> DllRedirector::GetDllGetClassObjectPtr(original_module);
> DCHECK that this is never NULL?
>
> http://codereview.chromium.org/1994007/diff/3002/11003
> File chrome_frame/module_utils.cc (right):
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode43
> chrome_frame/module_utils.cc:43: if (hwnd) {
> DCHECK IsWindow?
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode49
> chrome_frame/module_utils.cc:49: if (!GetLastError()) {
> prefer using ERROR_SUCCESS instead of !GetLastError() since GetLastError
> does not return bool.
>
> if (GetLastError() == ERROR_SUCCESS) {
> }
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode70
> chrome_frame/module_utils.cc:70: 0, 0, 0, NULL, NULL, NULL, NULL);
> DCHECK IsWindow?
>
> http://codereview.chromium.org/1994007/diff/3002/11004
> File chrome_frame/module_utils.h (right):
>
> http://codereview.chromium.org/1994007/diff/3002/11004#newcode17
> chrome_frame/module_utils.h:17: static bool
> DllRedirector::RegisterHModuleWndclass();
> This is a total nit, but should we give the functions more logical names
> than implementation specific ones?
> E.g. RegisterAsFirstCfModule(), GetFirstCfModule(), etc?
>
>
> http://codereview.chromium.org/1994007/show
>

[Sigurður Ásgeirsson, 2010-05-07 20:44:22]

http://codereview.chromium.org/1994007/diff/3002/11003
File chrome_frame/module_utils.cc (right):

http://codereview.chromium.org/1994007/diff/3002/11003#newcode10
chrome_frame/module_utils.cc:10: const wchar_t kBeaconWindowClassName[] =
L"ChromeFrameBeaconWindowClass";
I'm sure this is fine, but a GUID would be more unique.

http://codereview.chromium.org/1994007/diff/3002/11003#newcode69
chrome_frame/module_utils.cc:69: HWND hwnd = CreateWindow(MAKEINTATOM(atom_),
L"temp_window", WS_POPUP, 0,
You'd want to create this window by class name, since you do this only when the
class was registered in another module?
I'd say to use CWindow to wrap the HWND for consistency with other window code.

[robertshield, 2010-05-07 21:03:37]

Thanks, please take another look.

http://codereview.chromium.org/1994007/diff/3002/11002
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/1994007/diff/3002/11002#newcode178
chrome_frame/chrome_tab.cc:178: HMODULE original_module =
DllRedirector::GetHModuleFromWndclass();
On 2010/05/07 20:43:06, tommi wrote:
> should we dcheck that this is never NULL?

Done.

http://codereview.chromium.org/1994007/diff/3002/11002#newcode184
chrome_frame/chrome_tab.cc:184:
DllRedirector::GetDllGetClassObjectPtr(original_module);
On 2010/05/07 20:43:06, tommi wrote:
> DCHECK that this is never NULL?

Done.

http://codereview.chromium.org/1994007/diff/3002/11003
File chrome_frame/module_utils.cc (right):

http://codereview.chromium.org/1994007/diff/3002/11003#newcode10
chrome_frame/module_utils.cc:10: const wchar_t kBeaconWindowClassName[] =
L"ChromeFrameBeaconWindowClass";
On 2010/05/07 20:44:22, Ruðrugis wrote:
> I'm sure this is fine, but a GUID would be more unique.

Appended a GUID so folk can still tell what this class is for.

http://codereview.chromium.org/1994007/diff/3002/11003#newcode43
chrome_frame/module_utils.cc:43: if (hwnd) {
On 2010/05/07 20:43:06, tommi wrote:
> DCHECK IsWindow?

Done.

http://codereview.chromium.org/1994007/diff/3002/11003#newcode49
chrome_frame/module_utils.cc:49: if (!GetLastError()) {
On 2010/05/07 20:43:06, tommi wrote:
> prefer using ERROR_SUCCESS instead of !GetLastError() since GetLastError does
> not return bool.
> 
> if (GetLastError() == ERROR_SUCCESS) {
> }

Done.

http://codereview.chromium.org/1994007/diff/3002/11003#newcode69
chrome_frame/module_utils.cc:69: HWND hwnd = CreateWindow(MAKEINTATOM(atom_),
L"temp_window", WS_POPUP, 0,
On 2010/05/07 20:44:22, Ruðrugis wrote:
> You'd want to create this window by class name, since you do this only when
the
> class was registered in another module?

ugh, yeah. Done.

> I'd say to use CWindow to wrap the HWND for consistency with other window
code. 

CWindow doesn't have a GetClassLongPtr method, so the code ends up looking
roughly the same as it does now.

http://codereview.chromium.org/1994007/diff/3002/11003#newcode70
chrome_frame/module_utils.cc:70: 0, 0, 0, NULL, NULL, NULL, NULL);
On 2010/05/07 20:43:06, tommi wrote:
> DCHECK IsWindow?

Done.

http://codereview.chromium.org/1994007/diff/3002/11004
File chrome_frame/module_utils.h (right):

http://codereview.chromium.org/1994007/diff/3002/11004#newcode17
chrome_frame/module_utils.h:17: static bool
DllRedirector::RegisterHModuleWndclass();
On 2010/05/07 20:43:06, tommi wrote:
> This is a total nit, but should we give the functions more logical names than
> implementation specific ones?
> E.g. RegisterAsFirstCfModule(), GetFirstCfModule(), etc?

Done.

[Sigurður Ásgeirsson, 2010-05-07 21:07:17]

Did you forget to upload?

On Fri, May 7, 2010 at 5:03 PM, <robertshield@chromium.org> wrote:

> Thanks, please take another look.
>
>
>
> http://codereview.chromium.org/1994007/diff/3002/11002
> File chrome_frame/chrome_tab.cc (right):
>
> http://codereview.chromium.org/1994007/diff/3002/11002#newcode178
> chrome_frame/chrome_tab.cc:178: HMODULE original_module =
> DllRedirector::GetHModuleFromWndclass();
> On 2010/05/07 20:43:06, tommi wrote:
>
>> should we dcheck that this is never NULL?
>>
>
> Done.
>
>
> http://codereview.chromium.org/1994007/diff/3002/11002#newcode184
> chrome_frame/chrome_tab.cc:184:
> DllRedirector::GetDllGetClassObjectPtr(original_module);
> On 2010/05/07 20:43:06, tommi wrote:
>
>> DCHECK that this is never NULL?
>>
>
> Done.
>
>
> http://codereview.chromium.org/1994007/diff/3002/11003
> File chrome_frame/module_utils.cc (right):
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode10
> chrome_frame/module_utils.cc:10: const wchar_t kBeaconWindowClassName[]
> = L"ChromeFrameBeaconWindowClass";
> On 2010/05/07 20:44:22, Ruðrugis wrote:
>
>> I'm sure this is fine, but a GUID would be more unique.
>>
>
> Appended a GUID so folk can still tell what this class is for.
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode43
> chrome_frame/module_utils.cc:43: if (hwnd) {
> On 2010/05/07 20:43:06, tommi wrote:
>
>> DCHECK IsWindow?
>>
>
> Done.
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode49
> chrome_frame/module_utils.cc:49: if (!GetLastError()) {
>
> On 2010/05/07 20:43:06, tommi wrote:
>
>> prefer using ERROR_SUCCESS instead of !GetLastError() since
>>
> GetLastError does
>
>> not return bool.
>>
>
>  if (GetLastError() == ERROR_SUCCESS) {
>> }
>>
>
> Done.
>
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode69
> chrome_frame/module_utils.cc:69: HWND hwnd =
> CreateWindow(MAKEINTATOM(atom_), L"temp_window", WS_POPUP, 0,
> On 2010/05/07 20:44:22, Ruðrugis wrote:
>
>> You'd want to create this window by class name, since you do this only
>>
> when the
>
>> class was registered in another module?
>>
>
> ugh, yeah. Done.
>
>
>  I'd say to use CWindow to wrap the HWND for consistency with other
>>
> window code.
>
> CWindow doesn't have a GetClassLongPtr method, so the code ends up
> looking roughly the same as it does now.
>
>
> http://codereview.chromium.org/1994007/diff/3002/11003#newcode70
> chrome_frame/module_utils.cc:70: 0, 0, 0, NULL, NULL, NULL, NULL);
> On 2010/05/07 20:43:06, tommi wrote:
>
>> DCHECK IsWindow?
>>
>
> Done.
>
>
> http://codereview.chromium.org/1994007/diff/3002/11004
> File chrome_frame/module_utils.h (right):
>
> http://codereview.chromium.org/1994007/diff/3002/11004#newcode17
> chrome_frame/module_utils.h:17: static bool
> DllRedirector::RegisterHModuleWndclass();
> On 2010/05/07 20:43:06, tommi wrote:
>
>> This is a total nit, but should we give the functions more logical
>>
> names than
>
>> implementation specific ones?
>> E.g. RegisterAsFirstCfModule(), GetFirstCfModule(), etc?
>>
>
> Done.
>
>
> http://codereview.chromium.org/1994007/show
>

[robertshield, 2010-05-07 21:09:27]

On 2010/05/07 21:07:17, Ruðrugis wrote:
> Did you forget to upload?
> 
> On Fri, May 7, 2010 at 5:03 PM, <mailto:robertshield@chromium.org> wrote:
> 
> > Thanks, please take another look.
> >
> >
> >
> > http://codereview.chromium.org/1994007/diff/3002/11002
> > File chrome_frame/chrome_tab.cc (right):
> >
> > http://codereview.chromium.org/1994007/diff/3002/11002#newcode178
> > chrome_frame/chrome_tab.cc:178: HMODULE original_module =
> > DllRedirector::GetHModuleFromWndclass();
> > On 2010/05/07 20:43:06, tommi wrote:
> >
> >> should we dcheck that this is never NULL?
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/1994007/diff/3002/11002#newcode184
> > chrome_frame/chrome_tab.cc:184:
> > DllRedirector::GetDllGetClassObjectPtr(original_module);
> > On 2010/05/07 20:43:06, tommi wrote:
> >
> >> DCHECK that this is never NULL?
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/1994007/diff/3002/11003
> > File chrome_frame/module_utils.cc (right):
> >
> > http://codereview.chromium.org/1994007/diff/3002/11003#newcode10
> > chrome_frame/module_utils.cc:10: const wchar_t kBeaconWindowClassName[]
> > = L"ChromeFrameBeaconWindowClass";
> > On 2010/05/07 20:44:22, Ruðrugis wrote:
> >
> >> I'm sure this is fine, but a GUID would be more unique.
> >>
> >
> > Appended a GUID so folk can still tell what this class is for.
> >
> > http://codereview.chromium.org/1994007/diff/3002/11003#newcode43
> > chrome_frame/module_utils.cc:43: if (hwnd) {
> > On 2010/05/07 20:43:06, tommi wrote:
> >
> >> DCHECK IsWindow?
> >>
> >
> > Done.
> >
> > http://codereview.chromium.org/1994007/diff/3002/11003#newcode49
> > chrome_frame/module_utils.cc:49: if (!GetLastError()) {
> >
> > On 2010/05/07 20:43:06, tommi wrote:
> >
> >> prefer using ERROR_SUCCESS instead of !GetLastError() since
> >>
> > GetLastError does
> >
> >> not return bool.
> >>
> >
> >  if (GetLastError() == ERROR_SUCCESS) {
> >> }
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/1994007/diff/3002/11003#newcode69
> > chrome_frame/module_utils.cc:69: HWND hwnd =
> > CreateWindow(MAKEINTATOM(atom_), L"temp_window", WS_POPUP, 0,
> > On 2010/05/07 20:44:22, Ruðrugis wrote:
> >
> >> You'd want to create this window by class name, since you do this only
> >>
> > when the
> >
> >> class was registered in another module?
> >>
> >
> > ugh, yeah. Done.
> >
> >
> >  I'd say to use CWindow to wrap the HWND for consistency with other
> >>
> > window code.
> >
> > CWindow doesn't have a GetClassLongPtr method, so the code ends up
> > looking roughly the same as it does now.
> >
> >
> > http://codereview.chromium.org/1994007/diff/3002/11003#newcode70
> > chrome_frame/module_utils.cc:70: 0, 0, 0, NULL, NULL, NULL, NULL);
> > On 2010/05/07 20:43:06, tommi wrote:
> >
> >> DCHECK IsWindow?
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/1994007/diff/3002/11004
> > File chrome_frame/module_utils.h (right):
> >
> > http://codereview.chromium.org/1994007/diff/3002/11004#newcode17
> > chrome_frame/module_utils.h:17: static bool
> > DllRedirector::RegisterHModuleWndclass();
> > On 2010/05/07 20:43:06, tommi wrote:
> >
> >> This is a total nit, but should we give the functions more logical
> >>
> > names than
> >
> >> implementation specific ones?
> >> E.g. RegisterAsFirstCfModule(), GetFirstCfModule(), etc?
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/1994007/show
> >
> 

ugh, sorry, upload failed. uploaded now.

[Sigurður Ásgeirsson, 2010-05-07 21:11:08]

lgtm

[tommi (sloooow) - chröme, 2010-05-07 21:30:31]

lgtm

http://codereview.chromium.org/1994007/diff/8002/19003
File chrome_frame/module_utils.cc (right):

http://codereview.chromium.org/1994007/diff/8002/19003#newcode45
chrome_frame/module_utils.cc:45: DCHECK(IsWindow(hwnd));
uhm, do this check before the if?

http://codereview.chromium.org/1994007/diff/8002/19003#newcode74
chrome_frame/module_utils.cc:74: DCHECK(IsWindow(hwnd));
ditto

[robertshield, 2010-05-07 21:31:19]

http://codereview.chromium.org/1994007/diff/8002/19003
File chrome_frame/module_utils.cc (right):

http://codereview.chromium.org/1994007/diff/8002/19003#newcode45
chrome_frame/module_utils.cc:45: DCHECK(IsWindow(hwnd));
On 2010/05/07 21:30:31, tommi wrote:
> uhm, do this check before the if?

Done.

http://codereview.chromium.org/1994007/diff/8002/19003#newcode74
chrome_frame/module_utils.cc:74: DCHECK(IsWindow(hwnd));
On 2010/05/07 21:30:31, tommi wrote:
> ditto

Done.

[stoyan, 2010-05-07 21:35:22]

lgtm