Support reordering of Browser Actions within the container. Currently does no...

Support reordering of Browser Actions within the container. Currently does not support dragging to/from the chevron menu.

Also fixed two bugs in the same code:
- the container would be 0 width if a value for it hasn't been saved (part of bug 32101).
- the default icon was not used when a tab specific icon was not found (bug 34317).

BUG=http://crbug.com/26990, http://crbug.com/32101, http://crbug.com/34317
TEST=In both LTR and RTL locale, try reordering the browser actions and make sure to test dragging to the ends with and without a chevron visible. Install Send to Gmail extension and make sure it has an icon while in the overflow menu.

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

[sky, 2010-02-02 18:54:30]

http://codereview.chromium.org/549224/diff/2004/2008
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/549224/diff/2004/2008#newcode342
chrome/browser/views/browser_actions_container.cc:342: int icon_count = 0;
Seems lame to do do this. Could you add a size to ExtensionToolbarModel.

http://codereview.chromium.org/549224/diff/2004/2008#newcode397
chrome/browser/views/browser_actions_container.cc:397:
DCHECK(browser_action_views_.size() == 0u);
empty()

http://codereview.chromium.org/549224/diff/2004/2008#newcode803
chrome/browser/views/browser_actions_container.cc:803: 
nit: nuke this line.

http://codereview.chromium.org/549224/diff/2004/2005
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/549224/diff/2004/2005#newcode53
chrome/browser/views/browser_actions_container.h:53: SkBitmap default_icon() {
return default_icon_; }
const SkBitmap&

http://codereview.chromium.org/549224/diff/2004/2005#newcode283
chrome/browser/views/browser_actions_container.h:283: virtual int
OnPerformDrop(const views::DropTargetEvent& event);
Because you need the data in CanDrop you need to override AreDropTypesRequired
to return true. It won't matter on the windows side, but it will on chrome os.

http://codereview.chromium.org/549224/diff/2004/2006
File chrome/browser/views/extensions/browser_action_drag_data.cc (right):

