Fix unit test style in Source/wtf/.

Fix unit test style in Source/wtf/.

Mainly fix namespace usage.
This CL has no behavior changes.

BUG=498604
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197101

[tkent, 2015-06-15 01:17:10]

Patchset #1 (id:1) has been deleted

[tkent, 2015-06-15 02:02:50]

tkent@chromium.org changed reviewers:
+ kochi@chromium.org

[tkent, 2015-06-15 02:02:50]

Kochi-san, would you review this please?
This is the last one.

[kochi, 2015-06-15 03:34:22]

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp
File Source/wtf/DequeTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp#ne...
Source/wtf/DequeTest.cpp:36: namespace {
Is this anonymous namespace necessary?

It seems anonymous namespace within namespace WTF is inconsistently
used in these tests in this CL.

Unless the name conflicts with symbols defined in other files,
technically it's fine to go without anonymous namespace,
but for consistency, do we enforce use of anonymous namespace
for file-local class/function/typedef etc. (i.e. anything without
test body)?

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp#ne...
Source/wtf/DequeTest.cpp:236: static HashSet<void*> constructedWrappedInts;
'static' keyword unnecessary in anonymous namespace.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/FunctionalTest.cpp
File Source/wtf/FunctionalTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/FunctionalTest.c...
Source/wtf/FunctionalTest.cpp:82: namespace {
Maybe it's worth moving this start of anonymous namespace above
'class UnwrappedClass' definition.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashMapTest.cpp
File Source/wtf/HashMapTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashMapTest.cpp#...
Source/wtf/HashMapTest.cpp:142: class DummyRefCounted: public
WTF::RefCounted<DummyRefCounted> {
One space between DummyRefCounted and ':'.
WTF:: not necessary.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashSetTest.cpp
File Source/wtf/HashSetTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashSetTest.cpp#...
Source/wtf/HashSetTest.cpp:166: class DummyRefCounted: public
WTF::RefCounted<DummyRefCounted> {
one space after DummyRefCounted.
WTF:: not necessary.

There are 3 DummyRefCounted defined (in HashMapTest, HashSetTest,
ListHashSetTest).
This one is defined in WTF:: namespace, and others are in WTF:{} namespace.
Could they be consistently defined?

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/ListHashSetTest.cpp
File Source/wtf/ListHashSetTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/ListHashSetTest....
Source/wtf/ListHashSetTest.cpp:249: class DummyRefCounted: public
WTF::RefCounted<DummyRefCounted> {
One space between DummyRefCounted and ':'.
WTF:: unnecessary.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/PartitionAllocTe...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/PartitionAllocTe...
Source/wtf/PartitionAllocTest.cpp:57: static const size_t kTestMaxAllocation =
4096;
How about using 'namespace {}' to remove all the 'static' keywords
used in this file?

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/StringHasherTest...
File Source/wtf/StringHasherTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/StringHasherTest...
Source/wtf/StringHasherTest.cpp:33: static const LChar nullLChars[2] = { 0, 0 };
How about using anonymous namespace to remove all 'static' keywords in this
file?

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/StringBuild...
File Source/wtf/text/StringBuilderTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/StringBuild...
Source/wtf/text/StringBuilderTest.cpp:43: static std::ostream&
operator<<(std::ostream& os, const String& string)
How about using anonymous namespace to remove 'static' keywords on this
and L.48 (and expectEmpty() on L.57 as well)?

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/WTFStringTe...
File Source/wtf/text/WTFStringTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/WTFStringTe...
Source/wtf/text/WTFStringTest.cpp:62: static void
testNumberToStringECMAScript(double number, const char* reference)
Use anonymous namespace?

[tkent, 2015-06-15 03:49:23]

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp
File Source/wtf/DequeTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp#ne...
Source/wtf/DequeTest.cpp:36: namespace {
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> Is this anonymous namespace necessary?
> 
> It seems anonymous namespace within namespace WTF is inconsistently
> used in these tests in this CL.

Yes, this CL keeps anonymous namespaces only if they are necessary.  I saw some
ODR violations without anonymous namespaces.

> Unless the name conflicts with symbols defined in other files,
> technically it's fine to go without anonymous namespace,
> but for consistency, do we enforce use of anonymous namespace
> for file-local class/function/typedef etc. (i.e. anything without
> test body)?

Any local helpers should be wrapped with anonymous namespace ideally.  However,
I don't intend to do it by myself at this moment.

[kochi, 2015-06-15 04:02:39]

lgtm

Could you fix nits for DummyRefCounted on
HashMapTest/HashSetTest/ListHashSetTest?

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp
File Source/wtf/DequeTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp#ne...
Source/wtf/DequeTest.cpp:36: namespace {
On 2015/06/15 03:49:23, tkent wrote:
> On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> > Is this anonymous namespace necessary?
> > 
> > It seems anonymous namespace within namespace WTF is inconsistently
> > used in these tests in this CL.
> 
> Yes, this CL keeps anonymous namespaces only if they are necessary.  I saw
some
> ODR violations without anonymous namespaces.
> 
> > Unless the name conflicts with symbols defined in other files,
> > technically it's fine to go without anonymous namespace,
> > but for consistency, do we enforce use of anonymous namespace
> > for file-local class/function/typedef etc. (i.e. anything without
> > test body)?
> 
> Any local helpers should be wrapped with anonymous namespace ideally. 
However,
> I don't intend to do it by myself at this moment.
> 

Okay, then.

[tkent, 2015-06-15 04:45:58]

Historically we prefer |static| to anonymous namespace in WebKit/Blink.  IMO, we
should use anonymous namespace to align with Chromium.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp
File Source/wtf/DequeTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/DequeTest.cpp#ne...
Source/wtf/DequeTest.cpp:236: static HashSet<void*> constructedWrappedInts;
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> 'static' keyword unnecessary in anonymous namespace.

Done.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/FunctionalTest.cpp
File Source/wtf/FunctionalTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/FunctionalTest.c...
Source/wtf/FunctionalTest.cpp:82: namespace {
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> Maybe it's worth moving this start of anonymous namespace above
> 'class UnwrappedClass' definition.

ParamStorageTraits above needs to have WTF namespace.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashMapTest.cpp
File Source/wtf/HashMapTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashMapTest.cpp#...
Source/wtf/HashMapTest.cpp:142: class DummyRefCounted: public
WTF::RefCounted<DummyRefCounted> {
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> One space between DummyRefCounted and ':'.
> WTF:: not necessary.

Done.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashSetTest.cpp
File Source/wtf/HashSetTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/HashSetTest.cpp#...
Source/wtf/HashSetTest.cpp:166: class DummyRefCounted: public
WTF::RefCounted<DummyRefCounted> {
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> one space after DummyRefCounted.
> WTF:: not necessary.
> 
> There are 3 DummyRefCounted defined (in HashMapTest, HashSetTest,
> ListHashSetTest).
> This one is defined in WTF:: namespace, and others are in WTF:{} namespace.
> Could they be consistently defined?

Done.
Added namespace {} to this file.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/ListHashSetTest.cpp
File Source/wtf/ListHashSetTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/ListHashSetTest....
Source/wtf/ListHashSetTest.cpp:249: class DummyRefCounted: public
WTF::RefCounted<DummyRefCounted> {
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> One space between DummyRefCounted and ':'.
> WTF:: unnecessary.

Done.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/PartitionAllocTe...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/PartitionAllocTe...
Source/wtf/PartitionAllocTest.cpp:57: static const size_t kTestMaxAllocation =
4096;
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> How about using 'namespace {}' to remove all the 'static' keywords
> used in this file?

Done.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/StringHasherTest...
File Source/wtf/StringHasherTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/StringHasherTest...
Source/wtf/StringHasherTest.cpp:33: static const LChar nullLChars[2] = { 0, 0 };
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> How about using anonymous namespace to remove all 'static' keywords in this
> file?

Done.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/StringBuild...
File Source/wtf/text/StringBuilderTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/StringBuild...
Source/wtf/text/StringBuilderTest.cpp:43: static std::ostream&
operator<<(std::ostream& os, const String& string)
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> How about using anonymous namespace to remove 'static' keywords on this
> and L.48 (and expectEmpty() on L.57 as well)?

Remove operator<< by including wtf/testing/WTFTestHelpers.h, and adding
namespace{} for expectBuilderContent() and expectEmpty().

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/WTFStringTe...
File Source/wtf/text/WTFStringTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/WTFStringTe...
Source/wtf/text/WTFStringTest.cpp:62: static void
testNumberToStringECMAScript(double number, const char* reference)
On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> Use anonymous namespace?

Done.

[kochi, 2015-06-15 04:54:45]

still lgtm

I didn't know WebKit historically preferred static,
but I agree that we should prefer anonymous namespace to
static, to more aligned with chromium style.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/FunctionalTest.cpp
File Source/wtf/FunctionalTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/FunctionalTest.c...
Source/wtf/FunctionalTest.cpp:82: namespace {
On 2015/06/15 04:45:57, tkent wrote:
> On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> > Maybe it's worth moving this start of anonymous namespace above
> > 'class UnwrappedClass' definition.
> 
> ParamStorageTraits above needs to have WTF namespace.

Acknowledged.

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/StringBuild...
File Source/wtf/text/StringBuilderTest.cpp (right):

https://codereview.chromium.org/1184043002/diff/2/Source/wtf/text/StringBuild...
Source/wtf/text/StringBuilderTest.cpp:43: static std::ostream&
operator<<(std::ostream& os, const String& string)
On 2015/06/15 04:45:57, tkent wrote:
> On 2015/06/15 03:34:22, Takayoshi Kochi wrote:
> > How about using anonymous namespace to remove 'static' keywords on this
> > and L.48 (and expectEmpty() on L.57 as well)?
> 
> Remove operator<< by including wtf/testing/WTFTestHelpers.h, and adding
> namespace{} for expectBuilderContent() and expectEmpty().

Acknowledged.

[tkent, 2015-06-15 05:50:53]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2015-06-15 05:51:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184043002/30001

[commit-bot: I haz the power, 2015-06-15 05:54:16]

Committed patchset #2 (id:30001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197101