Fix some warnings on Mac that are treated as errors:...

Fix some warnings on Mac that are treated as errors:
The warnings indicate that a public/protected method of a class
visible outside of a .cc file returns a type that's declared in
the anonymous namespace in that file (and hence not visible outside
the file).

BUG=23073
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29075

[Munjal (Google), 2009-10-14 21:20:42]

I am not sure if putting everything in browser_sync namespace will result in
other issues. I will see how it goes on trybots. If so, we can put evereything
in anonymous namespace and add a bunch of using statements.

[tim (not reviewing), 2009-10-14 21:24:42]

I think this subtly won't work? It will actually compile and "work" but some
tests can fail because it's using a different GaiaAuthMock (the one in
gaia_authenticator_unittest.cc)
I hit this problem when writing the test, until I realized my virtual override
wasn't getting used because of the collision. So i threw it in an anonymous
namespace.  If memory serves me.. maybe I'm forgetting a piece.  Either way you
can just rename it (GaiaAuthMockForAuthWatcherTest
or something)

[akalin (wrong akalin), 2009-10-14 21:48:37]

Yes, I ran into the same problem.  I solved it by just putting an
extra namespace (non-anonymous) around the GaiaAuthMock definitions.

Actually, I'd rather we solve the warnings by expanding the anonymous
namespaces as much as possible instead of contracting them, to avoid
any similar ODR-related issues like GaiaAuthMock.

On Wed, Oct 14, 2009 at 11:24 AM,  <tim@chromium.org> wrote:
> I think this subtly won't work? It will actually compile and "work" but s=
ome
> tests can fail because it's using a different GaiaAuthMock (the one in
> gaia_authenticator_unittest.cc)
> I hit this problem when writing the test, until I realized my virtual
> override
> wasn't getting used because of the collision. So i threw it in an anonymo=
us
> namespace. =C2=A0If memory serves me.. maybe I'm forgetting a piece. =C2=
=A0Either way
> you
> can just rename it (GaiaAuthMockForAuthWatcherTest
> or something)
>
>
> http://codereview.chromium.org/273058
>



--=20
Frederick Akalin
Software Engineer

[Munjal (Google), 2009-10-14 21:51:02]

Yep, I just talked to Nick in person. I thought of another way, to make
return type of gaia_auth() getter to the base class of the mock. If that
technique doesn't work then I will expand anonymous namespace and import
necessary identifiers.

-Munjal

On Wed, Oct 14, 2009 at 2:36 PM, Fred Akalin <akalin@google.com> wrote:

> Yes, I ran into the same problem.  I solved it by just putting an
> extra namespace (non-anonymous) around the GaiaAuthMock definitions.
>
> Actually, I'd rather we solve the warnings by expanding the anonymous
> namespaces as much as possible instead of contracting them, to avoid
> any similar ODR-related issues like GaiaAuthMock.
>
> On Wed, Oct 14, 2009 at 11:24 AM,  <tim@chromium.org> wrote:
> > I think this subtly won't work? It will actually compile and "work" but
> some
> > tests can fail because it's using a different GaiaAuthMock (the one in
> > gaia_authenticator_unittest.cc)
> > I hit this problem when writing the test, until I realized my virtual
> > override
> > wasn't getting used because of the collision. So i threw it in an
> anonymous
> > namespace.  If memory serves me.. maybe I'm forgetting a piece.  Either
> way
> > you
> > can just rename it (GaiaAuthMockForAuthWatcherTest
> > or something)
> >
> >
> > http://codereview.chromium.org/273058
> >
>
>
>
> --
> Frederick Akalin
> Software Engineer
>

[Munjal (Google), 2009-10-14 23:15:31]

It doesn't work to use the base class pointer in the getter since new methods
are added in inheriting classes that are used in the test code; so we need a
pointer to the inheriting class.

It also doesn't work to put everything in anonymous namespace since there are
friend declarations in auth_watcher.h and in syncer.h for test classes in
auth_watcher_unittest.cc and syncer_unittest.cc respectively so the test classes
have to be accessible outside the .cc files.

So I resorted to using a different name for GaiaAuthMock (GaiaAuthMockLocal) in
auth_watcher_unittest.cc and putting all the code in browser_sync namespace.

For syncable_unittest.cc, it was possible to put the tests in anonymous
namespace, so I did that.

[Munjal (Google), 2009-10-14 23:32:12]

Uploaded a new patch set with a small cosmetic change:
Renmaed GaiaAuthMockLocal/GaiaAuthMock in auth_watcher_unittest.cc and
gaia_authenticator_unittest.cc to more specific names since they both are in the
same namespace browser_sync.

[Fred, 2009-10-14 23:37:51]

http://codereview.chromium.org/273058/diff/5/1005
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/273058/diff/5/1005#oldcode87
Line 87: namespace {
These can remain in the anonymous namespace, and all the other functions, too. 
It's only the classes the cause trouble.

[Munjal (Google), 2009-10-14 23:53:35]

http://codereview.chromium.org/273058/diff/5/1005
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/273058/diff/5/1005#oldcode87
Line 87: namespace {
On 2009/10/14 23:37:51, Fred wrote:
> These can remain in the anonymous namespace, and all the other functions, too.

> It's only the classes the cause trouble.

Done.

[Fred, 2009-10-15 00:21:03]

LGTM, after you fix the comment.

http://codereview.chromium.org/273058/diff/4008/4010
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/273058/diff/4008/4010#oldcode4347
Line 4347: namespace {
Instead of removing these namespace clauses, just include the TEST_F into the
anonymous namespace; that should work unless there's a FRIEND_TEST for these
tests.

[Munjal (Google), 2009-10-15 00:32:50]

http://codereview.chromium.org/273058/diff/4008/4010
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/273058/diff/4008/4010#oldcode4347
Line 4347: namespace {
On 2009/10/15 00:21:05, Fred wrote:
> Instead of removing these namespace clauses, just include the TEST_F into the
> anonymous namespace; that should work unless there's a FRIEND_TEST for these
> tests.

I thought it's better to be consistent within the file rather than putting
maximum things in anonymous namespace. If I put some TEST_F inside anonymous and
some outside based on what has FRIEND_TEST and what doesn't it will become
inconsistent within the same file.

[Fred, 2009-10-15 00:37:29]

Okay, sounds fair.  LGTM.

On Wed, Oct 14, 2009 at 2:32 PM,  <munjal@chromium.org> wrote:
>
> http://codereview.chromium.org/273058/diff/4008/4010
> File chrome/browser/sync/engine/syncer_unittest.cc (left):
>
> http://codereview.chromium.org/273058/diff/4008/4010#oldcode4347
> Line 4347: namespace {
> On 2009/10/15 00:21:05, Fred wrote:
>>
>> Instead of removing these namespace clauses, just include the TEST_F
>
> into the
>>
>> anonymous namespace; that should work unless there's a FRIEND_TEST for
>
> these
>>
>> tests.
>
> I thought it's better to be consistent within the file rather than
> putting maximum things in anonymous namespace. If I put some TEST_F
> inside anonymous and some outside based on what has FRIEND_TEST and what
> doesn't it will become inconsistent within the same file.
>
> http://codereview.chromium.org/273058
>



-- 
Frederick Akalin
http://www.akalin.cx

[ncarter (slow), 2009-10-15 00:38:40]

LGTM