AppListModel / AppsGridView: Better error handling for corner cases.

AppListModel / AppsGridView: Better error handling for corner cases.

Fixes a potential crash if an app gets duplicated in the app list, and
the user tries to drop it onto itself. (There is no known way to repro
this currently, but there was recently; see http://crbug.com/415530.)
That situation will now result in a very descriptive DCHECK, rather than
a confusing low-level CHECK fail.

AppListModel::MergeItems:
- returns "" instead of crashing if the source and target IDs are equal.
- returns "" instead of crashing if the target ID is inside a folder (no
  longer needs to explicitly CHECK whether it is in a folder.)

Added plenty of new tests for the various corner cases in
AppListModel::MergeItems which would have caught these crashes.

AppsGridView::MoveItemToFolder:
- DCHECKs for a NULL result of GetViewAtSlotOnCurrentPage.
- DCHECKs that you aren't dropping an item onto itself (this should
  never happen).

BUG=417482
Committed: https://crrev.com/aebcdf88e420f281c950d741106e25dcdf0625e3
Cr-Commit-Position: refs/heads/master@{#297356}

[Matt Giuca, 2014-09-25 08:28:52]

mgiuca@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2014-09-25 08:35:53]

mgiuca@chromium.org changed reviewers:
+ calamity@chromium.org
- mgiuca@chromium.org

[Matt Giuca, 2014-09-25 08:35:53]

Reassigning reviewer. mgiuca was not appropriate.

[calamity, 2014-09-26 03:55:18]

lgtm

https://codereview.chromium.org/600393002/diff/1/ui/app_list/app_list_model.cc
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/600393002/diff/1/ui/app_list/app_list_model.c...
ui/app_list/app_list_model.cc:96: LOG(WARNING) << "MergeItems tried to drop item
onto itself ("
These can probably stand to be DCHECKs.

https://codereview.chromium.org/600393002/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/600393002/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:1741: // item.
(e.g a folder drop does not clean up properly)

[Matt Giuca, 2014-09-29 03:05:03]

https://codereview.chromium.org/600393002/diff/1/ui/app_list/app_list_model.cc
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/600393002/diff/1/ui/app_list/app_list_model.c...
ui/app_list/app_list_model.cc:96: LOG(WARNING) << "MergeItems tried to drop item
onto itself ("
Hmm. If this is a DCHECK and the one in apps_grid_view is also a DCHECK, it
means that in a Release build we will hit the same crash as before (RemoveItem)
if it is ever possible to duplicate an item in the launcher.

I think we should either keep it as-is (as a compile-time checked warning and
return ""), or make one of those a CHECK so that stack traces show the true
cause rather than an undiagnosable "RemoveItem: Item not found".

Which of these should we do?

https://codereview.chromium.org/600393002/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/600393002/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:1741: // item.
On 2014/09/26 03:55:18, calamity wrote:
> (e.g a folder drop does not clean up properly)

Done.

[Matt Giuca, 2014-09-29 03:34:35]

https://codereview.chromium.org/600393002/diff/1/ui/app_list/app_list_model.cc
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/600393002/diff/1/ui/app_list/app_list_model.c...
ui/app_list/app_list_model.cc:96: LOG(WARNING) << "MergeItems tried to drop item
onto itself ("
As discussed, it's reasonable to leave the model's semantics as "it's legal to
try to do this; you get back an error code" as opposed to crashing.

And safer, because we don't know if these other errors like the OEM one are
possible to trigger in the UI.

[Matt Giuca, 2014-09-30 02:52:35]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-09-30 02:53:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393002/40001

[commit-bot: I haz the power, 2014-09-30 03:49:52]

Committed patchset #3 (id:40001) as d013dabf00a08fe793a1bd6d7d6d735f8f892b58

[commit-bot: I haz the power, 2014-09-30 03:50:40]

Patchset 3 (id:??) landed as
https://crrev.com/aebcdf88e420f281c950d741106e25dcdf0625e3
Cr-Commit-Position: refs/heads/master@{#297356}