Fix crash on item drag in shelf.

Fix crash on item drag in shelf.

This handles the case when app list sync model may contain the
same ordinals for different items (which is valid case).

BUG=b/31708401
BUG=647855
TEST=Manually on device
TEST=Extended unit_tests

Committed: https://crrev.com/5e9607e4cd1197c7b9c4f2bfbeb76b825c91ef8f
Cr-Commit-Position: refs/heads/master@{#420944}

[khmel, 2016-09-23 22:45:13]

khmel@chromium.org changed reviewers:
+ stevenjb@chromium.org

[khmel, 2016-09-23 22:45:14]

Hi Steven,

PTAL

[stevenjb, 2016-09-23 23:05:01]

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:676: DCHECK(app_id_before.empty()
|| app_id_before != app_id_after);
Elim app_id_after and the checks from here and move to the if(s) where they are
used.

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:726: }
I think this is more complicated than necessary. Also, we shouldn't modify any
other app positions; that can create strange and difficult to debug edge cases.
Better to leave things as they are, they will eventually get resolved, or not,
which is also OK.

I think that the logic should roughly be (in pseudo code)

for (auto& app_id : app_ids_after) {
  position_after = GetPinPosition(app_id);
  if (position_after != position_before)
    break;
}

If all of the position_after entries are the same as position_before,
position_after will remain invalid, and we'll just get a position after
position_before, which is fine. The behavior will be a little unexpected since
the app will move to the end of the apps with the same position, but it is an
edge case and is correctable if one of the apps with matching positions gets
moved.

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:729: if (!app_id_after.empty())
} else if (!app_ids_after.empty()) {
  string app_id_after = app_ids_after.front();
  DCHECKs

[khmel, 2016-09-26 16:43:36]

Thank Steven for review

PTAL

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:676: DCHECK(app_id_before.empty()
|| app_id_before != app_id_after);
On 2016/09/23 23:05:01, stevenjb wrote:
> Elim app_id_after and the checks from here and move to the if(s) where they
are
> used.

Done.

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:726: }
On 2016/09/23 23:05:01, stevenjb wrote:
> I think this is more complicated than necessary. Also, we shouldn't modify any
> other app positions; that can create strange and difficult to debug edge
cases.
> Better to leave things as they are, they will eventually get resolved, or not,
> which is also OK.
> 
> I think that the logic should roughly be (in pseudo code)
> 
> for (auto& app_id : app_ids_after) {
>   position_after = GetPinPosition(app_id);
>   if (position_after != position_before)
>     break;
> }
> 
> If all of the position_after entries are the same as position_before,
> position_after will remain invalid, and we'll just get a position after
> position_before, which is fine. The behavior will be a little unexpected since
> the app will move to the end of the apps with the same position, but it is an
> edge case and is correctable if one of the apps with matching positions gets
> moved.

My initial intention was to implemented something like that, however I
considered edge cases cased by this important to handle it. For example I was
thinking about APP1 APP2 APP3, where APP1 APP2 have the same ordinals. Whenever
user moves APP3 between them, it would be automatically moved back to its
initial position. Also this problem is kind of hidden from user, because
visually item position is changed and user would see problem during the next
sync or new sign in.

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:729: if (!app_id_after.empty())
On 2016/09/23 23:05:01, stevenjb wrote:
> } else if (!app_ids_after.empty()) {
>   string app_id_after = app_ids_after.front();
>   DCHECKs

Done.

[stevenjb, 2016-09-26 17:34:07]

LGTM w/ NOTREACHED -> DCHECK

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2366073002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:726: }
On 2016/09/26 16:43:36, khmel wrote:
> On 2016/09/23 23:05:01, stevenjb wrote:
> > I think this is more complicated than necessary. Also, we shouldn't modify
any
> > other app positions; that can create strange and difficult to debug edge
> cases.
> > Better to leave things as they are, they will eventually get resolved, or
not,
> > which is also OK.
> > 
> > I think that the logic should roughly be (in pseudo code)
> > 
> > for (auto& app_id : app_ids_after) {
> >   position_after = GetPinPosition(app_id);
> >   if (position_after != position_before)
> >     break;
> > }
> > 
> > If all of the position_after entries are the same as position_before,
> > position_after will remain invalid, and we'll just get a position after
> > position_before, which is fine. The behavior will be a little unexpected
since
> > the app will move to the end of the apps with the same position, but it is
an
> > edge case and is correctable if one of the apps with matching positions gets
> > moved.
> 
> My initial intention was to implemented something like that, however I
> considered edge cases cased by this important to handle it. For example I was
> thinking about APP1 APP2 APP3, where APP1 APP2 have the same ordinals.
Whenever
> user moves APP3 between them, it would be automatically moved back to its
> initial position. Also this problem is kind of hidden from user, because
> visually item position is changed and user would see problem during the next
> sync or new sign in.

Acknowledged. While I agree that is less than ideal, I still think that trying
to handle the edge case is risky and expensive to maintain.

If we really wanted to address the edge case we could do so by adding an
app_service->SanitizePinPosition(start_pos) method that would ensure that
|start_pos| is unique. Then we could carefully test that, including sync tests
(which is where things get dicey). We didn't do this the first time around
because of time constraints (and again, it's an edge case that shouldn't
generally happen).

https://codereview.chromium.org/2366073002/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2366073002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:691: }
NOTREACHED should only be used to document code that should never logically be
reached; we shouldn't 'continue' after it. If we think it might happen and need
to handle it then we should LOG(ERROR) instead.

In this case I would replace this with:
DCHECK(position.IsValid())

[khmel, 2016-09-26 18:11:08]

Thanks Steven for review. I also like your idea:

>> app_service->SanitizePinPosition(start_pos)

We can address it in next CLs.

https://codereview.chromium.org/2366073002/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2366073002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:691: }
On 2016/09/26 17:34:07, stevenjb wrote:
> NOTREACHED should only be used to document code that should never logically be
> reached; we shouldn't 'continue' after it. If we think it might happen and
need
> to handle it then we should LOG(ERROR) instead.
> 
> In this case I would replace this with:
> DCHECK(position.IsValid())

Thanks, LOG(ERROR) looks good.

[khmel, 2016-09-26 18:11:36]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-09-26 18:11:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2366073002/#ps60001
(title: "Drop NOTREACHED")

[commit-bot: I haz the power, 2016-09-26 18:12:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 18:40:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-26 18:44:16]

Description was changed from

==========
Fix crash on item drag in shelf.

This handles the case when app list sync model may contain the
same ordinals for different items (which is valid case).

BUG=b/31708401
BUG=647855
TEST=Manually on device
TEST=Extended unit_tests
==========

to

==========
Fix crash on item drag in shelf.

This handles the case when app list sync model may contain the
same ordinals for different items (which is valid case).

BUG=b/31708401
BUG=647855
TEST=Manually on device
TEST=Extended unit_tests

Committed: https://crrev.com/5e9607e4cd1197c7b9c4f2bfbeb76b825c91ef8f
Cr-Commit-Position: refs/heads/master@{#420944}
==========

[commit-bot: I haz the power, 2016-09-26 18:44:17]

Patchset 4 (id:??) landed as
https://crrev.com/5e9607e4cd1197c7b9c4f2bfbeb76b825c91ef8f
Cr-Commit-Position: refs/heads/master@{#420944}