Fix crash on item drag in shelf.

chrome/browser/ui/ash/chrome_launcher_prefs.cc

Index: chrome/browser/ui/ash/chrome_launcher_prefs.cc
diff --git a/chrome/browser/ui/ash/chrome_launcher_prefs.cc b/chrome/browser/ui/ash/chrome_launcher_prefs.cc
index 5d804d908a9b28bf586e494e8cef548af9d6a95a..ccc04402569ec07d1e0e4b2b39887b7d7358989f 100644
--- a/chrome/browser/ui/ash/chrome_launcher_prefs.cc
+++ b/chrome/browser/ui/ash/chrome_launcher_prefs.cc
@@ -666,10 +666,12 @@ void RemovePinPosition(Profile* profile, const std::string& app_id) {
 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.

 
@@ -682,9 +684,51 @@ void SetPinPosition(Profile* profile,
   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())