Simplify WebContentsState::GetContentsStateAsByteBuffer

chrome/browser/android/tab_state.cc

 // Copyright 2014 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/android/tab_state.h"
 
 #include <jni.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 268 matching lines @@
   if (*current_entry_index < 0 ||
       *current_entry_index >= static_cast<int>(navigations->size()))
     return false;
 
   return true;
 }
 
 };  // 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
 // CreateContentsStateAsByteBuffer(). Does not assume ownership of the
 // navigations.
 ScopedJavaLocalRef<jobject> WebContentsState::WriteNavigationsAsByteBuffer(
     JNIEnv* env,
     bool is_off_the_record,
     const std::vector<content::NavigationEntry*>& navigations,
     int current_entry) {
@@ skipping 242 matching lines @@
   std::unique_ptr<WebContents> web_contents(WebContents::FromJavaWebContents(
       WebContentsState::RestoreContentsFromByteBuffer(
           env, clazz, state, saved_state_version, true)));
   if (web_contents.get())
     TabAndroid::CreateHistoricalTabFromContents(web_contents.get());
 }
 
 bool RegisterTabState(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }