Add test of MakeUniqueID() and friends.

Add test of MakeUniqueID() and friends.

Committed: https://crrev.com/cc2f61204c0ede5c3c5699bc0796bb2bbfab7259
Cr-Commit-Position: refs/heads/master@{#416259}

[sfiera, 2016-09-02 12:23:06]

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

[commit-bot: I haz the power, 2016-09-02 12:23:24]

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

[sfiera, 2016-09-02 12:23:51]

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

[sfiera, 2016-09-02 12:23:51]

We said these functions were simple, so they are probably correct, but there is
no test :[

[commit-bot: I haz the power, 2016-09-02 12:32:52]

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

[commit-bot: I haz the power, 2016-09-02 12:32:53]

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

[sfiera, 2016-09-02 12:40:27]

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

[commit-bot: I haz the power, 2016-09-02 12:40:45]

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

[Marc Treib, 2016-09-02 12:43:25]

lgtm

LGTM. FWIW, I'm planning to remove this string wrangling completely and instead
just pass around category + within-category-id, but I'm waiting until all the
M54 merges have happened.

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider_unittest.cc (right):

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider_unittest.cc:60:
CategoryFactory factory_;
nit: swap these two, so the provider gets deleted before the factory

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider_unittest.cc:104:
EXPECT_THAT(GetCategoryFromUniqueID(kUrl), Eq(kNoCategory));
Is the DCHECK-off part here for any particular reason, as in, are you
investigating something in particular? If not, I'd argue that this is not
something we want to test.

[sfiera, 2016-09-02 12:49:23]

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider_unittest.cc (right):

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider_unittest.cc:60:
CategoryFactory factory_;
On 2016/09/02 12:43:25, Marc Treib wrote:
> nit: swap these two, so the provider gets deleted before the factory

Done.

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider_unittest.cc:104:
EXPECT_THAT(GetCategoryFromUniqueID(kUrl), Eq(kNoCategory));
On 2016/09/02 12:43:25, Marc Treib wrote:
> Is the DCHECK-off part here for any particular reason, as in, are you
> investigating something in particular? If not, I'd argue that this is not
> something we want to test.

Nothing in particular.

But why wouldn't we want to test it? The idea of using DCHECK() instead of
CHECK() is that we should continue to function in a release build, right? Isn't
this branch the *more* important of the two?

[sfiera, 2016-09-02 12:49:27]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-09-02 12:49: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/2306043002/#ps40001
(title: "Use proper construction order.")

[commit-bot: I haz the power, 2016-09-02 12:49:42]

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

[Marc Treib, 2016-09-02 13:02:02]

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider_unittest.cc (right):

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider_unittest.cc:104:
EXPECT_THAT(GetCategoryFromUniqueID(kUrl), Eq(kNoCategory));
On 2016/09/02 12:49:23, sfiera wrote:
> On 2016/09/02 12:43:25, Marc Treib wrote:
> > Is the DCHECK-off part here for any particular reason, as in, are you
> > investigating something in particular? If not, I'd argue that this is not
> > something we want to test.
> 
> Nothing in particular.
> 
> But why wouldn't we want to test it? The idea of using DCHECK() instead of
> CHECK() is that we should continue to function in a release build, right?
Isn't
> this branch the *more* important of the two?

No, DCHECKS are treated like asserts, i.e. something that can *never* happen.
They're essentially documentation. We're so sure of that that we don't even want
to pay the performance cost for the check in release builds.
The code style guide used to say something to that effect, but I can't seem to
find it right now.

[commit-bot: I haz the power, 2016-09-02 14:03:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 14:04:57]

Description was changed from

==========
Add test of MakeUniqueID() and friends.
==========

to

==========
Add test of MakeUniqueID() and friends.

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

[commit-bot: I haz the power, 2016-09-02 14:04:57]

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

[findit-for-me, 2016-09-02 15:21:00]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 416259 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Marc Treib, 2016-09-02 15:30:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2302363002/ by treib@chromium.org.

The reason for reverting is: Fails on several bots:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....

[grt (UTC plus 2), 2016-09-02 19:21:47]

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider_unittest.cc (right):

https://codereview.chromium.org/2306043002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider_unittest.cc:104:
EXPECT_THAT(GetCategoryFromUniqueID(kUrl), Eq(kNoCategory));
On 2016/09/02 13:02:02, Marc Treib wrote:
> On 2016/09/02 12:49:23, sfiera wrote:
> > On 2016/09/02 12:43:25, Marc Treib wrote:
> > > Is the DCHECK-off part here for any particular reason, as in, are you
> > > investigating something in particular? If not, I'd argue that this is not
> > > something we want to test.
> > 
> > Nothing in particular.
> > 
> > But why wouldn't we want to test it? The idea of using DCHECK() instead of
> > CHECK() is that we should continue to function in a release build, right?
> Isn't
> > this branch the *more* important of the two?
> 
> No, DCHECKS are treated like asserts, i.e. something that can *never* happen.
> They're essentially documentation. We're so sure of that that we don't even
want
> to pay the performance cost for the check in release builds.
> The code style guide used to say something to that effect, but I can't seem to
> find it right now.

I think you're looking for this:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[sfiera, 2016-09-03 09:43:46]

Thanks! This was a little surprising to me, coming from google3. Is there some
kind of "Chrome for google3 developers" guide somewhere? Should I start one?

[grt (UTC plus 2), 2016-09-05 09:10:40]

On 2016/09/03 09:43:46, sfiera wrote:
> Thanks! This was a little surprising to me, coming from google3. Is there some
> kind of "Chrome for google3 developers" guide somewhere? Should I start one?

I can't decide if this is a good idea or a bad one. :-)
https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md
and the docs it links to spell out the coding style pretty well. We expect all
developers to be familiar with the guide. If the guide you're proposing
duplicates info there, then that's not so good. If it would contain info that's
lacking from the current guides, then perhaps the current guides should be
improved.