[Remote suggestions] Move the throttler into the Scheduler

components/ntp_snippets/remote/json_request.h

 // 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.
 
 #ifndef COMPONENTS_NTP_SNIPPETS_REMOTE_JSON_REQUEST_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_JSON_REQUEST_H_
 
 #include <memory>
 #include <string>
 #include <utility>
@@ skipping 24 matching lines @@
 // histograms, so do not change existing values. Insert new values at the end,
 // and update the histogram definition.
 enum class FetchResult {
   SUCCESS,
   DEPRECATED_EMPTY_HOSTS,
   URL_REQUEST_STATUS_ERROR,
   HTTP_ERROR,
   JSON_PARSE_ERROR,
   INVALID_SNIPPET_CONTENT_ERROR,
   OAUTH_TOKEN_ERROR,
-  INTERACTIVE_QUOTA_ERROR,
-  NON_INTERACTIVE_QUOTA_ERROR,
+  INTERACTIVE_QUOTA_ERROR_OBSOLETE,

[tschumann, 2017/02/24 10:18:08]

If it's nothing widely adopted, I'd prefer prefixing enum names so that it's
more obvious.

Also, how we deal with UMA is a bit weird.
-- histograms.xml relies on the numeric value -- best to assign them
specifically.
-- the old constants themselves don't need to be preserved; it's important we
don't reuse codes. A comment with 'next available id' would also do the trick,
hm?

[Marc Treib, 2017/02/24 15:50:35]

+1 to making a prefix. In fact, why not use "DEPRECATED_" which is already used
above.

I don't much like those "next available ID" comments; they're too easy to miss.

No strong feelings about explicit numeric values.

[tschumann, 2017/02/25 17:02:20]

The numbers have the advantage that it's much easier to see histograms.xml being
in-sync (no need to count the order).
I'm unhappy about the fact that the code needs to deal with values which are not
used anymore (see the switch statements in this file). 
There is a manual (untested) step involved anyways (updating histograms.xml), so
I'd much more prefer the next_id comment (we can also add a comment before
RESULT_MAX explaining to take care of it).

Also, any reason we have the RESULT_MAX value in here?  given that there are
unused states in between, there doesn't seem to be much use for it.

[Marc Treib, 2017/02/27 09:18:43]

The MAX is there for the UMA macros, they need a "max_valid_input+1" param.

[jkrcal, 2017/02/27 10:26:23]

Added numeric values which allowed removing the deprecated ones.
I kept the RESULT_MAX due to the reason mentioned by Marc and because it further
emphasises what is the next available ID.

+  NON_INTERACTIVE_QUOTA_ERROR_OBSOLETE,
   RESULT_MAX
 };
 
 enum FetchAPI {
   CHROME_READER_API,
   CHROME_CONTENT_SUGGESTIONS_API,
 };
 
 // A single request to query remote suggestions. On success, the suggestions are
 // returned in parsed JSON form (base::Value).
@@ skipping 120 matching lines @@
   base::WeakPtrFactory<JsonRequest> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(JsonRequest);
 };
 
 }  // namespace internal
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_JSON_REQUEST_H_