Cleanup namespace usage in modules/.

Cleanup namespace usage in modules/.

BUG=None
TEST=blink_tests
R=abarth@chromium.org

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

[tkent, 2014-08-11 01:18:52]

The following pattern looks strange.

A)
namespace blink {
namespace {
...
}
}

IMO, it should be either of the followings:

B)
namespace {

using namespace blink;

...
}

C)
namespace blink {

namespace {
...The code which should not be exported. (Optional)
}

TEST_F()...
TEST() ...
}

It seems both of Blink and Chromium have no consistent style for namespace in
tests.

[tfarina, 2014-08-11 02:25:58]

On 2014/08/11 01:18:52, tkent wrote:
> The following pattern looks strange.
> 
> A)
> namespace blink {
> namespace {
> ...
> }
> }
> 
I have seem this in Chromium and used that there and reviewers did not complain
about this style.

> IMO, it should be either of the followings:
> 
> B)
> namespace {
> 
> using namespace blink;
> 
> ...
> }
> 
No.

> C)
> namespace blink {
> 
> namespace {
> ...The code which should not be exported. (Optional)
> }
> 
> TEST_F()...
> TEST() ...
> }
>
It is encouraged to wrap TEST() and TEST_F() inside unnamed namespace.

So I don't see a problem with:

namespace blink {
namespace {
...
}  // namespace
}  // namespace blink

Am I missing something? This pattern is already used by many tests files in
chromium, but I agree that we are not consistent about this in either sides.

[tyoshino (SeeGerritForStatus), 2014-08-11 04:09:19]

On 2014/08/11 02:25:58, tfarina wrote:
> On 2014/08/11 01:18:52, tkent wrote:
> namespace blink {
> namespace {
> ...
> }  // namespace
> }  // namespace blink
> 
> Am I missing something? This pattern is already used by many tests files in
> chromium, but I agree that we are not consistent about this in either sides.

I prefer A) or C). Though I don't have any strong opinion between them, IIRC I
also saw lots of files using the pattern A) and ones using C) in Chromium.

FYI, Google C++ style guide prohibits use of "using namespace foo" basically.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp....
I understand this wouldn't be problematic for a unit test binary (or in an
unnamed namespace), but, FYI.

[tfarina, 2014-08-11 16:46:17]

Adam, Kent, is it fine what I have here?

Otherwise, let me know to which style I should change.

Thanks!

[abarth-chromium, 2014-08-11 18:12:10]

lgtm

[commit-bot: I haz the power, 2014-08-11 18:12:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tfarina@chromium.org/454353002/20001

[commit-bot: I haz the power, 2014-08-11 18:15:21]

Change committed as 179981

[eseidel, 2014-08-11 18:19:40]

I don't care about the namespace strategy in tests. :)

Personally I find B more in line with other blink code.  (Then again, until
recently we didn't have any anonymous namespaces inside blink.)

Blink's style is different from Chromium.  I found the response to tkent's
review somewhat rude.  At least the 'No.' with no justification. :/

Eventually Blink's style will probably grow to be more like Chromium's as you
see it doing in this patch.  But currently they are separate.

[tfarina, 2014-08-11 18:37:13]

On Mon, Aug 11, 2014 at 3:19 PM, <eseidel@chromium.org> wrote:

> I don't care about the namespace strategy in tests. :)
>
> Personally I find B more in line with other blink code.  (Then again, until
> recently we didn't have any anonymous namespaces inside blink.)
>
> I'm fine to change, but again, I don't see any technical argument against:

namespace blink {
namespace {
..
}
}


> Blink's style is different from Chromium.  I found the response to tkent's
> review somewhat rude.  At least the 'No.' with no justification. :/
>
Sorry, that was not my intention. Although I know the answer was terse (I'd
expect him to know about the 'using namespace' directive rule of Google C++
style guide).

tyoshino linked to it -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...


> Eventually Blink's style will probably grow to be more like Chromium's as
> you
> see it doing in this patch.  But currently they are separate.
>
I don't think it has much to do with Chromium's style as this is valid C++
code and does not seem to have a big impact in the overral style used by
Blink.

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eseidel, 2014-08-11 19:11:41]

No need to change anything.  :)

[eseidel, 2014-08-11 19:12:27]

Thank you again for cleaning this up.  The real win is getting rid of all these
blink:: lines, regardless of what the namespace delcarations look like at the
top of the files.