New unittests for infobar.

New unittests for infobar.
BUG=None
TEST=None

[vadimb, 2010-11-15 06:39:25]

Ready for the review.

[MAD, 2010-11-16 12:58:12]

Excellent...

Just a few minor comments...

Thanks!

BYE
MAD

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/executor_uni...
File ceee/ie/plugin/bho/executor_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/executor_uni...
ceee/ie/plugin/bho/executor_unittest.cc:333: void DeinitializeInfobarManager() {
We typically use Term as opposed to Deinitialize as a counterpart to Initialize.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
File ceee/ie/plugin/bho/infobar_manager_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:16: #include
"testing/gtest/include/gtest/gtest.h"
Just a note that if you don't submit before Jói does (which is unlikely), you'll
need to change this because he is removing the testing/g*/include/ part of these
include folders.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:62: virtual bool IsDestroyed()
const { return false; }
Would be nice to mock those two and test the different behaviors when they
return different values. Could be a TODO for now.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:65: /*
Why this commented out code?

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:118: RECT window_rect = {131,
213, 831, 1013};
Where do these numbers come from?
Comments please... :-)

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
File ceee/ie/plugin/bho/infobar_window_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:34: class MockInfobarBrowserWindow
: public infobar_api::IInfobarBrowserWindow {
It's highly recommended to use the instance counting whenever and object can be
reference counted...
Either comment on why it might not be appropriate here, or add a TODO if you
prefer to do it in another CL.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:81: RECT window_rect = {131, 213,
831, 1013};
Again, please add a comment stating that these values are not meaningful.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:128: // Should not be the target
height yet.
I would add to the comment that we need to call the timer before it reaches max
height.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:135: for (int i = 0;
infobar_window_->sliding_infobar_ && i < 100; ++i) {
Please comment on why you chose to hard code 100 as the max number of loop
turns. Or use a constant declared close to the max height constant since they
are related, right?

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:151: CRect rect1(255, 311, 814,
1015);
Please comment the value choices...

[vadimb, 2010-11-16 22:46:01]

Addressed all the comments.

Reminder: I need you to submit the CL for me when you are happy with it :)

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/executor_uni...
File ceee/ie/plugin/bho/executor_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/executor_uni...
ceee/ie/plugin/bho/executor_unittest.cc:333: void DeinitializeInfobarManager() {
On 2010/11/16 12:58:12, mad1 wrote:
> We typically use Term as opposed to Deinitialize as a counterpart to
Initialize.

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
File ceee/ie/plugin/bho/infobar_manager_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:16: #include
"testing/gtest/include/gtest/gtest.h"
On 2010/11/16 12:58:12, mad1 wrote:
> Just a note that if you don't submit before Jói does (which is unlikely),
you'll
> need to change this because he is removing the testing/g*/include/ part of
these
> include folders.

I will sync before submitting. Or maybe you will have as well to because you
will have to submit on my behalf :)

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:62: virtual bool IsDestroyed()
const { return false; }
On 2010/11/16 12:58:12, mad1 wrote:
> Would be nice to mock those two and test the different behaviors when they
> return different values. Could be a TODO for now.

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:65: /*
On 2010/11/16 12:58:12, mad1 wrote:
> Why this commented out code?

Removed. Sorry, leftover from the previous version.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_mana...
ceee/ie/plugin/bho/infobar_manager_unittest.cc:118: RECT window_rect = {131,
213, 831, 1013};
On 2010/11/16 12:58:12, mad1 wrote:
> Where do these numbers come from?
> Comments please... :-)

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
File ceee/ie/plugin/bho/infobar_window_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:34: class MockInfobarBrowserWindow
: public infobar_api::IInfobarBrowserWindow {
On 2010/11/16 12:58:12, mad1 wrote:
> It's highly recommended to use the instance counting whenever and object can
be
> reference counted...
> Either comment on why it might not be appropriate here, or add a TODO if you
> prefer to do it in another CL.

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:81: RECT window_rect = {131, 213,
831, 1013};
On 2010/11/16 12:58:12, mad1 wrote:
> Again, please add a comment stating that these values are not meaningful.

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:128: // Should not be the target
height yet.
On 2010/11/16 12:58:12, mad1 wrote:
> I would add to the comment that we need to call the timer before it reaches
max
> height.

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:135: for (int i = 0;
infobar_window_->sliding_infobar_ && i < 100; ++i) {
On 2010/11/16 12:58:12, mad1 wrote:
> Please comment on why you chose to hard code 100 as the max number of loop
> turns. Or use a constant declared close to the max height constant since they
> are related, right?

Done.

http://codereview.chromium.org/4991002/diff/1/ceee/ie/plugin/bho/infobar_wind...
ceee/ie/plugin/bho/infobar_window_unittest.cc:151: CRect rect1(255, 311, 814,
1015);
On 2010/11/16 12:58:12, mad1 wrote:
> Please comment the value choices...

Done.

[MAD, 2010-11-17 16:34:32]

LGTM...

Thanks!

There's just one minor typo that I'll fix before committing... and another
comment, just in case you are not familiar with the InstanceCountMixin testing
helper class.

I'll take your CL as is, and apply to tip of tree before I try to commit it...
Will let you know when it's done...

BYE
MAD

http://codereview.chromium.org/4991002/diff/12001/ceee/ie/plugin/bho/infobar_...
File ceee/ie/plugin/bho/infobar_window_unittest.cc (right):

http://codereview.chromium.org/4991002/diff/12001/ceee/ie/plugin/bho/infobar_...
ceee/ie/plugin/bho/infobar_window_unittest.cc:47: ULONG ref_count;
Note that you can also use the InstanceCountMixin base class to do this for you
(from ceee/testing/utils/instance_count_mixin.h, you can see sample usage in a
few places in the CEEE IE test code).

But this will do too...

http://codereview.chromium.org/4991002/diff/12001/ceee/ie/plugin/bho/infobar_...
ceee/ie/plugin/bho/infobar_window_unittest.cc:135: // With sliding enabled, the
hight of the infobar should not be the target
hight -> height

[vadimb, 2010-11-17 18:30:47]

Committed by MAD as
http://src.chromium.org/viewvc/chrome?view=rev&revision=66459

[MAD, 2010-11-17 18:48:15]

On 2010/11/17 18:30:47, vadimb wrote:
> Committed by MAD as
> http://src.chromium.org/viewvc/chrome?view=rev&revision=66459

We usually edit the issue and paste the link in the Change List Description
above...