First batch of context menu tests...

First batch of context menu tests

Refactored various methods to send keyboard and mouse
input. Fixed the context menu focus issue on IE7.
Improved existing tests to make them less flaky and
added 3 new tests for context menu items.


BUG=34673
TEST=new tests added



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

[tommi (sloooow) - chröme, 2010-02-11 05:03:00]

lgtm.  very nice.

http://codereview.chromium.org/604014/diff/1/5
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/604014/diff/1/5#newcode110
chrome_frame/chrome_frame_plugin.h:110: T* pThis = static_cast<T*>(this);
maybe rename pThis to... er.. self? :)

http://codereview.chromium.org/604014/diff/1/8
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/604014/diff/1/8#newcode598
chrome_frame/test/chrome_frame_test_utils.cc:598: IsWindow(first_child);
first_child = GetWindow(first_child, GW_CHILD)) {
I remember something about EnumChildWindows being preferred to this way of
enumerating windows.  Up to you.

http://codereview.chromium.org/604014/diff/1/8#newcode604
chrome_frame/test/chrome_frame_test_utils.cc:604:
EXPECT_TRUE(!_wcsicmp(class_name, L"Chrome_RenderWidgetHostHWND"));
prefer using == 0 in this case as using ! reads as "not compares"

http://codereview.chromium.org/604014/diff/1/8#newcode611
chrome_frame/test/chrome_frame_test_utils.cc:611:
AttachThreadInput(GetCurrentThreadId(), renderer_thread, TRUE);
maybe check the return value here?

http://codereview.chromium.org/604014/diff/1/10
File chrome_frame/test/simulate_input.cc (right):

http://codereview.chromium.org/604014/diff/1/10#newcode12
chrome_frame/test/simulate_input.cc:12: #include "base/scoped_handle.h"
remove this block

http://codereview.chromium.org/604014/diff/1/10#newcode51
chrome_frame/test/simulate_input.cc:51: if (msg.message == WM_HOTKEY)
{}

http://codereview.chromium.org/604014/diff/1/7
File chrome_frame/test/simulate_input.h (right):

http://codereview.chromium.org/604014/diff/1/7#newcode1
chrome_frame/test/simulate_input.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
2010

http://codereview.chromium.org/604014/diff/1/4
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/604014/diff/1/4#newcode866
chrome_frame/utils.cc:866: if (!parent)
this and the remaining lines could be reduced to

return !parent || parent == GetDesktopWindow();

[stoyan, 2010-02-11 20:44:40]

http://codereview.chromium.org/604014/diff/1/8
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/604014/diff/1/8#newcode563
chrome_frame/test/chrome_frame_test_utils.cc:563: HWND
WebBrowserEventSink::GetTabWindow() {
This returns the renderer window, right? The name looks misleading

http://codereview.chromium.org/604014/diff/1/10
File chrome_frame/test/simulate_input.cc (right):

http://codereview.chromium.org/604014/diff/1/10#newcode33
chrome_frame/test/simulate_input.cc:33: static const int kHotKeyId = 0x0000baba;
baba in bulgarian means "grandmother".

http://codereview.chromium.org/604014/diff/1/10#newcode36
chrome_frame/test/simulate_input.cc:36: SetWindowLongPtr(GWLP_USERDATA,
reinterpret_cast<ULONG_PTR>(window));
you can store 'window' as a member of the class.

http://codereview.chromium.org/604014/diff/1/10#newcode53
chrome_frame/test/simulate_input.cc:53: else if (msg.message == WM_TIMER) {
no need of "else", right?

http://codereview.chromium.org/604014/diff/1/10#newcode193
chrome_frame/test/simulate_input.cc:193: double screen_height =
::GetSystemMetrics(SM_CYSCREEN) - 1;
GetSystemMetrics(SM_CXSCREEN) will retrieve screen size of the primarary monitor
only. And monitors arrangement could be pretty arbitrary.

http://codereview.chromium.org/604014/diff/1/7
File chrome_frame/test/simulate_input.h (right):

http://codereview.chromium.org/604014/diff/1/7#newcode48
chrome_frame/test/simulate_input.h:48: void SendString(const char_type* s) {
Can we come with a scheme where we can mix control keys and chars in a single
string?
To have something like:
char input[] = "AB" KEY(VK_UP) "CD" KEY(VK_DOWN);

#define QUOTE2(x)  #x
#define QUOTE(x)  QUOTE2(x)
#define MERGE(x, y) x ## y
#define KEY(x)  "$" QUOTE(MERGE(\,x))

Here the $ is the escape character so we know to treat next entry not as char
but as a control code.

http://codereview.chromium.org/604014/diff/1/11
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/604014/diff/1/11#newcode60
chrome_frame/test/test_mock_with_web_server.cc:60: const wchar_t kKeyEventUrl[]
= L"http://localhost:1337/files/keyevent.html";
use server.Resolve("keyevent.html") instead of hardcoding, (here and below)

http://codereview.chromium.org/604014/diff/1/11#newcode111
chrome_frame/test/test_mock_with_web_server.cc:111:
testing::InvokeWithoutArgs(CreateFunctor(&mock,
Woudn't be easier you simply quit the loop and then call
ExpectRendererWidnowHasfocus?
Anyway you have mock.Uninitialize(0 and CloseAllIEWindows() at the end of the
test.

http://codereview.chromium.org/604014/diff/1/11#newcode260
chrome_frame/test/test_mock_with_web_server.cc:260:
testing::InvokeWithoutArgs(CreateFunctor(&mock,
same as above. Just quit the loop and do cleanup later.

[amit, 2010-02-12 02:53:43]

Done with the suggestions. Some more tests fixed and new patch is uploaded.
Please take another look. Thanks.

http://codereview.chromium.org/604014/diff/1/5
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/604014/diff/1/5#newcode110
chrome_frame/chrome_frame_plugin.h:110: T* pThis = static_cast<T*>(this);
On 2010/02/11 05:03:00, tommi wrote:
> maybe rename pThis to... er.. self? :)

Done.

http://codereview.chromium.org/604014/diff/1/8
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/604014/diff/1/8#newcode563
chrome_frame/test/chrome_frame_test_utils.cc:563: HWND
WebBrowserEventSink::GetTabWindow() {
On 2010/02/11 20:44:40, stoyan wrote:
> This returns the renderer window, right? The name looks misleading

Changed to GetRendererWindow()

http://codereview.chromium.org/604014/diff/1/8#newcode604
chrome_frame/test/chrome_frame_test_utils.cc:604:
EXPECT_TRUE(!_wcsicmp(class_name, L"Chrome_RenderWidgetHostHWND"));
On 2010/02/11 05:03:00, tommi wrote:
> prefer using == 0 in this case as using ! reads as "not compares"

Done.

http://codereview.chromium.org/604014/diff/1/8#newcode611
chrome_frame/test/chrome_frame_test_utils.cc:611:
AttachThreadInput(GetCurrentThreadId(), renderer_thread, TRUE);
On 2010/02/11 05:03:00, tommi wrote:
> maybe check the return value here?

Done.

http://codereview.chromium.org/604014/diff/1/10
File chrome_frame/test/simulate_input.cc (right):

http://codereview.chromium.org/604014/diff/1/10#newcode12
chrome_frame/test/simulate_input.cc:12: #include "base/scoped_handle.h"
On 2010/02/11 05:03:00, tommi wrote:
> remove this block

Done.

http://codereview.chromium.org/604014/diff/1/10#newcode33
chrome_frame/test/simulate_input.cc:33: static const int kHotKeyId = 0x0000baba;
On 2010/02/11 20:44:40, stoyan wrote:
> baba in bulgarian means "grandmother".
Means "dad" in a few Indian languages. :)

http://codereview.chromium.org/604014/diff/1/10#newcode36
chrome_frame/test/simulate_input.cc:36: SetWindowLongPtr(GWLP_USERDATA,
reinterpret_cast<ULONG_PTR>(window));
On 2010/02/11 20:44:40, stoyan wrote:
> you can store 'window' as a member of the class.

Done.

http://codereview.chromium.org/604014/diff/1/10#newcode51
chrome_frame/test/simulate_input.cc:51: if (msg.message == WM_HOTKEY)
On 2010/02/11 05:03:00, tommi wrote:
> {}

Done.

http://codereview.chromium.org/604014/diff/1/10#newcode53
chrome_frame/test/simulate_input.cc:53: else if (msg.message == WM_TIMER) {
On 2010/02/11 20:44:40, stoyan wrote:
> no need of "else", right?

Done.

http://codereview.chromium.org/604014/diff/1/10#newcode193
chrome_frame/test/simulate_input.cc:193: double screen_height =
::GetSystemMetrics(SM_CYSCREEN) - 1;
On 2010/02/11 20:44:40, stoyan wrote:
> GetSystemMetrics(SM_CXSCREEN) will retrieve screen size of the primarary
monitor
> only. And monitors arrangement could be pretty arbitrary.

True, Added a todo to this effect for now.

http://codereview.chromium.org/604014/diff/1/7
File chrome_frame/test/simulate_input.h (right):

http://codereview.chromium.org/604014/diff/1/7#newcode1
chrome_frame/test/simulate_input.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/02/11 05:03:00, tommi wrote:
> 2010

Done.

http://codereview.chromium.org/604014/diff/1/7#newcode48
chrome_frame/test/simulate_input.h:48: void SendString(const char_type* s) {
On 2010/02/11 20:44:40, stoyan wrote:
> Can we come with a scheme where we can mix control keys and chars in a single
> string?
> To have something like:
> char input[] = "AB" KEY(VK_UP) "CD" KEY(VK_DOWN);
> 
> #define QUOTE2(x)  #x
> #define QUOTE(x)  QUOTE2(x)
> #define MERGE(x, y) x ## y
> #define KEY(x)  "$" QUOTE(MERGE(\,x))
> 
> Here the $ is the escape character so we know to treat next entry not as char
> but as a control code.
Interesting idea :) The input simulation code looks simple but is somewhat
finicky. So will keep in mind during next iteration.

http://codereview.chromium.org/604014/diff/1/11
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/604014/diff/1/11#newcode60
chrome_frame/test/test_mock_with_web_server.cc:60: const wchar_t kKeyEventUrl[]
= L"http://localhost:1337/files/keyevent.html";
On 2010/02/11 20:44:40, stoyan wrote:
> use server.Resolve("keyevent.html") instead of hardcoding, (here and below)
Thanks for pointing this out. Right now multiple tests share a few URL
constants. So if it is OK with you, I would prefer to get to this in the next
CL.

http://codereview.chromium.org/604014/diff/1/11#newcode111
chrome_frame/test/test_mock_with_web_server.cc:111:
testing::InvokeWithoutArgs(CreateFunctor(&mock,
On 2010/02/11 20:44:40, stoyan wrote:
> Woudn't be easier you simply quit the loop and then call
> ExpectRendererWidnowHasfocus?
> Anyway you have mock.Uninitialize(0 and CloseAllIEWindows() at the end of the
> test.
> 

It's probably better to have a message loop running when the thread inputs are
attached.

http://codereview.chromium.org/604014/diff/1/4
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/604014/diff/1/4#newcode866
chrome_frame/utils.cc:866: if (!parent)
On 2010/02/11 05:03:00, tommi wrote:
> this and the remaining lines could be reduced to
> 
> return !parent || parent == GetDesktopWindow();

Done.

[tommi (sloooow) - chröme, 2010-02-12 16:21:18]

I don't see the latest changes

On Thu, Feb 11, 2010 at 9:53 PM, <amit@chromium.org> wrote:

> Done with the suggestions. Some more tests fixed and new patch is uploaded.
> Please take another look. Thanks.
>
>
>
> http://codereview.chromium.org/604014/diff/1/5
> File chrome_frame/chrome_frame_plugin.h (right):
>
> http://codereview.chromium.org/604014/diff/1/5#newcode110
> chrome_frame/chrome_frame_plugin.h:110: T* pThis =
> static_cast<T*>(this);
> On 2010/02/11 05:03:00, tommi wrote:
>
>> maybe rename pThis to... er.. self? :)
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/8
> File chrome_frame/test/chrome_frame_test_utils.cc (right):
>
> http://codereview.chromium.org/604014/diff/1/8#newcode563
> chrome_frame/test/chrome_frame_test_utils.cc:563: HWND
> WebBrowserEventSink::GetTabWindow() {
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> This returns the renderer window, right? The name looks misleading
>>
>
> Changed to GetRendererWindow()
>
>
> http://codereview.chromium.org/604014/diff/1/8#newcode604
> chrome_frame/test/chrome_frame_test_utils.cc:604:
> EXPECT_TRUE(!_wcsicmp(class_name, L"Chrome_RenderWidgetHostHWND"));
> On 2010/02/11 05:03:00, tommi wrote:
>
>> prefer using == 0 in this case as using ! reads as "not compares"
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/8#newcode611
> chrome_frame/test/chrome_frame_test_utils.cc:611:
> AttachThreadInput(GetCurrentThreadId(), renderer_thread, TRUE);
> On 2010/02/11 05:03:00, tommi wrote:
>
>> maybe check the return value here?
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/10
> File chrome_frame/test/simulate_input.cc (right):
>
> http://codereview.chromium.org/604014/diff/1/10#newcode12
> chrome_frame/test/simulate_input.cc:12: #include "base/scoped_handle.h"
> On 2010/02/11 05:03:00, tommi wrote:
>
>> remove this block
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/10#newcode33
> chrome_frame/test/simulate_input.cc:33: static const int kHotKeyId =
> 0x0000baba;
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> baba in bulgarian means "grandmother".
>>
> Means "dad" in a few Indian languages. :)
>
>
> http://codereview.chromium.org/604014/diff/1/10#newcode36
> chrome_frame/test/simulate_input.cc:36: SetWindowLongPtr(GWLP_USERDATA,
> reinterpret_cast<ULONG_PTR>(window));
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> you can store 'window' as a member of the class.
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/10#newcode51
> chrome_frame/test/simulate_input.cc:51: if (msg.message == WM_HOTKEY)
> On 2010/02/11 05:03:00, tommi wrote:
>
>> {}
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/10#newcode53
> chrome_frame/test/simulate_input.cc:53: else if (msg.message ==
> WM_TIMER) {
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> no need of "else", right?
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/10#newcode193
> chrome_frame/test/simulate_input.cc:193: double screen_height =
> ::GetSystemMetrics(SM_CYSCREEN) - 1;
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> GetSystemMetrics(SM_CXSCREEN) will retrieve screen size of the
>>
> primarary monitor
>
>> only. And monitors arrangement could be pretty arbitrary.
>>
>
> True, Added a todo to this effect for now.
>
>
> http://codereview.chromium.org/604014/diff/1/7
> File chrome_frame/test/simulate_input.h (right):
>
> http://codereview.chromium.org/604014/diff/1/7#newcode1
> chrome_frame/test/simulate_input.h:1: // Copyright (c) 2009 The Chromium
> Authors. All rights reserved.
> On 2010/02/11 05:03:00, tommi wrote:
>
>> 2010
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014/diff/1/7#newcode48
> chrome_frame/test/simulate_input.h:48: void SendString(const char_type*
> s) {
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> Can we come with a scheme where we can mix control keys and chars in a
>>
> single
>
>> string?
>> To have something like:
>> char input[] = "AB" KEY(VK_UP) "CD" KEY(VK_DOWN);
>>
>
>  #define QUOTE2(x)  #x
>> #define QUOTE(x)  QUOTE2(x)
>> #define MERGE(x, y) x ## y
>> #define KEY(x)  "$" QUOTE(MERGE(\,x))
>>
>
>  Here the $ is the escape character so we know to treat next entry not
>>
> as char
>
>> but as a control code.
>>
> Interesting idea :) The input simulation code looks simple but is
> somewhat finicky. So will keep in mind during next iteration.
>
>
> http://codereview.chromium.org/604014/diff/1/11
> File chrome_frame/test/test_mock_with_web_server.cc (right):
>
> http://codereview.chromium.org/604014/diff/1/11#newcode60
> chrome_frame/test/test_mock_with_web_server.cc:60: const wchar_t
> kKeyEventUrl[] = L"http://localhost:1337/files/keyevent.html";
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> use server.Resolve("keyevent.html") instead of hardcoding, (here and
>>
> below)
> Thanks for pointing this out. Right now multiple tests share a few URL
> constants. So if it is OK with you, I would prefer to get to this in the
> next CL.
>
>
> http://codereview.chromium.org/604014/diff/1/11#newcode111
> chrome_frame/test/test_mock_with_web_server.cc:111:
> testing::InvokeWithoutArgs(CreateFunctor(&mock,
> On 2010/02/11 20:44:40, stoyan wrote:
>
>> Woudn't be easier you simply quit the loop and then call
>> ExpectRendererWidnowHasfocus?
>> Anyway you have mock.Uninitialize(0 and CloseAllIEWindows() at the end
>>
> of the
>
>> test.
>>
>
>
> It's probably better to have a message loop running when the thread
> inputs are attached.
>
>
> http://codereview.chromium.org/604014/diff/1/4
> File chrome_frame/utils.cc (right):
>
> http://codereview.chromium.org/604014/diff/1/4#newcode866
> chrome_frame/utils.cc:866: if (!parent)
> On 2010/02/11 05:03:00, tommi wrote:
>
>> this and the remaining lines could be reduced to
>>
>
>  return !parent || parent == GetDesktopWindow();
>>
>
> Done.
>
>
> http://codereview.chromium.org/604014
>

[amit, 2010-02-12 16:52:51]

Uploaded again :)

On Fri, Feb 12, 2010 at 8:21 AM, Tommi <tommi@chromium.org> wrote:
> I don't see the latest changes
>
> On Thu, Feb 11, 2010 at 9:53 PM, <amit@chromium.org> wrote:
>>
>> Done with the suggestions. Some more tests fixed and new patch is
>> uploaded.
>> Please take another look. Thanks.
>>
>>
>> http://codereview.chromium.org/604014/diff/1/5
>> File chrome_frame/chrome_frame_plugin.h (right):
>>
>> http://codereview.chromium.org/604014/diff/1/5#newcode110
>> chrome_frame/chrome_frame_plugin.h:110: T* pThis =
>> static_cast<T*>(this);
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> maybe rename pThis to... er.. self? :)
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/8
>> File chrome_frame/test/chrome_frame_test_utils.cc (right):
>>
>> http://codereview.chromium.org/604014/diff/1/8#newcode563
>> chrome_frame/test/chrome_frame_test_utils.cc:563: HWND
>> WebBrowserEventSink::GetTabWindow() {
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> This returns the renderer window, right? The name looks misleading
>>
>> Changed to GetRendererWindow()
>>
>> http://codereview.chromium.org/604014/diff/1/8#newcode604
>> chrome_frame/test/chrome_frame_test_utils.cc:604:
>> EXPECT_TRUE(!_wcsicmp(class_name, L"Chrome_RenderWidgetHostHWND"));
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> prefer using == 0 in this case as using ! reads as "not compares"
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/8#newcode611
>> chrome_frame/test/chrome_frame_test_utils.cc:611:
>> AttachThreadInput(GetCurrentThreadId(), renderer_thread, TRUE);
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> maybe check the return value here?
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/10
>> File chrome_frame/test/simulate_input.cc (right):
>>
>> http://codereview.chromium.org/604014/diff/1/10#newcode12
>> chrome_frame/test/simulate_input.cc:12: #include "base/scoped_handle.h"
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> remove this block
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/10#newcode33
>> chrome_frame/test/simulate_input.cc:33: static const int kHotKeyId =
>> 0x0000baba;
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> baba in bulgarian means "grandmother".
>>
>> Means "dad" in a few Indian languages. :)
>>
>> http://codereview.chromium.org/604014/diff/1/10#newcode36
>> chrome_frame/test/simulate_input.cc:36: SetWindowLongPtr(GWLP_USERDATA,
>> reinterpret_cast<ULONG_PTR>(window));
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> you can store 'window' as a member of the class.
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/10#newcode51
>> chrome_frame/test/simulate_input.cc:51: if (msg.message == WM_HOTKEY)
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> {}
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/10#newcode53
>> chrome_frame/test/simulate_input.cc:53: else if (msg.message ==
>> WM_TIMER) {
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> no need of "else", right?
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/10#newcode193
>> chrome_frame/test/simulate_input.cc:193: double screen_height =
>> ::GetSystemMetrics(SM_CYSCREEN) - 1;
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> GetSystemMetrics(SM_CXSCREEN) will retrieve screen size of the
>>
>> primarary monitor
>>>
>>> only. And monitors arrangement could be pretty arbitrary.
>>
>> True, Added a todo to this effect for now.
>>
>> http://codereview.chromium.org/604014/diff/1/7
>> File chrome_frame/test/simulate_input.h (right):
>>
>> http://codereview.chromium.org/604014/diff/1/7#newcode1
>> chrome_frame/test/simulate_input.h:1: // Copyright (c) 2009 The Chromium
>> Authors. All rights reserved.
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> 2010
>>
>> Done.
>>
>> http://codereview.chromium.org/604014/diff/1/7#newcode48
>> chrome_frame/test/simulate_input.h:48: void SendString(const char_type*
>> s) {
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> Can we come with a scheme where we can mix control keys and chars in a
>>
>> single
>>>
>>> string?
>>> To have something like:
>>> char input[] = "AB" KEY(VK_UP) "CD" KEY(VK_DOWN);
>>
>>> #define QUOTE2(x)  #x
>>> #define QUOTE(x)  QUOTE2(x)
>>> #define MERGE(x, y) x ## y
>>> #define KEY(x)  "$" QUOTE(MERGE(\,x))
>>
>>> Here the $ is the escape character so we know to treat next entry not
>>
>> as char
>>>
>>> but as a control code.
>>
>> Interesting idea :) The input simulation code looks simple but is
>> somewhat finicky. So will keep in mind during next iteration.
>>
>> http://codereview.chromium.org/604014/diff/1/11
>> File chrome_frame/test/test_mock_with_web_server.cc (right):
>>
>> http://codereview.chromium.org/604014/diff/1/11#newcode60
>> chrome_frame/test/test_mock_with_web_server.cc:60: const wchar_t
>> kKeyEventUrl[] = L"http://localhost:1337/files/keyevent.html";
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> use server.Resolve("keyevent.html") instead of hardcoding, (here and
>>
>> below)
>> Thanks for pointing this out. Right now multiple tests share a few URL
>> constants. So if it is OK with you, I would prefer to get to this in the
>> next CL.
>>
>> http://codereview.chromium.org/604014/diff/1/11#newcode111
>> chrome_frame/test/test_mock_with_web_server.cc:111:
>> testing::InvokeWithoutArgs(CreateFunctor(&mock,
>> On 2010/02/11 20:44:40, stoyan wrote:
>>>
>>> Woudn't be easier you simply quit the loop and then call
>>> ExpectRendererWidnowHasfocus?
>>> Anyway you have mock.Uninitialize(0 and CloseAllIEWindows() at the end
>>
>> of the
>>>
>>> test.
>>
>>
>> It's probably better to have a message loop running when the thread
>> inputs are attached.
>>
>> http://codereview.chromium.org/604014/diff/1/4
>> File chrome_frame/utils.cc (right):
>>
>> http://codereview.chromium.org/604014/diff/1/4#newcode866
>> chrome_frame/utils.cc:866: if (!parent)
>> On 2010/02/11 05:03:00, tommi wrote:
>>>
>>> this and the remaining lines could be reduced to
>>
>>> return !parent || parent == GetDesktopWindow();
>>
>> Done.
>>
>> http://codereview.chromium.org/604014
>
>

[tommi (sloooow) - chröme, 2010-02-12 17:14:05]

lgtm.  looks _great_ actually.

http://codereview.chromium.org/604014/diff/8005/5023
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/604014/diff/8005/5023#newcode487
chrome_frame/test/chrome_frame_test_utils.cc:487: ScopedVariant data, origin,
source;
very nice

http://codereview.chromium.org/604014/diff/8005/5023#newcode621
chrome_frame/test/chrome_frame_test_utils.cc:621: IsWindow(first_child);
first_child = GetWindow(first_child, GW_CHILD)) {
indent off

http://codereview.chromium.org/604014/diff/8005/5031
File testing/generate_gmock_mutant.py (right):

http://codereview.chromium.org/604014/diff/8005/5031#newcode442
testing/generate_gmock_mutant.py:442: for prebound in xrange(0, 6 + 1):
fyi - I have no context to review this file :-/

[stoyan, 2010-02-12 17:31:56]

lg