Fix crash on item drag in shelf.

chrome/browser/ui/ash/chrome_launcher_prefs.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/ash/chrome_launcher_prefs.h"
 
 #include <stddef.h>
 
 #include <set>
 
@@ skipping 648 matching lines @@
   DCHECK(profile);
   DCHECK(!app_id.empty());
   app_list::AppListSyncableService* app_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(profile);
   app_service->SetPinPosition(app_id, syncer::StringOrdinal());
 }
 
 void SetPinPosition(Profile* profile,
                     const std::string& app_id,
                     const std::string& app_id_before,
-                    const std::string& app_id_after) {
+                    const std::vector<std::string>& app_ids_after) {
   DCHECK(profile);
   DCHECK(!app_id.empty());
   DCHECK_NE(app_id, app_id_before);
+  const std::string app_id_after =
+      app_ids_after.empty() ? std::string() : app_ids_after.front();
   DCHECK_NE(app_id, app_id_after);
   DCHECK(app_id_before.empty() || app_id_before != app_id_after);

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

Elim app_id_after and the checks from here and move to the if(s) where they are
used.

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

Done.

 
   app_list::AppListSyncableService* app_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(profile);
   // Some unit tests may not have this service.
   if (!app_service)
     return;
 
   syncer::StringOrdinal position_before =
       app_id_before.empty() ? syncer::StringOrdinal()
                             : app_service->GetPinPosition(app_id_before);
-  syncer::StringOrdinal position_after =
-      app_id_after.empty() ? syncer::StringOrdinal()
-                           : app_service->GetPinPosition(app_id_after);
+  syncer::StringOrdinal position_after = syncer::StringOrdinal();
+  if (position_before.IsValid() && !app_ids_after.empty()) {
+    // We have an element before and at least one after. Find the first element
+    // from after elements that differs from position_before.
+    std::vector<syncer::StringOrdinal> positions_after;
+    for (const auto& app_id_after_it : app_ids_after) {
+      syncer::StringOrdinal position =
+          app_service->GetPinPosition(app_id_after_it);
+      if (!position.IsValid()) {
+        NOTREACHED();
+        break;
+      }
+      positions_after.push_back(position);
+      if (!position_before.Equals(position)) {
+        DCHECK(position_before.LessThan(position));
+        break;
+      }
+    }
+    if (!positions_after.empty()) {
+      if (positions_after[0].Equals(position_before)) {
+        size_t index = positions_after.size() - 1;
+        if (positions_after[index].Equals(position_before)) {
+          // Last after element is still the same as before element. Update it.
+          positions_after[index] = position_before.CreateAfter();
+          app_service->SetPinPosition(app_ids_after[index],
+                                      positions_after[index]);
+        }
+        // Now update the rest of after elements.
+        while (index) {
+          --index;
+          DCHECK(positions_after[index].Equals(position_before));
+          positions_after[index] =
+              position_before.CreateBetween(positions_after[index + 1]);
+          app_service->SetPinPosition(app_ids_after[index],
+                                      positions_after[index]);
+        }
+      }
+      position_after = positions_after[0];
+      DCHECK(position_after.GreaterThan(position_before));
+    }

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

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.

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

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.

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

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).

+  } else {
+    // No before or after position.
+    if (!app_id_after.empty())

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

} else if (!app_ids_after.empty()) {
  string app_id_after = app_ids_after.front();
  DCHECKs

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

Done.

+      position_after = app_service->GetPinPosition(app_id_after);
+  }
 
   syncer::StringOrdinal pin_position;
   if (position_before.IsValid() && position_after.IsValid())
     pin_position = position_before.CreateBetween(position_after);
   else if (position_before.IsValid())
     pin_position = position_before.CreateAfter();
   else if (position_after.IsValid())
     pin_position = position_after.CreateBefore();
   else
     pin_position = syncer::StringOrdinal::CreateInitialOrdinal();
   app_service->SetPinPosition(app_id, pin_position);
 }
 
 }  // namespace launcher
 }  // namespace ash