Review Request: fix issue 6223 -- issues on drag/drop tab

This CL fixes issue 6223 (comment #10 and #15).

There are the following 3 issues:
1. the tab image in gray box is wrong in RTL locale. This is fixed by flipping canvas in RTL locale when draw those images.
2. dropped tab was not rendered in the correct position when gray box appears but the tab was not dropped inside the gray box. This is fixed by adjusting x coordinate of the dropped tab when there is dock information available in RTL in CompleteDrag().
3. when chrome is maximized, drag/drop a tab to left/right window, then drag the tab through original main browser tabstrip and drop it anywhere else, dropped tab was not rendered in the right position. This is fixed by initialize window create point based on source_tabstrip_, not attached_tabstrip_, and move the initialization function from Attach() (since the position is no longer related to attached_tabstrip_) to CaptureDragInfo() where the mouse_offset_ is set.

BUG=http://crbug.com/6223
TEST=
1. Open Hebrew Chrome, 
2. drag a tab to the right of the screen till the gray box showed up, the tab image showed in the gray box should be similar to the tab image in Hebrew Chrome (not that in English Chrome), 
3. drop the tab outside of the gray box while gray box is showing, the dropped tab should be rendered using the mouse click as the top-right corner (not top-left corner).
4. maximize chrome and open at least 2 tabs in it.
5. drag a tab out to left screen till the "left-half of screen" gray box showed up and drop it inside the gray box so that the tab was in the left-half of the screen and the original chrome browser is still maximized.
6. drag this tab back to the title of chrome (as if to re-insert it back to the maximized chrome's tabstrip) but do not release mouse, continue drag it back to somewhere on the screen and drop it. The dropped tab should be rendered using the mouse click as a point in its tabstrip (not rendered far away from the mouse click).



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

[jungshik at Google, 2009-06-11 22:19:31]

I'm deferring this to sky.

[jungshik at Google, 2009-06-11 22:26:18]

http://codereview.chromium.org/126006/diff/5/6
File chrome/browser/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/126006/diff/5/6#newcode496
Line 496: // are get from source_tabstrip_. So, we need to get the first_tab
based on
nit: are get from => are obtained from ?

[sky, 2009-06-11 22:39:43]

http://codereview.chromium.org/126006/diff/5/6
File chrome/browser/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/126006/diff/5/6#newcode500
Line 500: Tab* first_tab = source_tabstrip_->GetTabAt(0);
Are you sure we want tab 0 here? I would think we want the index of the tab the
user is dragging.

[xji, 2009-06-11 22:45:05]

http://codereview.chromium.org/126006/diff/5/6
File chrome/browser/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/126006/diff/5/6#newcode500
Line 500: Tab* first_tab = source_tabstrip_->GetTabAt(0);
I had a doubt on this too but I did not change it since it was originally there.
Let me check on it. (it will be slow since I am sync/rebuilding now)

On 2009/06/11 22:39:43, sky wrote:
> Are you sure we want tab 0 here? I would think we want the index of the tab
the
> user is dragging.

[xji, 2009-06-12 19:22:05]

Yes, we should use tab 0 here.
window_create_point_ is used to position *detached* tab, it is the distance of
the mouse from the top left of the *dragged* tab (which should be only 1 dragged
tab). It is the same distance of the mouse from the top left of the *first* tab
in the *source(attached)* tab.
(there is comment about it in the include file).

Using the index of the tab the user is dragging will cause position shift.

On 2009/06/11 22:39:43, sky wrote:
> http://codereview.chromium.org/126006/diff/5/6
> File chrome/browser/views/tabs/dragged_tab_controller.cc (right):
> 
> http://codereview.chromium.org/126006/diff/5/6#newcode500
> Line 500: Tab* first_tab = source_tabstrip_->GetTabAt(0);
> Are you sure we want tab 0 here? I would think we want the index of the tab
the
> user is dragging.

[sky, 2009-06-12 20:27:12]

That makes sense. Thanks for the clarification.

LGTM.

  -Scott

On 2009/06/12 19:22:05, xji wrote:
> Yes, we should use tab 0 here.
> window_create_point_ is used to position *detached* tab, it is the distance of
> the mouse from the top left of the *dragged* tab (which should be only 1
dragged
> tab). It is the same distance of the mouse from the top left of the *first*
tab
> in the *source(attached)* tab.
> (there is comment about it in the include file).
> 
> Using the index of the tab the user is dragging will cause position shift.
> 
> On 2009/06/11 22:39:43, sky wrote:
> > http://codereview.chromium.org/126006/diff/5/6
> > File chrome/browser/views/tabs/dragged_tab_controller.cc (right):
> > 
> > http://codereview.chromium.org/126006/diff/5/6#newcode500
> > Line 500: Tab* first_tab = source_tabstrip_->GetTabAt(0);
> > Are you sure we want tab 0 here? I would think we want the index of the tab
> the
> > user is dragging.

[jeremy, 2009-06-12 21:08:25]

very nice!

http://codereview.chromium.org/126006/diff/5/6
File chrome/browser/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/126006/diff/5/6#newcode82
Line 82: int x_of_inactive_tab;
For the sake of being defensive, could you initialize these to -1 here?

http://codereview.chromium.org/126006/diff/5/6#newcode495
Line 495: // attached_tabstrip_ is NULL and all the window's related bound
information
attached -> the attached

http://codereview.chromium.org/126006/diff/5/6#newcode496
Line 496: // are get from source_tabstrip_. So, we need to get the first_tab
based on
Think you missed this comment?

http://codereview.chromium.org/126006/diff/5/6#newcode1090
Line 1090: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT &&
Could you change this comment to point to the bug, something like:
// When modifying this code, make sure not to introduce <bug url>

Also I think this would be a little easier to read if you decomposed a bit:
bool is_rtl_ui = ...
bool not_docked = ...

if (is_rtl_ui && not_docked) {
// Mirror X axis.
...
}

That way you also wouldn't need the comment, since it would be clear from the
code.

[idana, 2009-06-12 22:06:49]

LGTM (assuming the things Jeremy pointed out are addressed)

[xji, 2009-06-12 22:40:23]

done

On 2009/06/11 22:26:18, Jungshik Shin wrote:
> http://codereview.chromium.org/126006/diff/5/6
> File chrome/browser/views/tabs/dragged_tab_controller.cc (right):
> 
> http://codereview.chromium.org/126006/diff/5/6#newcode496
> Line 496: // are get from source_tabstrip_. So, we need to get the first_tab
> based on
> nit: are get from => are obtained from ?

[xji, 2009-06-12 22:41:03]

all done.


On 2009/06/12 21:08:25, jeremy wrote:
> very nice!
> 
> http://codereview.chromium.org/126006/diff/5/6
> File chrome/browser/views/tabs/dragged_tab_controller.cc (right):
> 
> http://codereview.chromium.org/126006/diff/5/6#newcode82
> Line 82: int x_of_inactive_tab;
> For the sake of being defensive, could you initialize these to -1 here?
> 
> http://codereview.chromium.org/126006/diff/5/6#newcode495
> Line 495: // attached_tabstrip_ is NULL and all the window's related bound
> information
> attached -> the attached
> 
> http://codereview.chromium.org/126006/diff/5/6#newcode496
> Line 496: // are get from source_tabstrip_. So, we need to get the first_tab
> based on
> Think you missed this comment?
> 
> http://codereview.chromium.org/126006/diff/5/6#newcode1090
> Line 1090: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT &&
> Could you change this comment to point to the bug, something like:
> // When modifying this code, make sure not to introduce <bug url>
> 
> Also I think this would be a little easier to read if you decomposed a bit:
> bool is_rtl_ui = ...
> bool not_docked = ...
> 
> if (is_rtl_ui && not_docked) {
> // Mirror X axis.
> ...
> }
> 
> That way you also wouldn't need the comment, since it would be clear from the
> code.

[jeremy, 2009-06-12 22:45:50]

LGTM