http://codereview.chromium.org/549224/diff/2004/2006#newcode16
chrome/browser/views/extensions/browser_action_drag_data.cc:16: : button_(NULL),
index_(-1) {
each initializer one on it's own line.

http://codereview.chromium.org/549224/diff/2004/2006#newcode21
chrome/browser/views/extensions/browser_action_drag_data.cc:21: :
button_(button), index_(index) {
each initializer one on it's own line.

http://codereview.chromium.org/549224/diff/2004/2006#newcode27
chrome/browser/views/extensions/browser_action_drag_data.cc:27: #if
defined(WCHAR_T_IS_UTF16)
You should push this non-sense to Pickle or FilePath. When I wrote the code in
bookmark drop data there was only wstring, someone else added the ifdefs.

http://codereview.chromium.org/549224/diff/2004/2006#newcode91
chrome/browser/views/extensions/browser_action_drag_data.cc:91: if
(!pickle->ReadWString(&data_iterator, &profile_path_))
You should push this non-sense to Pickle or FilePath. When I wrote the code in
bookmark drop data there only wstring, someone else added the ifdefs.

http://codereview.chromium.org/549224/diff/2004/2007
File chrome/browser/views/extensions/browser_action_drag_data.h (right):

http://codereview.chromium.org/549224/diff/2004/2007#newcode25
chrome/browser/views/extensions/browser_action_drag_data.h:25:
BrowserActionButton* button() { return button_; }
const

http://codereview.chromium.org/549224/diff/2004/2007#newcode27
chrome/browser/views/extensions/browser_action_drag_data.h:27: size_t index() {
return index_; }
const

http://codereview.chromium.org/549224/diff/2004/2007#newcode48
chrome/browser/views/extensions/browser_action_drag_data.h:48: std::wstring
profile_path_;
This should be FilePath::StringType.

http://codereview.chromium.org/549224/diff/2004/2007#newcode54
chrome/browser/views/extensions/browser_action_drag_data.h:54:
BrowserActionButton* button_;
This worries me, and it looks like you only need it for a DCHECK. Couldn't you
write some extension id rather than the actual button itself?

[Finnur, 2010-02-02 22:12:39]

As requested...

On 2010/02/02 18:54:30, sky wrote:
> http://codereview.chromium.org/549224/diff/2004/2008
> File chrome/browser/views/browser_actions_container.cc (right):
> 
> http://codereview.chromium.org/549224/diff/2004/2008#newcode342
> chrome/browser/views/browser_actions_container.cc:342: int icon_count = 0;
> Seems lame to do do this. Could you add a size to ExtensionToolbarModel.
> 
> http://codereview.chromium.org/549224/diff/2004/2008#newcode397
> chrome/browser/views/browser_actions_container.cc:397:
> DCHECK(browser_action_views_.size() == 0u);
> empty()
> 
> http://codereview.chromium.org/549224/diff/2004/2008#newcode803
> chrome/browser/views/browser_actions_container.cc:803: 
> nit: nuke this line.
> 
> http://codereview.chromium.org/549224/diff/2004/2005
> File chrome/browser/views/browser_actions_container.h (right):
> 
> http://codereview.chromium.org/549224/diff/2004/2005#newcode53
> chrome/browser/views/browser_actions_container.h:53: SkBitmap default_icon() {
> return default_icon_; }
> const SkBitmap&
> 
> http://codereview.chromium.org/549224/diff/2004/2005#newcode283
> chrome/browser/views/browser_actions_container.h:283: virtual int
> OnPerformDrop(const views::DropTargetEvent& event);
> Because you need the data in CanDrop you need to override AreDropTypesRequired
> to return true. It won't matter on the windows side, but it will on chrome os.
> 
> http://codereview.chromium.org/549224/diff/2004/2006
> File chrome/browser/views/extensions/browser_action_drag_data.cc (right):
> 
> http://codereview.chromium.org/549224/diff/2004/2006#newcode16
> chrome/browser/views/extensions/browser_action_drag_data.cc:16: :
button_(NULL),
> index_(-1) {
> each initializer one on it's own line.
> 
> http://codereview.chromium.org/549224/diff/2004/2006#newcode21
> chrome/browser/views/extensions/browser_action_drag_data.cc:21: :
> button_(button), index_(index) {
> each initializer one on it's own line.
> 
> http://codereview.chromium.org/549224/diff/2004/2006#newcode27
> chrome/browser/views/extensions/browser_action_drag_data.cc:27: #if
> defined(WCHAR_T_IS_UTF16)
> You should push this non-sense to Pickle or FilePath. When I wrote the code in
> bookmark drop data there was only wstring, someone else added the ifdefs.
> 
> http://codereview.chromium.org/549224/diff/2004/2006#newcode91
> chrome/browser/views/extensions/browser_action_drag_data.cc:91: if
> (!pickle->ReadWString(&data_iterator, &profile_path_))
> You should push this non-sense to Pickle or FilePath. When I wrote the code in
> bookmark drop data there only wstring, someone else added the ifdefs.
> 
> http://codereview.chromium.org/549224/diff/2004/2007
> File chrome/browser/views/extensions/browser_action_drag_data.h (right):
> 
> http://codereview.chromium.org/549224/diff/2004/2007#newcode25
> chrome/browser/views/extensions/browser_action_drag_data.h:25:
> BrowserActionButton* button() { return button_; }
> const
> 
> http://codereview.chromium.org/549224/diff/2004/2007#newcode27
> chrome/browser/views/extensions/browser_action_drag_data.h:27: size_t index()
{
> return index_; }
> const
> 
> http://codereview.chromium.org/549224/diff/2004/2007#newcode48
> chrome/browser/views/extensions/browser_action_drag_data.h:48: std::wstring
> profile_path_;
> This should be FilePath::StringType.
> 
> http://codereview.chromium.org/549224/diff/2004/2007#newcode54
> chrome/browser/views/extensions/browser_action_drag_data.h:54:
> BrowserActionButton* button_;
> This worries me, and it looks like you only need it for a DCHECK. Couldn't you
> write some extension id rather than the actual button itself?

[sky, 2010-02-02 22:16:02]

LGTM