History page keeps remote and local results in lock step.

When signed in and returning Web History results, the previous implementation of the BrowsingHistoryService would be to fetch about history::QueryOptions::max_count (100) worth of results from both local and web history. Then these results would be merged together in reverse chronological order, and thrown back to the UI. The UI would preset the user with a scrollbar, and when they scrolled to somewhere near the end of the results, the UI would take the results on screen, grab the last one, take the timestamp of that entry, and ask the BrowsingHistoryService for another page, started at that timestamp. This works nicely if web and local history return identical results, but if they return different results, we end up skipping ahead and missing results. This is extremely bad if one source is missing a large gap, the other source only gets a page of results before it gets cut off. To fix this, stop returning results when one source is ahead of the other. These pending results are stored in a closure that can be used when the next page is needed. Things got a bit ugly tracking all the different pieces of state and some refactoring was done to try to keep things as nice as possible. Bug: 728727 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I3709c21095fce7840ba88f3c094ccb5c94dee23d Reviewed-on: https://chromium-review.googlesource.com/577968 Reviewed-by: Ramya Sharma <[email protected]> Reviewed-by: calamity <[email protected]> Reviewed-by: Yusuf Ozuysal <[email protected]> Reviewed-by: Sylvain Defresne <[email protected]> Reviewed-by: Theresa <[email protected]> Commit-Queue: Sky Malice <[email protected]> Cr-Commit-Position: refs/heads/master@{#499655}
tenta-browser · Sep 5, 2017 · d441317 · d441317
1 parent 16c1ecf
commit d441317
Show file tree

Hide file tree

Showing 21 changed files with 870 additions and 567 deletions.
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/history/BrowsingHistoryBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/history/BrowsingHistoryBridge.java
@@ -34,8 +34,13 @@ public void destroy() {
     }
 
     @Override
