Implement drag and drop testing in a platform independent way.

Implement drag and drop testing in a platform independent way.

The CL also adds a simple framework for writing platform
independent tests.

TEST=PanelBrowserTest.DragPanels
BUG=73936
modified:   chrome/browser/ui/panels/native_panel.h
modified:   chrome/browser/ui/panels/panel_browser_view.cc
modified:   chrome/browser/ui/panels/panel_browser_view.h
modified:   chrome/browser/ui/panels/panel_browser_view_browsertest.cc
modified:   chrome/browser/ui/panels/panel_browser_window_cocoa.h
modified:   chrome/browser/ui/panels/panel_browser_window_cocoa.mm
modified:   chrome/browser/ui/panels/panel_browser_window_gtk.cc
modified:   chrome/browser/ui/panels/panel_browser_window_gtk.h
modified:   chrome/browser/ui/panels/panel_browsertest.cc

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

[prasadt, 2011-08-09 20:40:51]

I tested this on Linux.  Relying on try bots for testing on windows.  Test
disabled on Mac.

[jianli, 2011-08-09 21:10:23]

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:14: class Panel;
We already include panel.h. I think this is removed in jennb's patch.

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:74: virtual void
TitlebarLeftButtonPress(gfx::Point point) = 0;
It would be better to call it either TitlebarLeftButtonPressed or
PressTitlebarLeftButton.

Also, button seems to be vague since it might mean the mouse button or the UI
button. How about calling it TitlebarLeftMouseButtonPressed.

One other thing is about we should make bar capitalized. It seems that we have
mixed use of both Titlebar and TitleBar.

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:405:
panel_browser_view_->OnTitleBarMousePressed(pressed);
It seems to me that we can refactor PanelBrowserView::OnTitleBarMousePressed and
other related methods a little bit to simplify the testing code, like:

void PanelBrowserView::OnTitleBarMousePressed(const gfx::Point& point) {
  mouse_pressed_ = true;
  mouse_pressed_point_ = event.location();
  mouse_dragging_ = false;
}

void NativePanelTestingWin::TitlebarLeftMouseButtonPressed(const gfx::Point&
point) {
  panel_browser_view_->OnTitleBarMousePressed(pressed);
}

We need to change the class PanelBrowserFrameView to pass the point directly.

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.h:28: friend class
NativePanelTestingGtk;
Should it be NativePanelTestingWin? Also it would be better to be moved to
private section.

[prasadt, 2011-08-09 23:05:04]

Thanks for the feedback.

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:14: class Panel;
On 2011/08/09 21:10:23, jianli wrote:
> We already include panel.h. I think this is removed in jennb's patch.

Done.

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:74: virtual void
TitlebarLeftButtonPress(gfx::Point point) = 0;
On 2011/08/09 21:10:23, jianli wrote:
> It would be better to call it either TitlebarLeftButtonPressed or
> PressTitlebarLeftButton.
> 
> Also, button seems to be vague since it might mean the mouse button or the UI
> button. How about calling it TitlebarLeftMouseButtonPressed.
> 
> One other thing is about we should make bar capitalized. It seems that we have
> mixed use of both Titlebar and TitleBar.

I changed the name to PressLeftMouseButtonTitlebar.  Keeping Titlebar to the end
- I think its makes better grouping of the functions.

TitlebarLeftMouseButtonPressed sounds like an event handler than a verb. 
Titlebar vs TitleBar, you're right we're mixing them up.  I picked Titlebar, I
think of it as one word.

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:405:
panel_browser_view_->OnTitleBarMousePressed(pressed);
On 2011/08/09 21:10:23, jianli wrote:
> It seems to me that we can refactor PanelBrowserView::OnTitleBarMousePressed
and
> other related methods a little bit to simplify the testing code, like:
> 
> void PanelBrowserView::OnTitleBarMousePressed(const gfx::Point& point) {
>   mouse_pressed_ = true;
>   mouse_pressed_point_ = event.location();
>   mouse_dragging_ = false;
> }
> 
> void NativePanelTestingWin::TitlebarLeftMouseButtonPressed(const gfx::Point&
> point) {
>   panel_browser_view_->OnTitleBarMousePressed(pressed);
> }
> 
> We need to change the class PanelBrowserFrameView to pass the point directly.

Wouldn't it be better to leave it the way it is in case we need something else
from the MouseEvent object later on?

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.h:28: friend class
NativePanelTestingGtk;
On 2011/08/09 21:10:23, jianli wrote:
> Should it be NativePanelTestingWin? Also it would be better to be moved to
> private section.

Done.

[jianli, 2011-08-09 23:24:16]

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:405:
panel_browser_view_->OnTitleBarMousePressed(pressed);
On 2011/08/09 23:05:04, prasadt wrote:
> On 2011/08/09 21:10:23, jianli wrote:
> > It seems to me that we can refactor PanelBrowserView::OnTitleBarMousePressed
> and
> > other related methods a little bit to simplify the testing code, like:
> > 
> > void PanelBrowserView::OnTitleBarMousePressed(const gfx::Point& point) {
> >   mouse_pressed_ = true;
> >   mouse_pressed_point_ = event.location();
> >   mouse_dragging_ = false;
> > }
> > 
> > void NativePanelTestingWin::TitlebarLeftMouseButtonPressed(const gfx::Point&
> > point) {
> >   panel_browser_view_->OnTitleBarMousePressed(pressed);
> > }
> > 
> > We need to change the class PanelBrowserFrameView to pass the point
directly.
> 
> Wouldn't it be better to leave it the way it is in case we need something else
> from the MouseEvent object later on?

I rather worry about it later since it is more important to reduce the
testing-specific code size by sharing the common code.

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
chrome/browser/ui/panels/native_panel.h:73: virtual void
PressLeftMouseButtonTitlebar(gfx::Point point) = 0;
PressLeftMouseButtonOnTitlebar? Or simply as PressMouseOnTitlebar since we don't
care about right mouse.

Also, might be better to use "const gfx::Point&" for point.

I do not have preference for using either Titlebar or TitleBar. But we need to
have consistent naming in panel code. Could you please clean them up?

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
chrome/browser/ui/panels/native_panel.h:76: virtual void CancelDragTitlebar() =
0;
CancelDraggingTitlebar?

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
chrome/browser/ui/panels/native_panel.h:77: virtual void FinishDragTitlebar() =
0;
ditto.

[prasadt, 2011-08-09 23:49:40]

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:405:
panel_browser_view_->OnTitleBarMousePressed(pressed);
On 2011/08/09 23:24:16, jianli wrote:
> On 2011/08/09 23:05:04, prasadt wrote:
> > On 2011/08/09 21:10:23, jianli wrote:
> > > It seems to me that we can refactor
PanelBrowserView::OnTitleBarMousePressed
> > and
> > > other related methods a little bit to simplify the testing code, like:
> > > 
> > > void PanelBrowserView::OnTitleBarMousePressed(const gfx::Point& point) {
> > >   mouse_pressed_ = true;
> > >   mouse_pressed_point_ = event.location();
> > >   mouse_dragging_ = false;
> > > }
> > > 
> > > void NativePanelTestingWin::TitlebarLeftMouseButtonPressed(const
gfx::Point&
> > > point) {
> > >   panel_browser_view_->OnTitleBarMousePressed(pressed);
> > > }
> > > 
> > > We need to change the class PanelBrowserFrameView to pass the point
> directly.
> > 
> > Wouldn't it be better to leave it the way it is in case we need something
else
> > from the MouseEvent object later on?
> 
> I rather worry about it later since it is more important to reduce the
> testing-specific code size by sharing the common code.

I started making the change and then saw that you're using MouseEvent in
PanelBrowserView::OnTitlebarMousePressed() method implementation:

if (!event.IsOnlyLeftMouseButton())

I can change the other methods but then they won't be consistent in their
signatures.  Let me know what you think.

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
chrome/browser/ui/panels/native_panel.h:73: virtual void
PressLeftMouseButtonTitlebar(gfx::Point point) = 0;
On 2011/08/09 23:24:16, jianli wrote:
> PressLeftMouseButtonOnTitlebar? Or simply as PressMouseOnTitlebar since we
don't
> care about right mouse.

