Local NTP OneGoogleBar: In case of network errors, keep the cached data

Before, we'd clear the cached data when the refresh request failed due
to e.g. missing network connectivity. Now, we keep the cache in that
case. We still clear the cache if the server responds with an HTTP
error, or when it returns invalid/unexpected data.

Bug: 583292
Change-Id: I974ec774591d77bb11417a7cafdb511934250fcb
Reviewed-on: https://chromium-review.googlesource.com/541357
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481866}

chrome/browser/search/one_google_bar/one_google_bar_fetcher.h

@@ -13,8 +13,19 @@ struct OneGoogleBarData;
 // Interface for fetching OneGoogleBarData over the network.
 class OneGoogleBarFetcher {
  public:
+  enum class Status {
+    // Received a valid response.
+    OK,
+    // Some transient error occurred, e.g. the network request failed because
+    // there is no network connectivity. A previously cached response may still
+    // be used.
+    TRANSIENT_ERROR,
+    // A fatal error occurred, such as the server responding with an error code
+    // or with invalid data. Any previously cached response should be cleared.
+    FATAL_ERROR
+  };
   using OneGoogleCallback =
-      base::OnceCallback<void(const base::Optional<OneGoogleBarData>&)>;
+      base::OnceCallback<void(Status, const base::Optional<OneGoogleBarData>&)>;
 
   virtual ~OneGoogleBarFetcher() = default;
 

chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc

@@ -353,7 +353,7 @@ void OneGoogleBarFetcherImpl::FetchDone(const net::URLFetcher* source) {
     // response).
     DLOG(WARNING) << "Request failed with error: " << request_status.error()
                   << ": " << net::ErrorToString(request_status.error());
-    Respond(base::nullopt);
+    Respond(Status::TRANSIENT_ERROR, base::nullopt);
     return;
   }
 
@@ -363,7 +363,7 @@ void OneGoogleBarFetcherImpl::FetchDone(const net::URLFetcher* source) {
     std::string response;
     source->GetResponseAsString(&response);
     DLOG(WARNING) << "Response: " << response;
-    Respond(base::nullopt);
+    Respond(Status::FATAL_ERROR, base::nullopt);
     return;
   }
 
@@ -386,18 +386,20 @@ void OneGoogleBarFetcherImpl::FetchDone(const net::URLFetcher* source) {
 }
 
 void OneGoogleBarFetcherImpl::JsonParsed(std::unique_ptr<base::Value> value) {
-  Respond(JsonToOGBData(*value));
+  base::Optional<OneGoogleBarData> result = JsonToOGBData(*value);
+  Respond(result.has_value() ? Status::OK : Status::FATAL_ERROR, result);
 }
 
 void OneGoogleBarFetcherImpl::JsonParseFailed(const std::string& message) {
   DLOG(WARNING) << "Parsing JSON failed: " << message;
-  Respond(base::nullopt);
+  Respond(Status::FATAL_ERROR, base::nullopt);
 }
 
 void OneGoogleBarFetcherImpl::Respond(
+    Status status,
     const base::Optional<OneGoogleBarData>& data) {
   for (auto& callback : callbacks_) {
-    std::move(callback).Run(data);
+    std::move(callback).Run(status, data);
   }
   callbacks_.clear();
 }

chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h

@@ -49,7 +49,7 @@ class OneGoogleBarFetcherImpl : public OneGoogleBarFetcher {
   void JsonParsed(std::unique_ptr<base::Value> value);
   void JsonParseFailed(const std::string& message);
 
-  void Respond(const base::Optional<OneGoogleBarData>& data);
+  void Respond(Status status, const base::Optional<OneGoogleBarData>& data);
 
   SigninManagerBase* signin_manager_;
   OAuth2TokenService* token_service_;

chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc

@@ -112,6 +112,19 @@ class OneGoogleBarFetcherImplTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
+  void RespondWithNetworkError() {
+    net::TestURLFetcher* url_fetcher = GetRunningURLFetcher();
+    url_fetcher->set_status(net::URLRequestStatus::FromError(net::ERR_FAILED));
+    url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+  }
+
+  void RespondWithHttpError() {
+    net::TestURLFetcher* url_fetcher = GetRunningURLFetcher();
+    url_fetcher->set_status(net::URLRequestStatus());
+    url_fetcher->set_response_code(net::HTTP_NOT_FOUND);
+    url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+  }
+
   OneGoogleBarFetcherImpl* one_google_bar_fetcher() {
     return &one_google_bar_fetcher_;
   }
@@ -143,7 +156,8 @@ TEST_F(OneGoogleBarFetcherImplTest, UnauthenticatedRequestReturns) {
   one_google_bar_fetcher()->Fetch(callback.Get());
 
   base::Optional<OneGoogleBarData> data;
-  EXPECT_CALL(callback, Run(_)).WillOnce(SaveArg<0>(&data));
+  EXPECT_CALL(callback, Run(OneGoogleBarFetcher::Status::OK, _))
+      .WillOnce(SaveArg<1>(&data));
   RespondWithData(kMinimalValidResponse);
 
   EXPECT_TRUE(data.has_value());
@@ -158,7 +172,8 @@ TEST_F(OneGoogleBarFetcherImplTest, AuthenticatedRequestReturns) {
   IssueAccessToken();
 
   base::Optional<OneGoogleBarData> data;
-  EXPECT_CALL(callback, Run(_)).WillOnce(SaveArg<0>(&data));
+  EXPECT_CALL(callback, Run(OneGoogleBarFetcher::Status::OK, _))
+      .WillOnce(SaveArg<1>(&data));
   RespondWithData(kMinimalValidResponse);
 
   EXPECT_TRUE(data.has_value());
@@ -222,7 +237,8 @@ TEST_F(OneGoogleBarFetcherImplTest, HandlesResponsePreamble) {
   // The reponse may contain a ")]}'" prefix. The fetcher should ignore that
   // during parsing.
   base::Optional<OneGoogleBarData> data;
-  EXPECT_CALL(callback, Run(_)).WillOnce(SaveArg<0>(&data));
+  EXPECT_CALL(callback, Run(OneGoogleBarFetcher::Status::OK, _))
+      .WillOnce(SaveArg<1>(&data));
   RespondWithData(std::string(")]}'") + kMinimalValidResponse);
 
   EXPECT_TRUE(data.has_value());
@@ -233,7 +249,8 @@ TEST_F(OneGoogleBarFetcherImplTest, ParsesFullResponse) {
   one_google_bar_fetcher()->Fetch(callback.Get());
 
   base::Optional<OneGoogleBarData> data;
-  EXPECT_CALL(callback, Run(_)).WillOnce(SaveArg<0>(&data));
+  EXPECT_CALL(callback, Run(OneGoogleBarFetcher::Status::OK, _))
+      .WillOnce(SaveArg<1>(&data));
   RespondWithData(R"json({"oneGoogleBar": {
     "html": { "privateDoNotAccessOrElseSafeHtmlWrappedValue": "bar_html" },
     "pageHooks": {
@@ -280,8 +297,10 @@ TEST_F(OneGoogleBarFetcherImplTest, CoalescesMultipleRequests) {
   base::Optional<OneGoogleBarData> first_data;
   base::Optional<OneGoogleBarData> second_data;
 
-  EXPECT_CALL(first_callback, Run(_)).WillOnce(SaveArg<0>(&first_data));
-  EXPECT_CALL(second_callback, Run(_)).WillOnce(SaveArg<0>(&second_data));
+  EXPECT_CALL(first_callback, Run(OneGoogleBarFetcher::Status::OK, _))
+      .WillOnce(SaveArg<1>(&first_data));
+  EXPECT_CALL(second_callback, Run(OneGoogleBarFetcher::Status::OK, _))
+      .WillOnce(SaveArg<1>(&second_data));
 
   RespondWithData(kMinimalValidResponse);
 
@@ -289,3 +308,42 @@ TEST_F(OneGoogleBarFetcherImplTest, CoalescesMultipleRequests) {
   EXPECT_TRUE(first_data.has_value());
   EXPECT_TRUE(second_data.has_value());
 }
+
+TEST_F(OneGoogleBarFetcherImplTest, NetworkErrorIsTransient) {
+  base::MockCallback<OneGoogleBarFetcher::OneGoogleCallback> callback;
+  one_google_bar_fetcher()->Fetch(callback.Get());
+
+  EXPECT_CALL(callback, Run(OneGoogleBarFetcher::Status::TRANSIENT_ERROR,
+                            Eq(base::nullopt)));
+  RespondWithNetworkError();
+}
+
+TEST_F(OneGoogleBarFetcherImplTest, HttpErrorIsFatal) {
+  base::MockCallback<OneGoogleBarFetcher::OneGoogleCallback> callback;
+  one_google_bar_fetcher()->Fetch(callback.Get());
+
+  EXPECT_CALL(callback,
+              Run(OneGoogleBarFetcher::Status::FATAL_ERROR, Eq(base::nullopt)));
+  RespondWithHttpError();
+}
+
+TEST_F(OneGoogleBarFetcherImplTest, InvalidJsonErrorIsFatal) {
+  base::MockCallback<OneGoogleBarFetcher::OneGoogleCallback> callback;
+  one_google_bar_fetcher()->Fetch(callback.Get());
+
+  EXPECT_CALL(callback,
+              Run(OneGoogleBarFetcher::Status::FATAL_ERROR, Eq(base::nullopt)));
+  RespondWithData(kMinimalValidResponse + std::string(")"));
+}
+
+TEST_F(OneGoogleBarFetcherImplTest, IncompleteJsonErrorIsFatal) {
+  base::MockCallback<OneGoogleBarFetcher::OneGoogleCallback> callback;
+  one_google_bar_fetcher()->Fetch(callback.Get());
+
+  EXPECT_CALL(callback,
+              Run(OneGoogleBarFetcher::Status::FATAL_ERROR, Eq(base::nullopt)));
+  RespondWithData(R"json({"oneGoogleBar": {
+  "html": {},
+  "pageHooks": {}
+}})json");
+}

chrome/browser/search/one_google_bar/one_google_bar_service.cc

@@ -79,8 +79,12 @@ void OneGoogleBarService::SigninStatusChanged() {
 }
 
 void OneGoogleBarService::OneGoogleBarDataFetched(
+    OneGoogleBarFetcher::Status status,
     const base::Optional<OneGoogleBarData>& data) {
-  SetOneGoogleBarData(data);
+  // In case of transient erros, keep our cached data (if any).
+  if (status != OneGoogleBarFetcher::Status::TRANSIENT_ERROR) {
+    SetOneGoogleBarData(data);
+  }
   if (!data) {
     for (auto& observer : observers_) {
       observer.OnOneGoogleBarFetchFailed();

chrome/browser/search/one_google_bar/one_google_bar_service.h

@@ -10,10 +10,10 @@
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_data.h"
+#include "chrome/browser/search/one_google_bar/one_google_bar_fetcher.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_service_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
 
-class OneGoogleBarFetcher;
 class SigninManagerBase;
 
 // A service that downloads, caches, and hands out OneGoogleBarData. It never
@@ -49,7 +49,8 @@ class OneGoogleBarService : public KeyedService {
 
   void SigninStatusChanged();
 
-  void OneGoogleBarDataFetched(const base::Optional<OneGoogleBarData>& data);
+  void OneGoogleBarDataFetched(OneGoogleBarFetcher::Status status,
+                               const base::Optional<OneGoogleBarData>& data);
 
   void SetOneGoogleBarData(const base::Optional<OneGoogleBarData>& data);
 

chrome/browser/search/one_google_bar/one_google_bar_service_observer.h

@@ -13,8 +13,9 @@ class OneGoogleBarServiceObserver {
   // OneGoogleBarService::one_google_bar_data().
   virtual void OnOneGoogleBarDataChanged() = 0;
 
-  // Called when an attempt to fetch the OneGoogleBarData failed. Note that if
-  // there was cached data before the failed fetch attempt, then
+  // Called when an attempt to fetch the OneGoogleBarData failed. Note that
+  // there may still be cached data from a previous fetch. If there was cached
+  // data before the failed fetch attempt and it got cleared, then
   // OnOneGoogleBarDataChanged gets called first.
   virtual void OnOneGoogleBarFetchFailed() {}
 

chrome/browser/search/one_google_bar/one_google_bar_service_unittest.cc

@@ -33,9 +33,10 @@ class FakeOneGoogleBarFetcher : public OneGoogleBarFetcher {
 
   size_t GetCallbackCount() const { return callbacks_.size(); }
 
-  void RespondToAllCallbacks(const base::Optional<OneGoogleBarData>& data) {
+  void RespondToAllCallbacks(Status status,
+                             const base::Optional<OneGoogleBarData>& data) {
     for (OneGoogleCallback& callback : callbacks_) {
-      std::move(callback).Run(data);
+      std::move(callback).Run(status, data);
     }
     callbacks_.clear();
   }
@@ -89,7 +90,7 @@ TEST_F(OneGoogleBarServiceTest, RefreshesOnRequest) {
   // Fulfill it.
   OneGoogleBarData data;
   data.bar_html = "<div></div>";
-  fetcher()->RespondToAllCallbacks(data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, data);
   EXPECT_THAT(service()->one_google_bar_data(), Eq(data));
 
   // Request another refresh.
@@ -102,7 +103,7 @@ TEST_F(OneGoogleBarServiceTest, RefreshesOnRequest) {
   // Fulfill the second request.
   OneGoogleBarData other_data;
   other_data.bar_html = "<div>Different!</div>";
-  fetcher()->RespondToAllCallbacks(other_data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, other_data);
   EXPECT_THAT(service()->one_google_bar_data(), Eq(other_data));
 }
 
@@ -119,34 +120,83 @@ TEST_F(OneGoogleBarServiceTest, NotifiesObserverOnChanges) {
   // it should not result in a "data changed".
   service()->Refresh();
   EXPECT_CALL(observer, OnOneGoogleBarFetchFailed());
-  fetcher()->RespondToAllCallbacks(base::nullopt);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK,
+                                   base::nullopt);
 
   // Non-empty response should result in a notification.
   service()->Refresh();
   OneGoogleBarData data;
   data.bar_html = "<div></div>";
   EXPECT_CALL(observer, OnOneGoogleBarDataChanged());
-  fetcher()->RespondToAllCallbacks(data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, data);
   EXPECT_THAT(service()->one_google_bar_data(), Eq(data));
 
   // Non-empty but identical response should not result in another notification.
   service()->Refresh();
   OneGoogleBarData identical_data = data;
-  fetcher()->RespondToAllCallbacks(identical_data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK,
+                                   identical_data);
 
   // Different response should result in a notification.
   service()->Refresh();
   OneGoogleBarData other_data;
   data.bar_html = "<div>Different</div>";
   EXPECT_CALL(observer, OnOneGoogleBarDataChanged());
-  fetcher()->RespondToAllCallbacks(other_data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, other_data);
   EXPECT_THAT(service()->one_google_bar_data(), Eq(other_data));
 
   // Finally, an empty response should result in a notification now.
   service()->Refresh();
   EXPECT_CALL(observer, OnOneGoogleBarDataChanged());
   EXPECT_CALL(observer, OnOneGoogleBarFetchFailed());
-  fetcher()->RespondToAllCallbacks(base::nullopt);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK,
+                                   base::nullopt);
+  EXPECT_THAT(service()->one_google_bar_data(), Eq(base::nullopt));
+
+  service()->RemoveObserver(&observer);
+}
+
+TEST_F(OneGoogleBarServiceTest, KeepsCacheOnTransientError) {
+  // Load some data.
+  service()->Refresh();
+  OneGoogleBarData data;
+  data.bar_html = "<div></div>";
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, data);
+  ASSERT_THAT(service()->one_google_bar_data(), Eq(data));
+
+  StrictMock<MockOneGoogleBarServiceObserver> observer;
+  service()->AddObserver(&observer);
+
+  // Request a refresh and respond with a transient error.
+  service()->Refresh();
+  // Note: No OnOneGoogleBarDataChanged, since the cached data remains.
+  EXPECT_CALL(observer, OnOneGoogleBarFetchFailed());
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::TRANSIENT_ERROR,
+                                   base::nullopt);
+  // Cached data should still be there.
+  EXPECT_THAT(service()->one_google_bar_data(), Eq(data));
+
+  service()->RemoveObserver(&observer);
+}
+
+TEST_F(OneGoogleBarServiceTest, ClearsCacheOnFatalError) {
+  // Load some data.
+  service()->Refresh();
+  OneGoogleBarData data;
+  data.bar_html = "<div></div>";
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, data);
+  ASSERT_THAT(service()->one_google_bar_data(), Eq(data));
+
+  StrictMock<MockOneGoogleBarServiceObserver> observer;
+  service()->AddObserver(&observer);
+
+  // Request a refresh and respond with a fatal error.
+  service()->Refresh();
+  EXPECT_CALL(observer, OnOneGoogleBarDataChanged());
+  EXPECT_CALL(observer, OnOneGoogleBarFetchFailed());
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::FATAL_ERROR,
+                                   base::nullopt);
+  // Cached data should be gone now.
   EXPECT_THAT(service()->one_google_bar_data(), Eq(base::nullopt));
 
   service()->RemoveObserver(&observer);
@@ -200,7 +250,7 @@ TEST_F(OneGoogleBarServiceSignInTest, ResetsOnSignIn) {
   service()->Refresh();
   OneGoogleBarData data;
   data.bar_html = "<div></div>";
-  fetcher()->RespondToAllCallbacks(data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, data);
   ASSERT_THAT(service()->one_google_bar_data(), Eq(data));
 
   // Sign in. This should clear the cached data.
@@ -215,7 +265,7 @@ TEST_F(OneGoogleBarServiceSignInTest, ResetsOnSignOut) {
   service()->Refresh();
   OneGoogleBarData data;
   data.bar_html = "<div></div>";
-  fetcher()->RespondToAllCallbacks(data);
+  fetcher()->RespondToAllCallbacks(OneGoogleBarFetcher::Status::OK, data);
   ASSERT_THAT(service()->one_google_bar_data(), Eq(data));
 
   // Sign out. This should clear the cached data.

chrome/browser/ui/search/local_ntp_browsertest.cc

@@ -358,7 +358,7 @@ IN_PROC_BROWSER_TEST_F(LocalNTPSmokeTest, FrenchGoogleNTPLoadsWithoutError) {
 class FakeOneGoogleBarFetcher : public OneGoogleBarFetcher {
  public:
   void Fetch(OneGoogleCallback callback) override {
-    std::move(callback).Run(one_google_bar_data_);
+    std::move(callback).Run(Status::OK, one_google_bar_data_);
   }
 
   void set_one_google_bar_data(