Move app list

Move app list


BUG=141378
TEST=app list icon should always be leftmost

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156092

[sky, 2012-09-11 00:18:32]

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_view.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view.cc:445:
view_model_->set_ideal_bounds(last_view_index, last_view_bounds);
Is this code needed anymore (438 and 441-445)? It was necessary when the app
list button was at the end and we were forcing moving it's visible position.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view.cc:727: if (modified_index ==
view_model_->view_size())
Your comment implies that if 727 is true then we're adding a new item. But isn't
727 only true if adding an item at the end?

[Mattias Nissler (ping if slow), 2012-09-11 08:42:30]

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_model_unittest.cc:203: // Browser shortcut and app list
should still be first and last, respectively.
This comment is stale.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_navigator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_navigator_unittest.cc:63: // beginning, so '2' refers the
first TYPE_TABBED item.
comment stale.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_navigator_unittest.cc:79: // Second one.  It starts a the
end, wraps to the beginning, and skips
s/a/at/

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_view_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view_unittest.cc:262: continue;
seems like this would be easier if we just had a NULL entry for the app list in
id_map?

[DaveMoore, 2012-09-11 17:07:00]

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_model_unittest.cc:203: // Browser shortcut and app list
should still be first and last, respectively.
On 2012/09/11 08:42:30, Mattias Nissler wrote:
> This comment is stale.

Done.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_navigator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_navigator_unittest.cc:63: // beginning, so '2' refers the
first TYPE_TABBED item.
On 2012/09/11 08:42:30, Mattias Nissler wrote:
> comment stale.

Done.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_navigator_unittest.cc:79: // Second one.  It starts a the
end, wraps to the beginning, and skips
On 2012/09/11 08:42:30, Mattias Nissler wrote:
> s/a/at/

Done.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_view.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view.cc:445:
view_model_->set_ideal_bounds(last_view_index, last_view_bounds);
Removed
On 2012/09/11 00:18:32, sky wrote:
> Is this code needed anymore (438 and 441-445)? It was necessary when the app
> list button was at the end and we were forcing moving it's visible position.

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view.cc:727: if (modified_index ==
view_model_->view_size())
Updated comment. There's a case that existed before where we return -1 if we're
adding a new view to the middle while dragging. @mattias, I believe this is your
code. Should it be -1?

On 2012/09/11 00:18:32, sky wrote:
> Your comment implies that if 727 is true then we're adding a new item. But
isn't
> 727 only true if adding an item at the end?

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_view_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view_unittest.cc:262: continue;
On 2012/09/11 08:42:30, Mattias Nissler wrote:
> seems like this would be easier if we just had a NULL entry for the app list
in
> id_map?

Done.
Also refactored so it's easier to change where the app list is.

[sky, 2012-09-11 18:13:29]

LGTM

[Mattias Nissler (ping if slow), 2012-09-12 12:08:18]

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
File ash/launcher/launcher_view.cc (right):

https://chromiumcodereview.appspot.com/10905201/diff/2001/ash/launcher/launch...
ash/launcher/launcher_view.cc:727: if (modified_index ==
view_model_->view_size())
On 2012/09/11 17:07:00, DaveMoore wrote:
> Updated comment. There's a case that existed before where we return -1 if
we're
> adding a new view to the middle while dragging. @mattias, I believe this is
your
> code. Should it be -1?

I fail to follow the case you're describing. If we're inserting something to the
middle, we have 0 <= modified_index < view_model_->view_size(), and the code
below will not return -1.

The return value should be the new position of the view previously at
modified_index.

The check you added is required good to cover the case of appending, in which
case we would have incorrectly returned -1, whereas the insertion position after
cancelling the drag is still at the end of the model. This hasn't been a problem
thus far since the app list was last, so we never appended.

I suggest you make the check a >= though (which should never happen, but
returning -1 in these cases is certainly not correct).

Regarding the code below, would you mind renaming |removed_view| to
|modified_view|? I should have changed it when moving the code but obviously
missed it.

> 
> On 2012/09/11 00:18:32, sky wrote:
> > Your comment implies that if 727 is true then we're adding a new item. But
> isn't
> > 727 only true if adding an item at the end?
>