NTP: Extract JSON requests from Fetcher.

NTP: Extract JSON requests from Fetcher.

This CL does not change any behavior.

Moving internal classes into an internal namespace.
This simplfies testing these classes (JsonRequest and its Builder) and
prevents the use of internal enums (e.g. FetchResults) that were
publicly exposed but not intended for public use.

BUG=672422
Committed: https://crrev.com/cb5d7fc0e1740712ca116c4615b8b938e750c204
Cr-Commit-Position: refs/heads/master@{#439780}

[fhorschig, 2016-12-16 10:27:50]

Patchset #1 (id:1) has been deleted

[fhorschig, 2016-12-16 10:43:43]

Patchset #1 (id:20001) has been deleted

[fhorschig, 2016-12-16 13:57:59]

Patchset #1 (id:40001) has been deleted

[fhorschig, 2016-12-16 13:58:55]

fhorschig@chromium.org changed reviewers:
+ tschumann@chromium.org

[fhorschig, 2016-12-16 13:58:55]

Hi Tim,
could you please take a look of this is also what you had in mind?

[tschumann, 2016-12-16 16:36:20]

overall looks good. Only a few nits and remarks (for later CLs).

https://codereview.chromium.org/2578173002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2578173002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:306: case
ntp_snippets::internal::Personalization::kPersonal:
nit while we're here: The string should be produced inside the snippets fetcher
and then you don't have to worry about accessing members from the internal
namespace.
Given that this is (as it looks) the only place where we need to access this
member, we should change the method to return a string:
string NTPSnippetsFetcher::PersonalizationModeString()

Thinking more of it, we shouldn't get access to the fetcher here anyways, as
this data is read from the variations anyways.

A TODO() for now is fine -- possibly to move the VariationParameter parsing out
of the snippetsfetcher (in a function) so that we can access it here.

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:35: namespace internal {
it's not in internal anymore ;-)

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:62: using
ParseJSONCallback = internal::NTPSnippetsJsonRequest::ParseJSONCallback;
for types which are exposed publicly, I'd try to avoid alias names and also
avoid exposing internal:: names.
What about moving NTPSnippetsRequestParams, ParseJSONCallback, and
Personalization enum into the ntp_snippets_request_params.h?

side note: untangling the cyclic dependencies between the ntsnippetsfetcher and
the json-request is cumbersome. It's actually a smell and could be resolved by
extracting a SnippetsFetcher interface. There we could define all shared types
as well. The SnippetsFetcherImpl can then depend on both, the interface and the
json-request builder.
Such an interface has another benefit that we can define clean mocks for the
NTPSnippetsFetcher (if we want to use it in tests). Might be worth a TODO(). But
we should probably only address that once we have the need to use a proper Mock
:-)

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.h (right):

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.h:48: struct
NTPSnippetsRequestParams;
no need to distinguish struct vs class in forward declarations. (at least IMO).

[fhorschig, 2016-12-20 10:07:36]

fhorschig@chromium.org changed reviewers:
+ bauerb@chromium.org, jkrcal@chromium.org

[fhorschig, 2016-12-20 10:07:38]

jkrcal@chromium.org: Could you please have a look at the changes in
components/snippets/remote/?

bauerb@chromium.org: Would you please take an OWNER look at
chrome/browser/ui/webui/snippets_internals_message_handler.cc

https://codereview.chromium.org/2578173002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2578173002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:306: case
ntp_snippets::internal::Personalization::kPersonal:
On 2016/12/16 16:36:20, tschumann wrote:
> nit while we're here: The string should be produced inside the snippets
fetcher
> and then you don't have to worry about accessing members from the internal
> namespace.
> Given that this is (as it looks) the only place where we need to access this
> member, we should change the method to return a string:
> string NTPSnippetsFetcher::PersonalizationModeString()
> 
> Thinking more of it, we shouldn't get access to the fetcher here anyways, as
> this data is read from the variations anyways.
> 
> A TODO() for now is fine -- possibly to move the VariationParameter parsing
out
> of the snippetsfetcher (in a function) so that we can access it here.

Fetcher creates the string.
TODO added.
Another TODO was added to the remotesuggestionsprovider. We only use 
|snippets_fetcher()| in snippets-internal/ and tests.
If we have a mock/fake, the provider tests shouldn't check the implementation of
the fetcher.

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:35: namespace internal {
On 2016/12/16 16:36:20, tschumann wrote:
> it's not in internal anymore ;-)

Done.

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:62: using
ParseJSONCallback = internal::NTPSnippetsJsonRequest::ParseJSONCallback;
On 2016/12/16 16:36:20, tschumann wrote:
> for types which are exposed publicly, I'd try to avoid alias names and also
> avoid exposing internal:: names.
> What about moving NTPSnippetsRequestParams, ParseJSONCallback, and
> Personalization enum into the ntp_snippets_request_params.h?
> 
> side note: untangling the cyclic dependencies between the ntsnippetsfetcher
and
> the json-request is cumbersome. It's actually a smell and could be resolved by
> extracting a SnippetsFetcher interface. There we could define all shared types
> as well. The SnippetsFetcherImpl can then depend on both, the interface and
the
> json-request builder.
> Such an interface has another benefit that we can define clean mocks for the
> NTPSnippetsFetcher (if we want to use it in tests). Might be worth a TODO().
But
> we should probably only address that once we have the need to use a proper
Mock
> :-)

Added the TODO. The idea is great, but refactoring in this large CL is not.

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.h (right):

https://codereview.chromium.org/2578173002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.h:48: struct
NTPSnippetsRequestParams;
On 2016/12/16 16:36:20, tschumann wrote:
> no need to distinguish struct vs class in forward declarations. (at least
IMO).

Done.

[tschumann, 2016-12-20 10:22:27]

lgtm

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:112: Personalization
personalization() const { return personalization_; }
Lets' add a TODO() to reconsider the tests -- they should probably test
PersonalizationModeString() instead. (different CL ;-)).

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_request_params.h (right):

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_request_params.h:46: // Callbacks
for JSON parsing, needed because the parsing is platform-
nit: Callbacks for JSON parsing to allow injecting platform-dependent code.