-    public void queryHistory(String query, long endQueryTime) {
-        nativeQueryHistory(mNativeHistoryBridge, new ArrayList<HistoryItem>(), query, endQueryTime);
+    public void queryHistory(String query) {
+        nativeQueryHistory(mNativeHistoryBridge, new ArrayList<HistoryItem>(), query);
+    }
+
+    @Override
+    public void queryHistoryContinuation() {
+        nativeQueryHistoryContinuation(mNativeHistoryBridge, new ArrayList<HistoryItem>());
     }
 
     @Override
@@ -96,8 +101,10 @@ public void hasOtherFormsOfBrowsingData(boolean hasOtherForms, boolean hasSynced
 
     private native long nativeInit(boolean isIncognito);
     private native void nativeDestroy(long nativeBrowsingHistoryBridge);
-    private native void nativeQueryHistory(long nativeBrowsingHistoryBridge,
-            List<HistoryItem> historyItems, String query, long queryEndTime);
+    private native void nativeQueryHistory(
+            long nativeBrowsingHistoryBridge, List<HistoryItem> historyItems, String query);
+    private native void nativeQueryHistoryContinuation(
+            long nativeBrowsingHistoryBridge, List<HistoryItem> historyItems);
     private native void nativeMarkItemForRemoval(
             long nativeBrowsingHistoryBridge, String url, long[] nativeTimestamps);
     private native void nativeRemoveItems(long nativeBrowsingHistoryBridge);

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
@@ -73,7 +73,6 @@ public class HistoryAdapter extends DateDividedAdapter implements BrowsingHistor
     private boolean mClearOnNextQueryComplete;
     private boolean mPrivacyDisclaimersVisible;
     private boolean mClearBrowsingDataButtonVisible;
-    private long mNextQueryEndTime;
     private String mQueryText = EMPTY_QUERY;
 
     public HistoryAdapter(SelectionDelegate<HistoryItem> delegate, HistoryManager manager,
@@ -115,9 +114,8 @@ public void onDestroyed() {
     public void initialize() {
         mIsInitialized = false;
         mIsLoadingItems = true;
-        mNextQueryEndTime = 0;
         mClearOnNextQueryComplete = true;
-        mHistoryProvider.queryHistory(mQueryText, mNextQueryEndTime);
+        mHistoryProvider.queryHistory(mQueryText);
     }
 
     @Override
@@ -143,7 +141,7 @@ public void loadMoreItems() {
         mIsLoadingItems = true;
         addFooter();
         notifyDataSetChanged();
-        mHistoryProvider.queryHistory(mQueryText, mNextQueryEndTime);
+        mHistoryProvider.queryHistoryContinuation();
     }
 
     /**
@@ -159,10 +157,9 @@ public boolean canLoadMoreItems() {
      */
     public void search(String query) {
         mQueryText = query;
-        mNextQueryEndTime = 0;
         mIsSearching = true;
         mClearOnNextQueryComplete = true;
-        mHistoryProvider.queryHistory(mQueryText, mNextQueryEndTime);
+        mHistoryProvider.queryHistory(mQueryText);
     }
 
     /**
@@ -267,9 +264,6 @@ public void onQueryHistoryComplete(List<HistoryItem> items, boolean hasMorePoten
 
         mIsLoadingItems = false;
         mHasMorePotentialItems = hasMorePotentialMatches;
-        if (items.size() > 0) {
-            mNextQueryEndTime = items.get(items.size() - 1).getTimestamp();
-        }
     }
 
     @Override

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryProvider.java b/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryProvider.java
@@ -19,7 +19,7 @@ public interface BrowsingHistoryObserver {
          * @param items The items that matched the #queryHistory() parameters.
          * @param hasMorePotentialMatches Whether there are more items that match the query text.
          *                                This will be false once the entire local history database
-         *                                has been searched.
+         *                                and remote web history has been searched.
          */
         void onQueryHistoryComplete(List<HistoryItem> items,
                 boolean hasMorePotentialMatches);
@@ -50,10 +50,14 @@ void onQueryHistoryComplete(List<HistoryItem> items,
      * Query browsing history. Only one query may be in-flight at any time. See
      * BrowsingHistoryService::QueryHistory.
      * @param query The query search text. May be empty.
-     * @param endQueryTime The end of the time range to search. A value of 0 indicates that there
-     *                     is no limit on the end time. See the native QueryOptions.
      */
-    void queryHistory(String query, long endQueryTime);
+    void queryHistory(String query);
+
+    /*
+     * Fetches more results using the previous query's text, only valid to call
+     * after queryHistory is called.
+     */
+    void queryHistoryContinuation();
 
     /**
      * Adds the HistoryItem to the list of items being removed. The removal will not be committed

diff --git a/...e/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java b/...e/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
@@ -3029,8 +3029,7 @@ public void run() {
                 BrowsingHistoryBridge historyService = new BrowsingHistoryBridge(false);
                 historyService.setObserver(historyObserver);
                 String historyQueryFilter = "";
-                int historyQueryTimeout = 0;
-                historyService.queryHistory(historyQueryFilter, historyQueryTimeout);
+                historyService.queryHistory(historyQueryFilter);
             }
         });
         historyObserver.getQueryCallback().waitForCallback(0);

diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/history/StubbedHistoryProvider.java b/chrome/android/javatests/src/org/chromium/chrome/browser/history/StubbedHistoryProvider.java
@@ -33,38 +33,33 @@ public void setObserver(BrowsingHistoryObserver observer) {
     }
 
     @Override
-    public void queryHistory(String query, long endQueryTime) {
-        // endQueryTime should be 0 if the query is changing.
-        if (!TextUtils.equals(query, mLastQuery)) assert endQueryTime == 0;
-
-        if (endQueryTime == 0) {
-            mLastQueryEndPosition = 0;
-        } else {
-            // If endQueryTime is not 0, more items are being paged in and endQueryTime should
-            // equal the timestamp of the last HistoryItem returned in the previous query.
-            assert endQueryTime == mItems.get(mLastQueryEndPosition - 1).getTimestamp();
-        }
+    public void queryHistory(String query) {
+        mLastQueryEndPosition = 0;
+        mLastQuery = query;
+        queryHistoryContinuation();
+    }
 
+    @Override
+    public void queryHistoryContinuation() {
         // Simulate basic paging to facilitate testing loading more items.
         // TODO(twellington): support loading more items while searching.
         int queryStartPosition = mLastQueryEndPosition;
         int queryStartPositionPlusFive = mLastQueryEndPosition + 5;
-        boolean hasMoreItems = queryStartPositionPlusFive < mItems.size()
-                && TextUtils.isEmpty(query);
+        boolean hasMoreItems =
+                queryStartPositionPlusFive < mItems.size() && TextUtils.isEmpty(mLastQuery);
         int queryEndPosition = hasMoreItems ? queryStartPositionPlusFive : mItems.size();
 
         mLastQueryEndPosition = queryEndPosition;
-        mLastQuery = query;
 
         List<HistoryItem> items = new ArrayList<>();
-        if (TextUtils.isEmpty(query)) {
+        if (TextUtils.isEmpty(mLastQuery)) {
             items = mItems.subList(queryStartPosition, queryEndPosition);
         } else {
             // Simulate basic search.
-            query = query.toLowerCase(Locale.getDefault());
+            mLastQuery = mLastQuery.toLowerCase(Locale.getDefault());
             for (HistoryItem item : mItems) {
-                if (item.getUrl().toLowerCase(Locale.getDefault()).contains(query)
-                        || item.getTitle().toLowerCase(Locale.getDefault()).contains(query)) {
+                if (item.getUrl().toLowerCase(Locale.getDefault()).contains(mLastQuery)
+                        || item.getTitle().toLowerCase(Locale.getDefault()).contains(mLastQuery)) {
                     items.add(item);
                 }
             }

diff --git a/chrome/browser/android/history/browsing_history_bridge.cc b/chrome/browser/android/history/browsing_history_bridge.cc
@@ -58,27 +58,33 @@ void BrowsingHistoryBridge::QueryHistory(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jobject>& j_result_obj,
-    jstring j_query,
-    int64_t j_query_end_time) {
+    jstring j_query) {
   j_query_result_obj_.Reset(env, j_result_obj);
+  query_history_continuation_.Reset();
 
   history::QueryOptions options;
   options.max_count = kMaxQueryCount;
-  if (j_query_end_time == 0) {
-    options.end_time = base::Time();
-  } else {
-    options.end_time = base::Time::FromJavaTime(j_query_end_time);
-  }
   options.duplicate_policy = history::QueryOptions::REMOVE_DUPLICATES_PER_DAY;
 
   browsing_history_service_->QueryHistory(
       base::android::ConvertJavaStringToUTF16(env, j_query), options);
 }
 
+void BrowsingHistoryBridge::QueryHistoryContinuation(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& j_result_obj) {
+  DCHECK(query_history_continuation_);
+  j_query_result_obj_.Reset(env, j_result_obj);
+  std::move(query_history_continuation_).Run();
+}
+
 void BrowsingHistoryBridge::OnQueryComplete(
     const std::vector<BrowsingHistoryService::HistoryEntry>& results,
-    const BrowsingHistoryService::QueryResultsInfo& query_results_info) {
+    const BrowsingHistoryService::QueryResultsInfo& query_results_info,
+    base::OnceClosure continuation_closure) {
   JNIEnv* env = base::android::AttachCurrentThread();
+  query_history_continuation_ = std::move(continuation_closure);
 
   for (const BrowsingHistoryService::HistoryEntry& entry : results) {
     // TODO(twellington): Move the domain logic to BrowsingHistoryServce so it
@@ -108,7 +114,7 @@ void BrowsingHistoryBridge::OnQueryComplete(
 
   Java_BrowsingHistoryBridge_onQueryHistoryComplete(
       env, j_history_service_obj_, j_query_result_obj_,
-      !(query_results_info.reached_beginning_of_local));
+      !(query_results_info.reached_beginning));
 }
 
 void BrowsingHistoryBridge::MarkItemForRemoval(

diff --git a/chrome/browser/android/history/browsing_history_bridge.h b/chrome/browser/android/history/browsing_history_bridge.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/android/scoped_java_ref.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "chrome/browser/history/profile_based_browsing_history_driver.h"
 
@@ -27,8 +28,11 @@ class BrowsingHistoryBridge : public ProfileBasedBrowsingHistoryDriver {
   void QueryHistory(JNIEnv* env,
                     const JavaParamRef<jobject>& obj,
                     const JavaParamRef<jobject>& j_result_obj,
-                    jstring j_query,
-                    int64_t j_query_end_time);
+                    jstring j_query);
+
+  void QueryHistoryContinuation(JNIEnv* env,
+                                const JavaParamRef<jobject>& obj,
+                                const JavaParamRef<jobject>& j_result_obj);
 
   // Adds a HistoryEntry with the |j_url| and |j_native_timestamps| to the list
   // of items being removed. The removal will not be committed until
@@ -47,7 +51,8 @@ class BrowsingHistoryBridge : public ProfileBasedBrowsingHistoryDriver {
   void OnQueryComplete(
       const std::vector<history::BrowsingHistoryService::HistoryEntry>& results,
       const history::BrowsingHistoryService::QueryResultsInfo&
-          query_results_info) override;
+          query_results_info,
+      base::OnceClosure continuation_closure) override;
   void OnRemoveVisitsComplete() override;
   void OnRemoveVisitsFailed() override;
   void HistoryDeleted() override;
@@ -68,6 +73,8 @@ class BrowsingHistoryBridge : public ProfileBasedBrowsingHistoryDriver {
 
   Profile* profile_;
 
+  base::OnceClosure query_history_continuation_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowsingHistoryBridge);
 };
 

diff --git a/chrome/browser/history/web_history_service_factory.cc b/chrome/browser/history/web_history_service_factory.cc
@@ -7,19 +7,20 @@
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
-#include "components/browser_sync/profile_sync_service.h"
 #include "components/history/core/browser/web_history_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
+#include "components/sync/driver/sync_service.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace {
 // Returns true if the user is signed in and full history sync is enabled,
 // and false otherwise.
 bool IsHistorySyncEnabled(Profile* profile) {
-  browser_sync::ProfileSyncService* sync =
-      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
+  syncer::SyncService* sync =
+      ProfileSyncServiceFactory::GetInstance()->GetSyncServiceForBrowserContext(
+          profile);
   return sync && sync->IsSyncActive() && !sync->IsLocalSyncEnabled() &&
          sync->GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES);
 }

diff --git a/chrome/browser/resources/md_history/history.js b/chrome/browser/resources/md_history/history.js
@@ -4,7 +4,7 @@
 
 // Send the history query immediately. This allows the query to process during
 // the initial page startup.
-chrome.send('queryHistory', ['', 0, RESULTS_PER_PAGE]);
+chrome.send('queryHistory', ['', RESULTS_PER_PAGE]);
 chrome.send('getForeignSessions');
 
 /** @type {Promise} */

diff --git a/chrome/browser/resources/md_history/query_manager.js b/chrome/browser/resources/md_history/query_manager.js
@@ -64,17 +64,14 @@ Polymer({
     this.set('queryState.querying', true);
     this.set('queryState.incremental', incremental);
 
-    var lastVisitTime = 0;
     if (incremental) {
-      var lastVisit = this.queryResult.results.slice(-1)[0];
-      lastVisitTime = lastVisit ? Math.floor(lastVisit.time) : 0;
+      chrome.send('queryHistoryContinuation');
+    } else {
+      chrome.send('queryHistory', [
+        queryState.searchTerm,
+        RESULTS_PER_PAGE,
+      ]);
     }
-
-    chrome.send('queryHistory', [
-      queryState.searchTerm,
-      lastVisitTime,
-      RESULTS_PER_PAGE,
-    ]);
   },
 
   /**