Simplify WebContentsState::GetContentsStateAsByteBuffer

chrome/browser/android/tab_state.cc

Index: chrome/browser/android/tab_state.cc
diff --git a/chrome/browser/android/tab_state.cc b/chrome/browser/android/tab_state.cc
index b3547a251ce1c89d51214178aaf7b8daa892885e..d79661332806b67620c7efd97c7c7f718a090d50 100644
--- a/chrome/browser/android/tab_state.cc
+++ b/chrome/browser/android/tab_state.cc
@@ -286,37 +286,31 @@ bool ExtractNavigationEntries(
 };  // anonymous namespace
 
 ScopedJavaLocalRef<jobject> WebContentsState::GetContentsStateAsByteBuffer(
-    JNIEnv* env, TabAndroid* tab) {
+    JNIEnv* env,
+    TabAndroid* tab) {
   Profile* profile = tab->GetProfile();
   if (!profile)
     return ScopedJavaLocalRef<jobject>();
 
   content::NavigationController& controller =
       tab->web_contents()->GetController();
+  const int entry_count = controller.GetEntryCount();
+  const int current_index = controller.GetLastCommittedEntryIndex();
   const int pending_index = controller.GetPendingEntryIndex();
-  int entry_count = controller.GetEntryCount();
-  if (entry_count == 0 && pending_index == 0)
-    entry_count++;
+  DCHECK(current_index < entry_count);
+  DCHECK(pending_index < entry_count);
 
   if (entry_count == 0)
     return ScopedJavaLocalRef<jobject>();
 
-  int current_entry = controller.GetLastCommittedEntryIndex();
-  if (current_entry == -1 && entry_count > 0)
-    current_entry = 0;
-
   std::vector<content::NavigationEntry*> navigations(entry_count);
   for (int i = 0; i < entry_count; ++i) {
-    content::NavigationEntry* entry = (i == pending_index) ?
-        controller.GetPendingEntry() : controller.GetEntryAtIndex(i);
-    navigations[i] = entry;
+    navigations[i] = (i == pending_index) ? controller.GetPendingEntry()

[Eugene But (OOO till 7-30), 2017/04/07 15:21:07]

I was under impression that if |i == pending_index)| then
|controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|. And that this
code is equivalent to  |navigations[i] = controller.GetEntryAtIndex(i);|

[arthursonzogni, 2017/04/10 08:16:35]

It would have been nice if the code could be simplified a little more like that.
I have taken a look, I don't think that |controller.GetPendingEntry()| and
|controller.GetEntryAtIndex(i)| returns the same object when i == pending_entry.

Take a look at the comment around the pointer returned by GetPendingEntry():
```
// An entry we haven't gotten a response for yet.  This will be discarded
// when we navigate again.  It's used only so we know what the currently
// displayed tab is.
//
// This may refer to an item in the entries_ list if the pending_entry_index_
// == -1, or it may be its own entry that should be deleted. Be careful with
// the memory management.
NavigationEntryImpl* pending_entry_;
```
So if |controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|
Then  |pending_index == -1|
Then  |i != pending_index|

+                                          : controller.GetEntryAtIndex(i);
   }
 
   return WebContentsState::WriteNavigationsAsByteBuffer(
-      env,
-      profile->IsOffTheRecord(),
-      navigations,
-      current_entry);
+      env, profile->IsOffTheRecord(), navigations, current_index);
 }
 
 // Common implementation for GetContentsStateAsByteBuffer() and