[fhorschig, 2016-12-20 10:30:32]

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:112: Personalization
personalization() const { return personalization_; }
On 2016/12/20 10:22:26, tschumann wrote:
> Lets' add a TODO() to reconsider the tests -- they should probably test
> PersonalizationModeString() instead. (different CL ;-)).

Added TODO here, too.
(I added a TODO to the tests in question.)

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_request_params.h (right):

https://codereview.chromium.org/2578173002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_request_params.h:46: // Callbacks
for JSON parsing, needed because the parsing is platform-
On 2016/12/20 10:22:26, tschumann wrote:
> nit: Callbacks for JSON parsing to allow injecting platform-dependent code.

Done.

[Bernhard Bauer, 2016-12-20 10:49:35]

LGTM w/ nits:

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:624: return "";
Nit: use the empty std::string() constructor (it saves a surprising amount of
instructions!).

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_json_request.h (right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_json_request.h:156: friend class
Builder;
Is this actually necessary, or does the Builder already have private access due
to being a member of the class?

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_json_request_unittest.cc
(right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_json_request_unittest.cc:81: for
(const auto& code : codes) {
Use the real type instead of auto?

[fhorschig, 2016-12-20 11:03:16]

The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 11:03:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 11:09:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 11:09:25]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jkrcal, 2016-12-20 11:59:20]

lgtm, with a nit. Thanks!

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_json_request.cc (right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_json_request.cc:66: bool
IsBooleanParameterEnabled(const std::string& param_name,
When you are moving it around, can you remove this function and use the new
variations::GetVariationParamByFeatureAsBool()?

https://cs.chromium.org/chromium/src/components/variations/variations_associa...

[fhorschig, 2016-12-20 12:11:36]

fhorschig@chromium.org changed reviewers:
+ noyau@chromium.org

[fhorschig, 2016-12-20 12:11:37]

noyau@chromium.org: Could you please have an OWNER's look for
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc?

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:624: return "";
On 2016/12/20 10:49:35, Bernhard Bauer wrote:
> Nit: use the empty std::string() constructor (it saves a surprising amount of
> instructions!).

Thank you!
(Now I finally know the reason and was surprised about the difference.)

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_json_request.cc (right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_json_request.cc:66: bool
IsBooleanParameterEnabled(const std::string& param_name,
On 2016/12/20 11:59:20, jkrcal wrote:
> When you are moving it around, can you remove this function and use the new
> variations::GetVariationParamByFeatureAsBool()?
> 
>
https://cs.chromium.org/chromium/src/components/variations/variations_associa...

Done.

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_json_request.h (right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_json_request.h:156: friend class
Builder;
On 2016/12/20 10:49:35, Bernhard Bauer wrote:
> Is this actually necessary, or does the Builder already have private access
due
> to being a member of the class?

Removed. Does not seem to be necessary.

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_json_request_unittest.cc
(right):

https://codereview.chromium.org/2578173002/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_json_request_unittest.cc:81: for
(const auto& code : codes) {
On 2016/12/20 10:49:35, Bernhard Bauer wrote:
> Use the real type instead of auto?

Done.

[fhorschig, 2016-12-20 12:11:48]

The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 12:12:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[noyau (Ping after 24h), 2016-12-20 12:26:43]

ios lgtm

[commit-bot: I haz the power, 2016-12-20 13:02:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 13:02:47]

Dry run: This issue passed the CQ dry run.

[fhorschig, 2016-12-20 13:10:16]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2016-12-20 13:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
tschumann@chromium.org, bauerb@chromium.org, jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2578173002/#ps180001
(title: "Use |GetVariationParamByFeatureAsBool|.")

[commit-bot: I haz the power, 2016-12-20 13:10:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 13:14:59]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482239416339790,
"parent_rev": "17d6d1fa491f3b1a7bb2b675973623086f2c6117", "commit_rev":
"38f822f5e65f1604b79cc6379a9f678592a63b53"}

[commit-bot: I haz the power, 2016-12-20 13:15:26]

Description was changed from

==========
NTP: Extract JSON requests from Fetcher.

This CL does not change any behavior.

Moving internal classes into an internal namespace.
This simplfies testing these classes (JsonRequest and its Builder) and
prevents the use of internal enums (e.g. FetchResults) that were
publicly exposed but not intended for public use.

BUG=672422
==========

to

==========
NTP: Extract JSON requests from Fetcher.

This CL does not change any behavior.

Moving internal classes into an internal namespace.
This simplfies testing these classes (JsonRequest and its Builder) and
prevents the use of internal enums (e.g. FetchResults) that were
publicly exposed but not intended for public use.

BUG=672422

Review-Url: https://codereview.chromium.org/2578173002
==========

[commit-bot: I haz the power, 2016-12-20 13:15:28]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-12-20 13:18:56]

Description was changed from

==========
NTP: Extract JSON requests from Fetcher.

This CL does not change any behavior.

Moving internal classes into an internal namespace.
This simplfies testing these classes (JsonRequest and its Builder) and
prevents the use of internal enums (e.g. FetchResults) that were
publicly exposed but not intended for public use.

BUG=672422

Review-Url: https://codereview.chromium.org/2578173002
==========

to

==========
NTP: Extract JSON requests from Fetcher.

This CL does not change any behavior.

Moving internal classes into an internal namespace.
This simplfies testing these classes (JsonRequest and its Builder) and
prevents the use of internal enums (e.g. FetchResults) that were
publicly exposed but not intended for public use.

BUG=672422

Committed: https://crrev.com/cb5d7fc0e1740712ca116c4615b8b938e750c204
Cr-Commit-Position: refs/heads/master@{#439780}
==========

[commit-bot: I haz the power, 2016-12-20 13:18:57]

Patchset 7 (id:??) landed as
https://crrev.com/cb5d7fc0e1740712ca116c4615b8b938e750c204
Cr-Commit-Position: refs/heads/master@{#439780}