NTPSnippets: Set MaxRetriesOn5xx only for interactive requests.

components/ntp_snippets/remote/ntp_snippets_fetcher.cc

Index: components/ntp_snippets/remote/ntp_snippets_fetcher.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
index 62236a8a375cab50981b1a7798b0ce55f963798c..cd03a6610dd196790a0c505b15a8449d4193ca85 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
@@ -4,6 +4,7 @@
 
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
 
+#include <algorithm>
 #include <cstdlib>
 #include <utility>
 
@@ -61,6 +62,9 @@ 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";
 
@@ -132,6 +136,12 @@ bool IsBooleanParameterEnabled(const std::string& 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;
@@ -812,8 +822,13 @@ NTPSnippetsFetcher::RequestBuilder::BuildURLFetcher(
 
   // 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;
 }