GMocking Externaltab tests.

GMocking Externaltab tests.
Yet I cannot delete AutomationProxyForExternalTab because it is used by extension_uitest.cc.
Enable some tests meanwhile - now they should be stable enough.

BUG=18797, 24664
TEST=ui_tests ok

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

[Paweł Hajdan Jr., 2009-10-29 19:49:39]

Drive-by.

http://codereview.chromium.org/339009/diff/11/7001
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/11/7001#newcode6
Line 6: #include <gmock/gmock.h>
Shouldn't this #include use "" syntax?

http://codereview.chromium.org/339009/diff/11/7001#newcode865
Line 865: #pragma warning(suppress:4355)
Please use macros from compiler_specific.h

http://codereview.chromium.org/339009/diff/11/7002
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/339009/diff/11/7002#newcode19
Line 19: struct ExternalTabUITestMockClient;
nit: please put an empty line after this line

http://codereview.chromium.org/339009/diff/11/7002#newcode29
Line 29: 
nit: there should be only one empty line here

[stoyan, 2009-10-29 20:26:23]

http://codereview.chromium.org/339009/diff/11/7001
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/11/7001#newcode6
Line 6: #include <gmock/gmock.h>
On 2009/10/29 19:49:40, Paweł Hajdan Jr. wrote:
> Shouldn't this #include use "" syntax?

Done.

http://codereview.chromium.org/339009/diff/11/7001#newcode865
Line 865: #pragma warning(suppress:4355)
On 2009/10/29 19:49:40, Paweł Hajdan Jr. wrote:
> Please use macros from compiler_specific.h

Done.

http://codereview.chromium.org/339009/diff/11/7002
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/339009/diff/11/7002#newcode19
Line 19: struct ExternalTabUITestMockClient;
On 2009/10/29 19:49:40, Paweł Hajdan Jr. wrote:
> nit: please put an empty line after this line

Done.

http://codereview.chromium.org/339009/diff/11/7002#newcode29
Line 29: 
On 2009/10/29 19:49:40, Paweł Hajdan Jr. wrote:
> nit: there should be only one empty line here

Done.

[amit, 2009-10-29 21:04:56]

Excellent stuff, all the odd waits are no longer necessary. Looks so much
better.

http://codereview.chromium.org/339009/diff/9002/10002
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/9002/10002#newcode866
Line 866: MSVC_SUPPRESS_WARNING(4355)
someone has created a framework for this hack: ALLOW_THIS_IN_INITIALIZER_LIST
from compiler_specific.h

http://codereview.chromium.org/339009/diff/9002/10002#newcode885
Line 885: // Here comes an ugly hack. Chrome sends InvalidateHandle IPC message
back
consider making AutomationProxy::InvalidateHandle a virtual function. Overriding
it here would be better than this hack.

http://codereview.chromium.org/339009/diff/9002/10002#newcode908
Line 908: MessageLoop::current()->PostDelayedTask(FROM_HERE, t, timeout_);
Is this is timer tick that occurs several times within a given timeout period?
In that case the tick time should be some integral factor of timeout.

http://codereview.chromium.org/339009/diff/9002/10002#newcode964
Line 964: HWND container_wnd;
better to initialize to NULL

