NTPSnippets: Set MaxRetriesOn5xx only for interactive requests.

components/ntp_snippets/remote/ntp_snippets_fetcher.cc

 // Copyright 2016 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 "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
 
+#include <algorithm>
 #include <cstdlib>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_writer.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
@@ skipping 37 matching lines @@
 const char kChromeReaderApiScope[] =
     "https://www.googleapis.com/auth/webhistory";
 const char kContentSuggestionsApiScope[] =
     "https://www.googleapis.com/auth/chrome-content-suggestions";
 const char kSnippetsServerNonAuthorizedFormat[] = "%s?key=%s";
 const char kAuthorizationRequestHeaderFormat[] = "Bearer %s";
 
 // Variation parameter for personalizing fetching of snippets.
 const char kPersonalizationName[] = "fetching_personalization";
 
+// Variation parameter for disabling the retry .

[Marc Treib, 2016/12/08 10:52:06]

nitty nit: no space before "."

[fhorschig, 2016/12/09 12:37:54]

Oh no, a Plenk! 
(https://en.wikipedia.org/wiki/Plenken)

[Marc Treib, 2016/12/09 12:59:24]

Haha, I didn't know that term!
I have to admit though that this kind of thing bothers me way more than it
should. So know I'll know what to call it :D

[fhorschig, 2016/12/12 09:54:03]

You are not alone ;)

+const char kNumberOf5xxRetries[] = "number_of_non_interactive_5xx_retries";

[Marc Treib, 2016/12/08 10:52:06]

nit1: Rename the variable to make clear it's a param name, not an actual value.
(Maybe just append "Name" to the name :) )

nit2: "background_5xx_retry_count"? (our other params use "_count" rather than
"number_of_", and "background" is IMO more descriptive than "non_interactive")

[fhorschig, 2016/12/09 12:37:54]

Done. Done.

+
 // Variation parameter for chrome-content-suggestions backend.
 const char kContentSuggestionsBackend[] = "content_suggestions_backend";
 
 // Constants for possible values of the "fetching_personalization" parameter.
 const char kPersonalizationPersonalString[] = "personal";
 const char kPersonalizationNonPersonalString[] = "non_personal";
 const char kPersonalizationBothString[] = "both";  // the default value
 
 const int kMaxExcludedIds = 100;
 
@@ skipping 51 matching lines @@
   if (param_value == kBooleanParameterDisabled) {
     return false;
   }
   if (!param_value.empty()) {
     LOG(WARNING) << "Invalid value \"" << param_value
                  << "\" for variation parameter " << param_name;
   }
   return default_value;
 }
 
+int GetNumberOfNonInteractiveRetries() {
+  std::string retries = variations::GetVariationParamValue(
+      ntp_snippets::kStudyName, kNumberOf5xxRetries);
+  return retries.empty() ? 0 : std::max(0, std::stoi(retries));

[Marc Treib, 2016/12/08 10:52:06]

Use GetParamAsInt from ntp_snippets/features.h.
It should also be attached to ntp_snippets::kArticleSuggestionsFeature rather
than to the study name.

[fhorschig, 2016/12/09 12:37:54]

Done.

[Marc Treib, 2016/12/09 12:59:24]

Heads-up: As of https://codereview.chromium.org/2558743003/, the helpers have
moved into components/variations.

[fhorschig, 2016/12/12 09:54:03]

Done.

+}
+
 bool UsesChromeContentSuggestionsAPI(const GURL& endpoint) {
   if (endpoint == kChromeReaderServer) {
     return false;
   }
 
   if (endpoint != kContentSuggestionsServer &&
       endpoint != kContentSuggestionsStagingServer &&
       endpoint != kContentSuggestionsAlphaServer) {
     LOG(WARNING) << "Unknown value for " << kContentSuggestionsBackend << ": "
                  << "assuming chromecontentsuggestions-style API";
@@ skipping 660 matching lines @@
   url_fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                             net::LOAD_DO_NOT_SAVE_COOKIES);
   data_use_measurement::DataUseUserData::AttachToFetcher(
       url_fetcher.get(), data_use_measurement::DataUseUserData::NTP_SNIPPETS);
 
   url_fetcher->SetExtraRequestHeaders(headers);
   url_fetcher->SetUploadData("application/json", body);
 
   // Fetchers are sometimes cancelled because a network change was detected.
   url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3);
-  // Try to make fetching the files bit more robust even with poor connection.
-  url_fetcher->SetMaxRetriesOn5xx(3);
+  if (params_.interactive_request) {
+    // Try to make fetching the files bit more robust even with poor connection.
+    url_fetcher->SetMaxRetriesOn5xx(2);
+  } else {
+    // Don't retry immediately if a scheduled fetch fails.

[Marc Treib, 2016/12/08 10:52:06]

Comment is now not accurate anymore.

[fhorschig, 2016/12/09 12:37:54]

Done.

+    url_fetcher->SetMaxRetriesOn5xx(GetNumberOfNonInteractiveRetries());

[Marc Treib, 2016/12/08 10:52:06]

Make a "GetNumberOf5xxRetries(bool interactive)" helper? Then SetMaxRetriesOn5xx
can be called only once.

[fhorschig, 2016/12/09 12:37:54]

Done.

+  }
   return url_fetcher;
 }
 
 void NTPSnippetsFetcher::RequestBuilder::PrepareLanguages(
     translate::LanguageModel::LanguageInfo* ui_language,
     translate::LanguageModel::LanguageInfo* other_top_language) const {
   // TODO(jkrcal): Add language model factory for iOS and add fakes to tests so
   // that |language_model| is never nullptr. Remove this check and add a DCHECK
   // into the constructor.
   if (!language_model_ || !IsSendingTopLanguagesEnabled()) {
@@ skipping 270 matching lines @@
   NOTREACHED();
   return false;
 }
 
 bool NTPSnippetsFetcher::NeedsAuthentication() const {
   return (personalization_ == Personalization::kPersonal ||
           personalization_ == Personalization::kBoth);
 }
 
 }  // namespace ntp_snippets