Added Star button to compact navigation mode....

Added Star button to compact navigation mode.

BUG=32326
TEST=Switch Chrome to compact mode by pressing Ctrl-Shift-C. Test that Star
icon works.


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

[whywhat, 2010-02-03 23:54:18]

I think I finally got it working. Could you take a look?

[oshima, 2010-02-04 00:32:34]

thanks. change looks fine. monior cosmetic comments.

http://codereview.chromium.org/543215/diff/6001/7002
File chrome/browser/chromeos/compact_location_bar_view.cc (right):

http://codereview.chromium.org/543215/diff/6001/7002#newcode233
chrome/browser/chromeos/compact_location_bar_view.cc:233: OSExchangeData* data)
{
indent

http://codereview.chromium.org/543215/diff/6001/7002#newcode235
chrome/browser/chromeos/compact_location_bar_view.cc:235:
GetDragOperations(sender, press_x, press_y) != DragDropTypes::DRAG_NONE);
I feel a bit uneasy to call non const function in DCHECK.
I don't have good suggestions tho.

http://codereview.chromium.org/543215/diff/6001/7002#newcode256
chrome/browser/chromeos/compact_location_bar_view.cc:256:
UTF16ToWideHack(tab->GetTitle()),
too bad that drag util uses wstring instead of string16...

http://codereview.chromium.org/543215/diff/6001/7006
File chrome/browser/views/toolbar_star_toggle.h (right):

http://codereview.chromium.org/543215/diff/6001/7006#newcode62
chrome/browser/views/toolbar_star_toggle.h:62: Profile* profile_;
emtpy line here

http://codereview.chromium.org/543215/diff/6001/7006#newcode64
chrome/browser/views/toolbar_star_toggle.h:64: views::View* host_view_;
and here.

http://codereview.chromium.org/543215/diff/6001/7004
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/543215/diff/6001/7004#newcode7
chrome/browser/views/toolbar_view.cc:7: #include <algorithm>
do you need this?

http://codereview.chromium.org/543215/diff/6001/7003
File chrome/browser/views/toolbar_view.h (right):

http://codereview.chromium.org/543215/diff/6001/7003#newcode114
chrome/browser/views/toolbar_view.h:114: // Overriden from
ToolbarStarToggle::Delegate:
I think you changed the impl and you don't need this any more?

[whywhat, 2010-02-04 00:59:02]

Maybe we should just drop drag-n-drop support for now? DragController overrides
would be just stubs then.

http://codereview.chromium.org/543215/diff/6001/7002
File chrome/browser/chromeos/compact_location_bar_view.cc (right):