I think its better to be explicit about it.
 
> Also, might be better to use "const gfx::Point&" for point.

Done.

> I do not have preference for using either Titlebar or TitleBar. But we need to
> have consistent naming in panel code. Could you please clean them up?

I can do that in a separate CL.

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
chrome/browser/ui/panels/native_panel.h:76: virtual void CancelDragTitlebar() =
0;
On 2011/08/09 23:24:16, jianli wrote:
> CancelDraggingTitlebar?

I like CancelDrag better.  I'm thinking of Drag as an activity and saying cancel
the drag.

http://codereview.chromium.org/7605009/diff/12/chrome/browser/ui/panels/nativ...
chrome/browser/ui/panels/native_panel.h:77: virtual void FinishDragTitlebar() =
0;
On 2011/08/09 23:24:16, jianli wrote:
> ditto.

See above.

[prasadt, 2011-08-10 23:18:58]

Uploaded a patch to fix the test failing on windows.

[prasadt, 2011-08-11 00:26:41]

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:405:
panel_browser_view_->OnTitleBarMousePressed(pressed);
On 2011/08/09 23:49:41, prasadt wrote:
> On 2011/08/09 23:24:16, jianli wrote:
> > On 2011/08/09 23:05:04, prasadt wrote:
> > > On 2011/08/09 21:10:23, jianli wrote:
> > > > It seems to me that we can refactor
> PanelBrowserView::OnTitleBarMousePressed
> > > and
> > > > other related methods a little bit to simplify the testing code, like:
> > > > 
> > > > void PanelBrowserView::OnTitleBarMousePressed(const gfx::Point& point) {
> > > >   mouse_pressed_ = true;
> > > >   mouse_pressed_point_ = event.location();
> > > >   mouse_dragging_ = false;
> > > > }
> > > > 
> > > > void NativePanelTestingWin::TitlebarLeftMouseButtonPressed(const
> gfx::Point&
> > > > point) {
> > > >   panel_browser_view_->OnTitleBarMousePressed(pressed);
> > > > }
> > > > 
> > > > We need to change the class PanelBrowserFrameView to pass the point
> > directly.
> > > 
> > > Wouldn't it be better to leave it the way it is in case we need something
> else
> > > from the MouseEvent object later on?
> > 
> > I rather worry about it later since it is more important to reduce the
> > testing-specific code size by sharing the common code.
> 
> I started making the change and then saw that you're using MouseEvent in
> PanelBrowserView::OnTitlebarMousePressed() method implementation:
> 
> if (!event.IsOnlyLeftMouseButton())
> 
> I can change the other methods but then they won't be consistent in their
> signatures.  Let me know what you think.

Made the changes following our offline chat.  Please take a look at the latest
changes.  Thanks.

[jianli, 2011-08-11 01:22:54]

