NTPSnippets: Set MaxRetriesOn5xx only for interactive requests.

components/ntp_snippets/remote/ntp_snippets_fetcher.h

Index: components/ntp_snippets/remote/ntp_snippets_fetcher.h
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.h b/components/ntp_snippets/remote/ntp_snippets_fetcher.h
index 62437c99096287262a34c2022b1411af6ca86f7f..1af70ba6eb6dc71df9458deeb23adff974ace490 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher.h
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.h
@@ -182,6 +182,8 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
                            BuildRequestWithUILanguageOnly);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
                            BuildRequestWithOtherLanguageOnly);
+  FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
+                           BuildRequestFetcherDependingOnInteractivity);
   friend class NTPSnippetsFetcherTest;
 
   enum FetchAPI {
@@ -223,8 +225,13 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
         const scoped_refptr<net::URLRequestContextGetter>& context_getter);
     RequestBuilder& SetUserClassifier(const UserClassifier& user_classifier);
 
-    std::string PreviewRequestBodyForTesting() { return BuildBody(); }
-    std::string PreviewRequestHeadersForTesting() { return BuildHeaders(); }
+    std::string PreviewRequestBodyForTesting() const { return BuildBody(); }

[tschumann, 2016/12/05 17:26:33]

hmm... this has a smell and doesn't seem to scale well.

Usually, a test should use the public interface and observe the expected
behavior or output.

In this case it's tricky as the built request object wont' have the necessary
information. What you want to verify is an interaction with the external world
(through the URL fetcher).

Can't we inject a fake or mock fetcher in the test? One like this
https://codesearch.chromium.org/chromium/src/net/url_request/test_url_fetcher...
via
https://codesearch.chromium.org/chromium/src/net/url_request/url_fetcher_impl...
?

[fhorschig, 2016/12/06 14:31:40]

Reverted changes made here and added a TODO. In order to inject the URLFetcher
for a specific builder, we would need to build and start the request or expose
the request. Both alternatives don't improve the code in any way. As soon as the
Builder and JsonRequest have moved to snippets::internal, these functions are
completely unnecessary.

+    std::string PreviewRequestHeadersForTesting() const {
+      return BuildHeaders();
+    }
+    // Creates a valid net::URLFetcher that has no request and can therefore not
+    // fetch and trigger a delegate. Body and Header are empty.
+    std::unique_ptr<net::URLFetcher> PreviewURLFetcherForTesting() const;
     RequestBuilder& SetUserClassForTesting(const std::string& user_class) {
       user_class_ = user_class;
       return *this;