http://codereview.chromium.org/543215/diff/6001/7002#newcode233
chrome/browser/chromeos/compact_location_bar_view.cc:233: OSExchangeData* data)
{
On 2010/02/04 00:32:34, oshima wrote:
> indent

Done.

http://codereview.chromium.org/543215/diff/6001/7006
File chrome/browser/views/toolbar_star_toggle.h (right):

http://codereview.chromium.org/543215/diff/6001/7006#newcode62
chrome/browser/views/toolbar_star_toggle.h:62: Profile* profile_;
On 2010/02/04 00:32:34, oshima wrote:
> emtpy line here

Done.

http://codereview.chromium.org/543215/diff/6001/7006#newcode64
chrome/browser/views/toolbar_star_toggle.h:64: views::View* host_view_;
On 2010/02/04 00:32:34, oshima wrote:
> and here.

Done.

http://codereview.chromium.org/543215/diff/6001/7004
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/543215/diff/6001/7004#newcode7
chrome/browser/views/toolbar_view.cc:7: #include <algorithm>
On 2010/02/04 00:32:34, oshima wrote:
> do you need this?

Yes, I fixed this because lint complained. std::max is used somewhere in the
file.

http://codereview.chromium.org/543215/diff/6001/7003
File chrome/browser/views/toolbar_view.h (right):

http://codereview.chromium.org/543215/diff/6001/7003#newcode114
chrome/browser/views/toolbar_view.h:114: // Overriden from
ToolbarStarToggle::Delegate:
On 2010/02/04 00:32:34, oshima wrote:
> I think you changed the impl and you don't need this any more?

Done.

[whywhat, 2010-02-04 05:25:16]

I tested Drag and drop on this star button: dragging surely works but dropping
doesn't since when you click on a link, compact location bar is hidden because
it looses the focus.

I also noticed that star button puts the bookmark on Bookmarks bar, but the bar
is absent in compact mode. 

On 2010/02/04 00:59:02, whywhat wrote:
> Maybe we should just drop drag-n-drop support for now? DragController
overrides
> would be just stubs then.
> 
> http://codereview.chromium.org/543215/diff/6001/7002
> File chrome/browser/chromeos/compact_location_bar_view.cc (right):
> 
> http://codereview.chromium.org/543215/diff/6001/7002#newcode233
> chrome/browser/chromeos/compact_location_bar_view.cc:233: OSExchangeData*
data)
> {
> On 2010/02/04 00:32:34, oshima wrote:
> > indent
> 
> Done.
> 
> http://codereview.chromium.org/543215/diff/6001/7006
> File chrome/browser/views/toolbar_star_toggle.h (right):
> 
> http://codereview.chromium.org/543215/diff/6001/7006#newcode62
> chrome/browser/views/toolbar_star_toggle.h:62: Profile* profile_;
> On 2010/02/04 00:32:34, oshima wrote:
> > emtpy line here
> 
> Done.
> 
> http://codereview.chromium.org/543215/diff/6001/7006#newcode64
> chrome/browser/views/toolbar_star_toggle.h:64: views::View* host_view_;
> On 2010/02/04 00:32:34, oshima wrote:
> > and here.
> 
> Done.
> 
> http://codereview.chromium.org/543215/diff/6001/7004
> File chrome/browser/views/toolbar_view.cc (right):
> 
> http://codereview.chromium.org/543215/diff/6001/7004#newcode7
> chrome/browser/views/toolbar_view.cc:7: #include <algorithm>
> On 2010/02/04 00:32:34, oshima wrote:
> > do you need this?
> 
> Yes, I fixed this because lint complained. std::max is used somewhere in the
> file.
> 
> http://codereview.chromium.org/543215/diff/6001/7003
> File chrome/browser/views/toolbar_view.h (right):
> 
> http://codereview.chromium.org/543215/diff/6001/7003#newcode114
> chrome/browser/views/toolbar_view.h:114: // Overriden from
> ToolbarStarToggle::Delegate:
> On 2010/02/04 00:32:34, oshima wrote:
> > I think you changed the impl and you don't need this any more?
> 
> Done.

[oshima, 2010-02-04 07:24:23]

On Wed, Feb 3, 2010 at 9:25 PM, <avayvod@chromium.org> wrote:

> I tested Drag and drop on this star button: dragging surely works but
> dropping
> doesn't since when you click on a link, compact location bar is hidden
> because
> it looses the focus.
>
I checked in the fix today so please sync and try again. it worked for me.


>
> I also noticed that star button puts the bookmark on Bookmarks bar, but the
> bar
> is absent in compact mode.
>
> compact mode only turns off tool bar. If you have checked off "always show
bookmark bar",
you get a bookmark bar only in new tab page.



>
> On 2010/02/04 00:59:02, whywhat wrote:
>
>> Maybe we should just drop drag-n-drop support for now? DragController
>>
> overrides
>
>> would be just stubs then.
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7002
>> File chrome/browser/chromeos/compact_location_bar_view.cc (right):
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7002#newcode233
>> chrome/browser/chromeos/compact_location_bar_view.cc:233: OSExchangeData*
>>
> data)
>
>> {
>> On 2010/02/04 00:32:34, oshima wrote:
>> > indent
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7006
>> File chrome/browser/views/toolbar_star_toggle.h (right):
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7006#newcode62
>> chrome/browser/views/toolbar_star_toggle.h:62: Profile* profile_;
>> On 2010/02/04 00:32:34, oshima wrote:
>> > emtpy line here
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7006#newcode64
>> chrome/browser/views/toolbar_star_toggle.h:64: views::View* host_view_;
>> On 2010/02/04 00:32:34, oshima wrote:
>> > and here.
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7004
>> File chrome/browser/views/toolbar_view.cc (right):
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7004#newcode7
>> chrome/browser/views/toolbar_view.cc:7: #include <algorithm>
>> On 2010/02/04 00:32:34, oshima wrote:
>> > do you need this?
>>
>
>  Yes, I fixed this because lint complained. std::max is used somewhere in
>> the
>> file.
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7003
>> File chrome/browser/views/toolbar_view.h (right):
>>
>
>  http://codereview.chromium.org/543215/diff/6001/7003#newcode114
>> chrome/browser/views/toolbar_view.h:114: // Overriden from
>> ToolbarStarToggle::Delegate:
>> On 2010/02/04 00:32:34, oshima wrote:
>> > I think you changed the impl and you don't need this any more?
>>
>
>  Done.
>>
>
>
>
> http://codereview.chromium.org/543215
>

[oshima, 2010-02-04 07:26:52]

LGTM


On 2010/02/04 05:25:16, whywhat wrote:
> I tested Drag and drop on this star button: dragging surely works but dropping
> doesn't since when you click on a link, compact location bar is hidden because
> it looses the focus.
> 
> I also noticed that star button puts the bookmark on Bookmarks bar, but the
bar
> is absent in compact mode. 
> 
> On 2010/02/04 00:59:02, whywhat wrote:
> > Maybe we should just drop drag-n-drop support for now? DragController
> overrides
> > would be just stubs then.
> > 
> > http://codereview.chromium.org/543215/diff/6001/7002
> > File chrome/browser/chromeos/compact_location_bar_view.cc (right):
> > 
> > http://codereview.chromium.org/543215/diff/6001/7002#newcode233
> > chrome/browser/chromeos/compact_location_bar_view.cc:233: OSExchangeData*
> data)
> > {
> > On 2010/02/04 00:32:34, oshima wrote:
> > > indent
> > 
> > Done.
> > 
> > http://codereview.chromium.org/543215/diff/6001/7006
> > File chrome/browser/views/toolbar_star_toggle.h (right):
> > 
> > http://codereview.chromium.org/543215/diff/6001/7006#newcode62
> > chrome/browser/views/toolbar_star_toggle.h:62: Profile* profile_;
> > On 2010/02/04 00:32:34, oshima wrote:
> > > emtpy line here
> > 
> > Done.
> > 
> > http://codereview.chromium.org/543215/diff/6001/7006#newcode64
> > chrome/browser/views/toolbar_star_toggle.h:64: views::View* host_view_;
> > On 2010/02/04 00:32:34, oshima wrote:
> > > and here.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/543215/diff/6001/7004
> > File chrome/browser/views/toolbar_view.cc (right):
> > 
> > http://codereview.chromium.org/543215/diff/6001/7004#newcode7
> > chrome/browser/views/toolbar_view.cc:7: #include <algorithm>
> > On 2010/02/04 00:32:34, oshima wrote:
> > > do you need this?
> > 
> > Yes, I fixed this because lint complained. std::max is used somewhere in the
> > file.
> > 
> > http://codereview.chromium.org/543215/diff/6001/7003
> > File chrome/browser/views/toolbar_view.h (right):
> > 
> > http://codereview.chromium.org/543215/diff/6001/7003#newcode114
> > chrome/browser/views/toolbar_view.h:114: // Overriden from
> > ToolbarStarToggle::Delegate:
> > On 2010/02/04 00:32:34, oshima wrote:
> > > I think you changed the impl and you don't need this any more?
> > 
> > Done.