Revert of Reland: Ntp: restore scroll position. (patchset #2 id:20001 of https://codereview.chromium.org/2365313002/ )
Reason for revert:
Breaks AndroidTests(Dbg: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/36381
Original issue's description:
> Reland: Ntp: restore scroll position.
>
> * The scroll position is stored as extra data on the NavigationEntry.
> * The main use case handled is when the user clicks on a suggested
> article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
> * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
> * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.
>
> BUG=606356
> TBR=clamy,bauerb,tedchoc
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
> Cr-Commit-Position: refs/heads/master@{#420075}
>
> patch from issue 2327083002 at patchset 160001 (http://crrev.com/2327083002#ps160001)
>
> Committed: https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07
> Cr-Commit-Position: refs/heads/master@{#420896}
TBR=tedchoc@chromium.org,clamy@chromium.org,bauerb@chromium.org,mvanouwerkerk@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=606356
Review-Url: https://codereview.chromium.org/2370023002
Cr-Commit-Position: refs/heads/master@{#420963}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
index 84f12fd..b42064c8 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
@@ -13,7 +13,6 @@
import android.os.Build;
import android.os.SystemClock;
import android.support.v4.view.ViewCompat;
-import android.support.v7.widget.RecyclerView;
import android.view.ContextMenu;
import android.view.LayoutInflater;
import android.view.Menu;
@@ -23,7 +22,6 @@
import org.chromium.base.ApiCompatibilityUtils;
import org.chromium.base.Callback;
import org.chromium.base.CommandLine;
-import org.chromium.base.Log;
import org.chromium.base.ThreadUtils;
import org.chromium.base.VisibleForTesting;
import org.chromium.base.metrics.RecordHistogram;
@@ -71,8 +69,6 @@
import org.chromium.chrome.browser.tabmodel.document.TabDelegate;
import org.chromium.chrome.browser.util.UrlUtilities;
import org.chromium.content_public.browser.LoadUrlParams;
-import org.chromium.content_public.browser.NavigationController;
-import org.chromium.content_public.browser.NavigationEntry;
import org.chromium.content_public.common.Referrer;
import org.chromium.net.NetworkChangeNotifier;
import org.chromium.ui.base.DeviceFormFactor;
@@ -90,7 +86,6 @@
*/
public class NewTabPage
implements NativePage, InvalidationAwareThumbnailProvider, TemplateUrlServiceObserver {
- private static final String TAG = "NewTabPage";
// MostVisitedItem Context menu item IDs.
static final int ID_OPEN_IN_NEW_WINDOW = 0;
@@ -108,9 +103,6 @@
private static final int CTA_IMAGE_CLICKED = 1;
private static final int ANIMATED_LOGO_CLICKED = 2;
- // Key for the scroll position data that may be stored in a navigation entry.
- private static final String NAVIGATION_ENTRY_SCROLL_POSITION_KEY = "NewTabPageScrollPosition";
-
private static final String CHROME_CONTENT_SUGGESTIONS_REFERRER =
"https://www.googleapis.com/auth/chrome-content-suggestions";
@@ -673,23 +665,6 @@
public void onHidden(Tab tab) {
if (mIsLoaded) recordNTPInteractionTime();
}
-
- @Override
- public void onPageLoadStarted(Tab tab, String url) {
- int scrollPosition = mNewTabPageView.getScrollPosition();
- if (scrollPosition == RecyclerView.NO_POSITION) return;
-
- if (mTab.getWebContents() == null) return;
-
- NavigationController controller = mTab.getWebContents().getNavigationController();
- int index = controller.getLastCommittedEntryIndex();
- NavigationEntry entry = controller.getEntryAtIndex(index);
- if (entry == null) return;
-
- assert isNTPUrl(entry.getUrl());
- controller.setEntryExtraData(index, NAVIGATION_ENTRY_SCROLL_POSITION_KEY,
- Integer.toString(scrollPosition));
- }
};
mTab.addObserver(mTabObserver);
mMostVisitedSites = buildMostVisitedSites(mProfile);
@@ -702,8 +677,7 @@
LayoutInflater inflater = LayoutInflater.from(activity);
mNewTabPageView = (NewTabPageView) inflater.inflate(R.layout.new_tab_page_view, null);
- mNewTabPageView.initialize(mNewTabPageManager, mSearchProviderHasLogo, mSnippetsBridge,
- getScrollPositionFromNavigationEntry());
+ mNewTabPageView.initialize(mNewTabPageManager, mSearchProviderHasLogo, mSnippetsBridge);
RecordHistogram.recordBooleanHistogram(
"NewTabPage.MobileIsUserOnline", NetworkChangeNotifier.isOnline());
@@ -863,29 +837,6 @@
}
/**
- * Returns the value of the adapter scroll position that was stored in the last committed
- * navigation entry. Returns {@code RecyclerView.NO_POSITION} if there is no last committed
- * navigation entry, or if no data is found.
- * @return The adapter scroll position.
- */
- private int getScrollPositionFromNavigationEntry() {
- if (mTab.getWebContents() == null) return RecyclerView.NO_POSITION;
-
- NavigationController controller = mTab.getWebContents().getNavigationController();
- int index = controller.getLastCommittedEntryIndex();
- String scrollPositionData =
- controller.getEntryExtraData(index, NAVIGATION_ENTRY_SCROLL_POSITION_KEY);
- if (scrollPositionData == null) return RecyclerView.NO_POSITION;
-
- try {
- return Integer.parseInt(scrollPositionData);
- } catch (NumberFormatException e) {
- Log.w(TAG, "Bad data found for scroll position: %s", scrollPositionData, e);
- return RecyclerView.NO_POSITION;
- }
- }
-
- /**
* @return Whether the NTP has finished loaded.
*/
@VisibleForTesting
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
index 68daf6f..fddc26ea 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
@@ -296,7 +296,7 @@
* @param snippetsBridge The optional bridge, that can be used to interact with the snippets.
*/
public void initialize(NewTabPageManager manager, boolean searchProviderHasLogo,
- SnippetsBridge snippetsBridge, int scrollPosition) {
+ SnippetsBridge snippetsBridge) {
mManager = manager;
mUiConfig = new UiConfig(this);
ViewStub stub = (ViewStub) findViewById(R.id.new_tab_page_layout_stub);
@@ -352,7 +352,6 @@
mNewTabPageAdapter =
new NewTabPageAdapter(mManager, mNewTabPageLayout, snippetsBridge, mUiConfig);
mRecyclerView.setAdapter(mNewTabPageAdapter);
- mRecyclerView.scrollToPosition(scrollPosition);
// Set up swipe-to-dismiss
ItemTouchHelper helper =
@@ -1179,11 +1178,4 @@
return mScrollView.getScrollY();
}
}
-
- /**
- * @return The adapter position the user has scrolled to.
- */
- public int getScrollPosition() {
- return mRecyclerView.getScrollPosition();
- }
}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
index 99b19a5cd..d9f4470 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
@@ -257,18 +257,6 @@
}
/**
- * Returns the approximate adapter position that the user has scrolled to. The purpose of this
- * value is that it can be stored and later retrieved to restore a scroll position that is
- * familiar to the user, showing (part of) the same content the user was previously looking at.
- * This position is valid for that purpose regardless of device orientation changes. Note that
- * if the underlying data has changed in the meantime, different content would be shown for this
- * position.
- */
- public int getScrollPosition() {
- return mLayoutManager.findFirstVisibleItemPosition();
- }
-
- /**
* Finds the view holder for the first header.
* @return The {@code ViewHolder} of the header, or null if it is not present.
*/
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
index 2e00f55..9635e68 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
@@ -217,14 +217,6 @@
@Override
public void copyStateFromAndPrune(NavigationController source, boolean replaceEntry) {
}
-
- @Override
- public String getEntryExtraData(int index, String key) {
- return null;
- }
-
- @Override
- public void setEntryExtraData(int index, String key, String value) {}
}
@MediumTest
diff --git a/content/browser/frame_host/navigation_controller_android.cc b/content/browser/frame_host/navigation_controller_android.cc
index 8c406e9..670343a 100644
--- a/content/browser/frame_host/navigation_controller_android.cc
+++ b/content/browser/frame_host/navigation_controller_android.cc
@@ -9,7 +9,6 @@
#include "base/android/jni_android.h"
#include "base/android/jni_string.h"
#include "base/callback.h"
-#include "base/strings/string16.h"
#include "content/browser/frame_host/navigation_controller_impl.h"
#include "content/browser/frame_host/navigation_entry_impl.h"
#include "content/public/browser/browser_context.h"
@@ -421,32 +420,4 @@
replace_entry);
}
-ScopedJavaLocalRef<jstring> NavigationControllerAndroid::GetEntryExtraData(
- JNIEnv* env,
- const JavaParamRef<jobject>& obj,
- jint index,
- const JavaParamRef<jstring>& jkey) {
- if (index < 0 || index >= navigation_controller_->GetEntryCount())
- return ScopedJavaLocalRef<jstring>();
-
- std::string key = base::android::ConvertJavaStringToUTF8(env, jkey);
- base::string16 value;
- navigation_controller_->GetEntryAtIndex(index)->GetExtraData(key, &value);
- return ConvertUTF16ToJavaString(env, value);
-}
-
-void NavigationControllerAndroid::SetEntryExtraData(
- JNIEnv* env,
- const JavaParamRef<jobject>& obj,
- jint index,
- const JavaParamRef<jstring>& jkey,
- const JavaParamRef<jstring>& jvalue) {
- if (index < 0 || index >= navigation_controller_->GetEntryCount())
- return;
-
- std::string key = base::android::ConvertJavaStringToUTF8(env, jkey);
- base::string16 value = base::android::ConvertJavaStringToUTF16(env, jvalue);
- navigation_controller_->GetEntryAtIndex(index)->SetExtraData(key, value);
-}
-
} // namespace content
diff --git a/content/browser/frame_host/navigation_controller_android.h b/content/browser/frame_host/navigation_controller_android.h
index 69b7e19..fda0654 100644
--- a/content/browser/frame_host/navigation_controller_android.h
+++ b/content/browser/frame_host/navigation_controller_android.h
@@ -135,16 +135,6 @@
const base::android::JavaParamRef<jobject>& obj,
jlong source_native_navigation_controller_android,
jboolean replace_entry);
- base::android::ScopedJavaLocalRef<jstring> GetEntryExtraData(
- JNIEnv* env,
- const base::android::JavaParamRef<jobject>& obj,
- jint index,
- const base::android::JavaParamRef<jstring>& jkey);
- void SetEntryExtraData(JNIEnv* env,
- const base::android::JavaParamRef<jobject>& obj,
- jint index,
- const base::android::JavaParamRef<jstring>& jkey,
- const base::android::JavaParamRef<jstring>& jvalue);
private:
NavigationControllerImpl* navigation_controller_;
diff --git a/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java b/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
index d23c38e..f4f35c6 100644
--- a/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
+++ b/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
@@ -277,18 +277,6 @@
replaceEntry);
}
- @Override
- public String getEntryExtraData(int index, String key) {
- if (mNativeNavigationControllerAndroid == 0) return null;
- return nativeGetEntryExtraData(mNativeNavigationControllerAndroid, index, key);
- }
-
- @Override
- public void setEntryExtraData(int index, String key, String value) {
- if (mNativeNavigationControllerAndroid == 0) return;
- nativeSetEntryExtraData(mNativeNavigationControllerAndroid, index, key, value);
- }
-
@CalledByNative
private static void addToNavigationHistory(Object history, Object navigationEntry) {
((NavigationHistory) history).addEntry((NavigationEntry) navigationEntry);
@@ -350,8 +338,4 @@
long sourceNavigationControllerAndroid);
private native void nativeCopyStateFromAndPrune(long nativeNavigationControllerAndroid,
long sourceNavigationControllerAndroid, boolean replaceEntry);
- private native String nativeGetEntryExtraData(
- long nativeNavigationControllerAndroid, int index, String key);
- private native void nativeSetEntryExtraData(
- long nativeNavigationControllerAndroid, int index, String key, String value);
}
diff --git a/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java b/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
index 84b5637..65da9eb 100644
--- a/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
+++ b/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
@@ -196,20 +196,4 @@
* @param replaceEntry Whether to replace the current entry in source
*/
public void copyStateFromAndPrune(NavigationController source, boolean replaceEntry);
-
- /**
- * Gets extra data on the {@link NavigationEntry} at {@code index}.
- * @param index The index of the navigation entry.
- * @param key The data key.
- * @return The data value, or null if not found.
- */
- String getEntryExtraData(int index, String key);
-
- /**
- * Sets extra data on the {@link NavigationEntry} at {@code index}.
- * @param index The index of the navigation entry.
- * @param key The data key.
- * @param value The data value.
- */
- void setEntryExtraData(int index, String key, String value);
}