[NTP Snippets] Add unit tests for NTPSnippetsFetcher

[NTP Snippets] Add unit tests for NTPSnippetsFetcher

Second attempt after https://codereview.chromium.org/1940843002/ was
reverted in https://codereview.chromium.org/1940843002/

The class had almost no coverage given that, besides not having unit
tests, the ones for NTPSnippetsService don't actually exercise the
fetcher.

The proposed tests make use of FakeURLFetcherFactory to exercise basic
success and failure cases and verify how callbacks are triggered.
FakeURLFetcherFactory doesn't however support verifying features like
the uploaded POST data for selected hosts which is left outside the
scope of this patch.

A custom fetcher factory (FailingFakeURLFetcherFactory) is introduced
to handle cases where no baked in response exists, which otherwise
leads to null pointers returned by URLFetcher::Create() and later
dereferenced.

BUG=584428, 608443
Committed: https://crrev.com/7bef8e81ca49e36e1f90dc50aa84a45a465eb3f6
Cr-Commit-Position: refs/heads/master@{#391212}

[mastiz, 2016-05-03 10:08:15]

mastiz@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-05-03 11:33:42]

LGTM with some comments below

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:95: CHECK(url_fetcher_ !=
nullptr);
DCHECK please (DCHECKS are enabled on all the bots, so it'll still get
exercised).
Also the "!= nullptr" isn't required, and is usually omitted in Chromium.

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:37:
FailingFakeURLFetcherFactory() = default;
Is this required? The default ctor should be generated anyway, no?

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:42: return
std::unique_ptr<net::FakeURLFetcher>(new net::FakeURLFetcher(
There's base::WrapUnique, so you don't have to spell out the type twice.

[mastiz, 2016-05-03 11:48:57]

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:95: CHECK(url_fetcher_ !=
nullptr);
On 2016/05/03 11:33:42, Marc Treib wrote:
> DCHECK please (DCHECKS are enabled on all the bots, so it'll still get
> exercised).
> Also the "!= nullptr" isn't required, and is usually omitted in Chromium.

Done. Removed, as discussed offline.

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:37:
FailingFakeURLFetcherFactory() = default;
On 2016/05/03 11:33:42, Marc Treib wrote:
> Is this required? The default ctor should be generated anyway, no?

Removed. You're right, I incorrectly assumed URLFetcherFactor would deleted the
default constructors.

https://codereview.chromium.org/1943783002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:42: return
std::unique_ptr<net::FakeURLFetcher>(new net::FakeURLFetcher(
On 2016/05/03 11:33:42, Marc Treib wrote:
> There's base::WrapUnique, so you don't have to spell out the type twice.

Done.

[mastiz, 2016-05-03 11:52:27]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-05-03 11:52:27]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1943783002/#ps40001
(title: "Addressed minor comments")

[commit-bot: I haz the power, 2016-05-03 11:52:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1943783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1943783002/40001

[commit-bot: I haz the power, 2016-05-03 12:52:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-03 12:54:34]

Description was changed from

==========
[NTP Snippets] Add unit tests for NTPSnippetsFetcher

Second attempt after https://codereview.chromium.org/1940843002/ was
reverted in https://codereview.chromium.org/1940843002/

The class had almost no coverage given that, besides not having unit
tests, the ones for NTPSnippetsService don't actually exercise the
fetcher.

The proposed tests make use of FakeURLFetcherFactory to exercise basic
success and failure cases and verify how callbacks are triggered.
FakeURLFetcherFactory doesn't however support verifying features like
the uploaded POST data for selected hosts which is left outside the
scope of this patch.

A custom fetcher factory (FailingFakeURLFetcherFactory) is introduced
to handle cases where no baked in response exists, which otherwise
leads to null pointers returned by URLFetcher::Create() and later
dereferenced.

BUG=584428,608443
==========

to

==========
[NTP Snippets] Add unit tests for NTPSnippetsFetcher

Second attempt after https://codereview.chromium.org/1940843002/ was
reverted in https://codereview.chromium.org/1940843002/

The class had almost no coverage given that, besides not having unit
tests, the ones for NTPSnippetsService don't actually exercise the
fetcher.

The proposed tests make use of FakeURLFetcherFactory to exercise basic
success and failure cases and verify how callbacks are triggered.
FakeURLFetcherFactory doesn't however support verifying features like
the uploaded POST data for selected hosts which is left outside the
scope of this patch.

A custom fetcher factory (FailingFakeURLFetcherFactory) is introduced
to handle cases where no baked in response exists, which otherwise
leads to null pointers returned by URLFetcher::Create() and later
dereferenced.

BUG=584428,608443

Committed: https://crrev.com/7bef8e81ca49e36e1f90dc50aa84a45a465eb3f6
Cr-Commit-Position: refs/heads/master@{#391212}
==========

[commit-bot: I haz the power, 2016-05-03 12:54:35]

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