Can we also add test cases to cover dragging a panel to swap with more than one
panels on the left or right?

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:77: virtual void FinishDragTitlebar() =
0;
Why do we need to differentiate between ReleaseMouseButtonTitlebar and
FinishDragTitlebar? Does calling ReleaseMouseButtonTitlebar after DragTitlebar
means the dragged is ended? We could also use it to mean that it was click
without dragging if DragTitlebar is not called at all.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:164: int drag_delta_x_;
For struct members, we do not end with "_".

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:166: bool is_big_delta_;  // Drag
big enough to cause shuffling.
Can we get rid of is_big_delta? I think we can get the conclusion by checking if
drag_delta_x_ is really big.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:170: void
TestDragging(std::vector<Panel*>& panels,
We do not like to pass by reference.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:171: const struct DragTestData&
drag_test_data) {
struct is not needed.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:173: for (size_t drag_index = 0;
drag_index < panels.size(); drag_index++) {
We prefer to use ++drag_index with the iterator.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:223: bool is_cancel_drag) {
is_cancel_drag => should_cancel_drag

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:226: for (size_t i = 0; i <
panels.size(); i++)
ditto.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:232: NativePanelTesting*
panel_to_drag_testing =
Memory leak here. Also, the name panel_to_drag_testing sounds quite confusing to
me. How about panel_testing_to_drag?

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:240: if (delta_x || delta_y) {
Please add a comment:
  // Trigger the drag.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:313:
panels.push_back(CreatePanel("PanelTest0", gfx::Rect()));
The creation of 1st panel can be moved into for loop. You can pass the size to
CreatePanel.

[prasadt, 2011-08-11 18:11:11]

I'll add the additional test you asked for in a separate patch.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:77: virtual void FinishDragTitlebar() =
0;
On 2011/08/11 01:22:54, jianli wrote:
> Why do we need to differentiate between ReleaseMouseButtonTitlebar and
> FinishDragTitlebar? Does calling ReleaseMouseButtonTitlebar after DragTitlebar
> means the dragged is ended? We could also use it to mean that it was click
> without dragging if DragTitlebar is not called at all.

On Linux they're two different event handlers.  For platforms where there's no
difference, they'll do the same.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:164: int drag_delta_x_;
On 2011/08/11 01:22:54, jianli wrote:
> For struct members, we do not end with "_".

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:166: bool is_big_delta_;  // Drag
big enough to cause shuffling.
On 2011/08/11 01:22:54, jianli wrote:
> Can we get rid of is_big_delta? I think we can get the conclusion by checking
if
> drag_delta_x_ is really big.

I like being explicit about this at the outset.  Leaving it as it is.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:170: void
TestDragging(std::vector<Panel*>& panels,
On 2011/08/11 01:22:54, jianli wrote:
> We do not like to pass by reference.

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:171: const struct DragTestData&
drag_test_data) {
On 2011/08/11 01:22:54, jianli wrote:
> struct is not needed.

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:173: for (size_t drag_index = 0;
drag_index < panels.size(); drag_index++) {
On 2011/08/11 01:22:54, jianli wrote:
> We prefer to use ++drag_index with the iterator.

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:223: bool is_cancel_drag) {
On 2011/08/11 01:22:54, jianli wrote:
> is_cancel_drag => should_cancel_drag

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:226: for (size_t i = 0; i <
panels.size(); i++)
On 2011/08/11 01:22:54, jianli wrote:
> ditto.

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:232: NativePanelTesting*
panel_to_drag_testing =
On 2011/08/11 01:22:54, jianli wrote:
> Memory leak here. Also, the name panel_to_drag_testing sounds quite confusing
to
> me. How about panel_testing_to_drag?

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:240: if (delta_x || delta_y) {
On 2011/08/11 01:22:54, jianli wrote:
> Please add a comment:
>   // Trigger the drag.

Done.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:313:
panels.push_back(CreatePanel("PanelTest0", gfx::Rect()));
On 2011/08/11 01:22:54, jianli wrote:
> The creation of 1st panel can be moved into for loop. You can pass the size to
> CreatePanel.

I would rather just use the default size than to try to guess.  We could have
different defaults for different platofrms, screen sizes etc.

[jianli, 2011-08-11 20:52:09]

LGTM with some minor nits.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:313:
panels.push_back(CreatePanel("PanelTest0", gfx::Rect()));
On 2011/08/11 18:11:11, prasadt wrote:
> On 2011/08/11 01:22:54, jianli wrote:
> > The creation of 1st panel can be moved into for loop. You can pass the size
to
> > CreatePanel.
> 
> I would rather just use the default size than to try to guess.  We could have
> different defaults for different platofrms, screen sizes etc.

It would be better to pass fixed size to CreatePanel. For now you only have 3
panels and the default panel width is 240 and it is mostly likely been fine for
nowadays screen not smaller than 800x600. However, when you add more panels for
new tests as we discussed, we have to pass specific size in order not to exceed
800x600. Furthermore, I think we should call PanelManager::SetWorkArea to use a
specific work area, like 800x600, for the testing purpose. This will make the
tests far less flaky. You can do it in your next patch to add more test cases.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:376: class NativePanelTestingWin
: public NativePanelTesting {
Please move this subclass to the anonymous namespace.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:391: NativePanelTesting*
NativePanelTesting::Create(NativePanel* native_panel) {
Please add the comment like the following:
  // static

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:170: //
NativePanelTesting implementation.
Please add the comment like the following:
  // static

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:319: class
NativePanelTestingGtk : public NativePanelTesting {
Please move this subclass to the anonymous namespace.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:335: NativePanelTesting*
NativePanelTesting::Create(NativePanel* native_panel) {
Please add the comment for indicate it is static.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:177: for (size_t j = 0; j <
panels->size(); ++j) {
Probably more efficient to say:
  size_t num_panels = panels->size();
  for (...) {
    std::vector<int> expected_delta_x_after_drag(num_panels, 0);
    std::vector<int> expected_delta_x_after_finish(num_panels, 0);

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:240: if (delta_x || delta_y) {
We can remove this if test since we do not have this test case.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:251: // Cancel the dragging if
asked.
To be consistent with other comment: dragging => drag

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:267: delete panel_testing_to_drag;
Better to use scoped_ptr, instead of self-deleting.

[prasadt, 2011-08-11 21:07:56]

Addressed the issues.  Will commit in a bit.

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7605009/diff/14001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:313:
panels.push_back(CreatePanel("PanelTest0", gfx::Rect()));
On 2011/08/11 20:52:09, jianli wrote:
> On 2011/08/11 18:11:11, prasadt wrote:
> > On 2011/08/11 01:22:54, jianli wrote:
> > > The creation of 1st panel can be moved into for loop. You can pass the
size
> to
> > > CreatePanel.
> > 
> > I would rather just use the default size than to try to guess.  We could
have
> > different defaults for different platofrms, screen sizes etc.
> 
> It would be better to pass fixed size to CreatePanel. For now you only have 3
> panels and the default panel width is 240 and it is mostly likely been fine
for
> nowadays screen not smaller than 800x600. However, when you add more panels
for
> new tests as we discussed, we have to pass specific size in order not to
exceed
> 800x600. Furthermore, I think we should call PanelManager::SetWorkArea to use
a
> specific work area, like 800x600, for the testing purpose. This will make the
> tests far less flaky. You can do it in your next patch to add more test cases.

Will look at doing this in my next patch.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:376: class NativePanelTestingWin
: public NativePanelTesting {
On 2011/08/11 20:52:09, jianli wrote:
> Please move this subclass to the anonymous namespace.

Need this class in header file.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:391: NativePanelTesting*
NativePanelTesting::Create(NativePanel* native_panel) {
On 2011/08/11 20:52:09, jianli wrote:
> Please add the comment like the following:
>   // static

Done.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:170: //
NativePanelTesting implementation.
On 2011/08/11 20:52:09, jianli wrote:
> Please add the comment like the following:
>   // static

Done.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:319: class
NativePanelTestingGtk : public NativePanelTesting {
On 2011/08/11 20:52:09, jianli wrote:
> Please move this subclass to the anonymous namespace.

Need it in header file.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:335: NativePanelTesting*
NativePanelTesting::Create(NativePanel* native_panel) {
On 2011/08/11 20:52:09, jianli wrote:
> Please add the comment for indicate it is static.

Done.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:177: for (size_t j = 0; j <
panels->size(); ++j) {
On 2011/08/11 20:52:09, jianli wrote:
> Probably more efficient to say:
>   size_t num_panels = panels->size();
>   for (...) {
>     std::vector<int> expected_delta_x_after_drag(num_panels, 0);
>     std::vector<int> expected_delta_x_after_finish(num_panels, 0);

Done.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:240: if (delta_x || delta_y) {
On 2011/08/11 20:52:09, jianli wrote:
> We can remove this if test since we do not have this test case.

Done.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:251: // Cancel the dragging if
asked.
On 2011/08/11 20:52:09, jianli wrote:
> To be consistent with other comment: dragging => drag

Done.

http://codereview.chromium.org/7605009/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:267: delete panel_testing_to_drag;
On 2011/08/11 20:52:09, jianli wrote:
> Better to use scoped_ptr, instead of self-deleting.

Done.