http://codereview.chromium.org/339009/diff/9002/10002#newcode980
Line 980: struct TimedMsgLoop {
This and a bunch of stuff below is common with chrome_frame_unittests.cc, please
factor it out.

http://codereview.chromium.org/339009/diff/9002/10002#newcode1007
Line 1007: template <typename T> T** PointerStorage(scoped_ptr<T>& p) {  //
NOLINT
nit: call it ReceivePointer?

http://codereview.chromium.org/339009/diff/9002/10002#newcode1087
Line 1087: EXPECT_TRUE(tab->SetCookie(url, "robert=burton; "
Can't call set a cookie unless the navigation is complete. This and rest of the
tests should to be done in a EXPECT_CALL(*mock_, OnDidNavigate(....))  block.

http://codereview.chromium.org/339009/diff/9002/10002#newcode1099
Line 1099: EXPECT_TRUE(tab->GetCookieByName(url, "robert", &value_result));
same here, should be done in OnDidNavigate

http://codereview.chromium.org/339009/diff/9002/10002#newcode1138
Line 1138: TEST_F(ExternalTabUITest, PostMessageTarget)  {
Heh, thanks for enabling this :)

[stoyan, 2009-10-29 21:32:11]

http://codereview.chromium.org/339009/diff/9002/10002
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/9002/10002#newcode885
Line 885: // Here comes an ugly hack. Chrome sends InvalidateHandle IPC message
back
On 2009/10/29 21:04:56, amit wrote:
> consider making AutomationProxy::InvalidateHandle a virtual function.
Overriding
> it here would be better than this hack.

Done.

http://codereview.chromium.org/339009/diff/9002/10002#newcode964
Line 964: HWND container_wnd;
On 2009/10/29 21:04:56, amit wrote:
> better to initialize to NULL

Done.

http://codereview.chromium.org/339009/diff/9002/10002#newcode1007
Line 1007: template <typename T> T** PointerStorage(scoped_ptr<T>& p) {  //
NOLINT
On 2009/10/29 21:04:56, amit wrote:
> nit: call it ReceivePointer?

Done.

http://codereview.chromium.org/339009/diff/9002/10002#newcode1087
Line 1087: EXPECT_TRUE(tab->SetCookie(url, "robert=burton; "
Apparently it works without performing any navigation.

[Paweł Hajdan Jr., 2009-10-30 06:06:58]

After fixing the issues below, LGTM.

http://codereview.chromium.org/339009/diff/6013/11011
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/6013/11011#newcode862
Line 862: struct ExternalTabUITestMockClient : public AutomationProxy {
nit: Why a struct?

http://codereview.chromium.org/339009/diff/6013/11011#newcode880
Line 880: HWND host_window_;
nit: There should be an empty line between this and protected:

Does it need to be public?

http://codereview.chromium.org/339009/diff/6013/11011#newcode881
Line 881: protected:
nit: this should be indented one spacd

[stoyan, 2009-10-30 16:28:37]

http://codereview.chromium.org/339009/diff/6013/11011
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/6013/11011#newcode862
Line 862: struct ExternalTabUITestMockClient : public AutomationProxy {
On 2009/10/30 06:06:58, Paweł Hajdan Jr. wrote:
> nit: Why a struct?

Done.

http://codereview.chromium.org/339009/diff/6013/11011#newcode880
Line 880: HWND host_window_;
On 2009/10/30 06:06:58, Paweł Hajdan Jr. wrote:
> nit: There should be an empty line between this and protected:
> 
> Does it need to be public?

Done.

http://codereview.chromium.org/339009/diff/6013/11011#newcode881
Line 881: protected:
On 2009/10/30 06:06:58, Paweł Hajdan Jr. wrote:
> nit: this should be indented one spacd

Done.

[stoyan, 2009-10-30 19:46:51]

moved TimedMsgLoop to ui_test_utils

[amit, 2009-10-30 20:28:07]

lgtm

http://codereview.chromium.org/339009/diff/10007/10008
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/10007/10008#newcode873
Line 873: MOCK_METHOD1(HandleDestroyed, void(int handle));
nit: rename it to HandleClosed or OnClosedHandle

http://codereview.chromium.org/339009/diff/8004/4006#newcode939
Line 939: 
I think we should move these helpers and the following macros to test_utils.h as
well.

[Paweł Hajdan Jr., 2009-10-30 20:33:42]

It's really important to keep thing in ui_test_utils hard to misuse and obvious
when looking at the caller code. I added some comments about that.

http://codereview.chromium.org/339009/diff/8004/4010
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/339009/diff/8004/4010#newcode24
Line 24: class MessageLoop;
nit: Please keep the list sorted.

http://codereview.chromium.org/339009/diff/8004/4010#newcode140
Line 140: class TimedMsgLoop {
I don't like the abbreviation in the name. And the class name is a little
non-obvious. I don't have any better, but please at least expand the
abbreviation. Or maybe even TimedMessageLoopRunner?

http://codereview.chromium.org/339009/diff/8004/4010#newcode142
Line 142: TimedMsgLoop();
Is this ctor ever used? It also lacks documentation.

http://codereview.chromium.org/339009/diff/8004/4010#newcode146
Line 146: void RunFor(int ms);
These methods lack comments...

http://codereview.chromium.org/339009/diff/8004/4010#newcode147
Line 147: void Quit();
Which is dangerous because for example it isn't obvious that it posts a QuitTask
instead of calling Quit directly.

http://codereview.chromium.org/339009/diff/8004/4010#newcode151
Line 151: bool owned_;
If you never use owned_ (which seems to be the case), the class would be just a
function. Or maybe even you could use RunMessageLoop... Anyway, it seems like
this class may be trying to do too much.

http://codereview.chromium.org/339009/diff/8004/4010#newcode152
Line 152: };
DISALLOW_COPY_AND_ASSIGN

[stoyan, 2009-10-30 22:20:28]

http://codereview.chromium.org/339009/diff/8004/4006
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/339009/diff/8004/4006#newcode939
Line 939: void ExternalTabUITestMockClient::DestroyHostWindow() {
Helpers moved.
Macros will pull gmock, is that ok?

http://codereview.chromium.org/339009/diff/8004/4010
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/339009/diff/8004/4010#newcode24
Line 24: class MessageLoop;
On 2009/10/30 20:33:42, Paweł Hajdan Jr. wrote:
> nit: Please keep the list sorted.

Done.

http://codereview.chromium.org/339009/diff/8004/4010#newcode142
Line 142: TimedMsgLoop();
It can be used when chrome_frame_unittests.cc version of TimedMsgLoop is
replaced by this one.

http://codereview.chromium.org/339009/diff/8004/4010#newcode146
Line 146: void RunFor(int ms);
On 2009/10/30 20:33:42, Paweł Hajdan Jr. wrote:
> These methods lack comments...

Done.

http://codereview.chromium.org/339009/diff/8004/4010#newcode151
Line 151: bool owned_;
It should be used in "normal" circumstances, where test sets the type of needed
message loop.

http://codereview.chromium.org/339009/diff/8004/4010#newcode152
Line 152: };
On 2009/10/30 20:33:42, Paweł Hajdan Jr. wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Paweł Hajdan Jr., 2009-10-30 22:27:45]

http://codereview.chromium.org/339009/diff/4017/8016
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/339009/diff/4017/8016#newcode140
Line 140: class TimedMsgLoop {
Please don't use the Msg abbreviation in the class name. I'd strongly prefer
TimedMessageLoopRunner instead.

http://codereview.chromium.org/339009/diff/4017/8016#newcode142
Line 142: TimedMsgLoop();
You didn't document the behavior of this ctor.

http://codereview.chromium.org/339009/diff/4017/8016#newcode146
Line 146: // Run the message loop for ms milliseconds
nit: Please put dot at the end of the sentence and put empty lines between
method declarations.

http://codereview.chromium.org/339009/diff/4017/8016#newcode148
Line 148: // Posts Quit task to the message loop
nit: same here

http://codereview.chromium.org/339009/diff/4017/8016#newcode153
Line 153: MessageLoop* loop_;
Ooops. Are you sure these should be public?

http://codereview.chromium.org/339009/diff/4017/8016#newcode155
Line 155: private:
nit: There should be one empty line before private:

http://codereview.chromium.org/339009/diff/4017/8016#newcode160
Line 160: template <typename T> T** ReceivePointer(scoped_ptr<T>& p) {  //
NOLINT
IMHO these *Pointer functions don't belong in ui_test_utils. If you need to
reuse them in other Chrome Frame test, please find a new home for them.
Especially that they seem to be a little scary.

[stoyan, 2009-11-02 20:34:21]

http://codereview.chromium.org/339009/diff/4017/8016
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/339009/diff/4017/8016#newcode140
Line 140: class TimedMsgLoop {
On 2009/10/30 22:27:46, Paweł Hajdan Jr. wrote:
> Please don't use the Msg abbreviation in the class name. I'd strongly prefer
> TimedMessageLoopRunner instead.

Done.

http://codereview.chromium.org/339009/diff/4017/8016#newcode142
Line 142: TimedMsgLoop();
On 2009/10/30 22:27:46, Paweł Hajdan Jr. wrote:
> You didn't document the behavior of this ctor.

Done.

http://codereview.chromium.org/339009/diff/4017/8016#newcode146
Line 146: // Run the message loop for ms milliseconds
On 2009/10/30 22:27:46, Paweł Hajdan Jr. wrote:
> nit: Please put dot at the end of the sentence and put empty lines between
> method declarations.

Done.

http://codereview.chromium.org/339009/diff/4017/8016#newcode148
Line 148: // Posts Quit task to the message loop
On 2009/10/30 22:27:46, Paweł Hajdan Jr. wrote:
> nit: same here

Done.

http://codereview.chromium.org/339009/diff/4017/8016#newcode153
Line 153: MessageLoop* loop_;
I moved them to private, though accessor to loop_ seems will be needed soon.

http://codereview.chromium.org/339009/diff/4017/8016#newcode155
Line 155: private:
On 2009/10/30 22:27:46, Paweł Hajdan Jr. wrote:
> nit: There should be one empty line before private:

Done.

http://codereview.chromium.org/339009/diff/4017/8016#newcode160
Line 160: template <typename T> T** ReceivePointer(scoped_ptr<T>& p) {  //
NOLINT
Ok, I moved them back.

[stoyan, 2009-11-04 20:39:17]

ping

[Paweł Hajdan Jr., 2009-11-04 20:45:17]

After fixing the nits, LGTM. Sorry for the delay.

http://codereview.chromium.org/339009/diff/13012/12009
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/339009/diff/13012/12009#newcode507
Line 507: : loop_(new MessageLoopForUI()), owned_(true) {
nit: The style guide says that there should be a new line after the comma.

http://codereview.chromium.org/339009/diff/13012/12008
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/339009/diff/13012/12008#newcode142
Line 142: // Create new MessageLoopForUI and attaches to it.
nit: should be consistent: "Creates" and "attaches" or "Create" and "attach".

http://codereview.chromium.org/339009/diff/13012/12008#newcode151
Line 151: // Run the message loop for ms milliseconds.
Please also make the form consistent in all comments about this class: Creates,
Attaches, Runs, Posts or Create, Attach, Run, Post.

http://codereview.chromium.org/339009/diff/13012/12008#newcode163
Line 163: DISALLOW_COPY_AND_ASSIGN(TimedMessageLoopRunner);
nit: Add an empty line above.