Moved all pipe functionality into a self-containing Communication::Pipe class

This also uses googletest to test the pipe functionality properly and makes sure all Unicode Windows functions are spelled out explicityl (meaning CreatePipeW rather than CreatePipe).

[Felix Dahlke, 2013-06-03 12:18:47]

http://codereview.adblockplus.org/10786016/diff/1/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/10786016/diff/1/src/engine/main.cpp#newcode88
src/engine/main.cpp:88: 
Shouldn't we delete pipe here?

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cpp
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cp...
src/shared/Communication.cpp:21: if (!::GetUserNameW(buffer.get(), &length))
Why GetUserNameW? We normally don't explicitly call the W functions. You're also
using a couple of explicit W functions further below, same applies to those.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cp...
src/shared/Communication.cpp:74: CloseHandle(pipe);
What about FlushFileBuffers() and DisconnectNamedPipe()? I thought they were all
necessary.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h
File src/shared/Communication.h (right):

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:14: extern const std::wstring pipeName;
Since we're really always using the same pipeName, I guess it makes sense to
have it as a private constant and not passing a name in. I don't see a need for
having more than one pipe.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:15: const int bufferSize = 1024;
This is only being used in Communication.cpp, I'd rather have it as an
implementation-only variable.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:30: int32_t currentType;
I'd like to have the typedef here I asked for in the other review.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:33: void CheckType(ValueType expectedType)
I'd prefer to see this function in the cpp, it's a bit big for an inline
function IMO.

[Wladimir Palant, 2013-06-03 14:00:26]

Only addressed two of the comments, for the rest of them please see my
responses.

http://codereview.adblockplus.org/10786016/diff/1/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/10786016/diff/1/src/engine/main.cpp#newcode88
src/engine/main.cpp:88: 
On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> Shouldn't we delete pipe here?

It's an auto_ptr.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cpp
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cp...
src/shared/Communication.cpp:21: if (!::GetUserNameW(buffer.get(), &length))
On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> Why GetUserNameW? We normally don't explicitly call the W functions.

We should however. We will fail if due to some macro misconfiguration we get the
ASCII function here - we want GetUserNameW and nothing else. In fact, the tests
don't set the UNICODE macro - so they will try to call the ASCII function and
fail.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cp...
src/shared/Communication.cpp:74: CloseHandle(pipe);
On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> What about FlushFileBuffers() and DisconnectNamedPipe()? I thought they were
all
> necessary.

I think that they are only necessary if the server wants to force the client off
the pipe - not if the client will close the connection anyway (in our case it
will).

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h
File src/shared/Communication.h (right):

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:14: extern const std::wstring pipeName;
On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> Since we're really always using the same pipeName, I guess it makes sense to
> have it as a private constant and not passing a name in. I don't see a need
for
> having more than one pipe.

The tests use a different pipe name.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:15: const int bufferSize = 1024;
On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> This is only being used in Communication.cpp, I'd rather have it as an
> implementation-only variable.

True.

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.h#...
src/shared/Communication.h:30: int32_t currentType;
On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> I'd like to have the typedef here I asked for in the other review.

You are getting it in the other review :)

[Felix Dahlke, 2013-06-04 06:39:39]

LGTM

http://codereview.adblockplus.org/10786016/diff/1/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/10786016/diff/1/src/engine/main.cpp#newcode88
src/engine/main.cpp:88: 
On 2013/06/03 14:00:26, Wladimir Palant wrote:
> On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> > Shouldn't we delete pipe here?
> 
> It's an auto_ptr.

Oops, then let's not delete it :)

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cpp
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/10786016/diff/1/src/shared/Communication.cp...
src/shared/Communication.cpp:21: if (!::GetUserNameW(buffer.get(), &length))
On 2013/06/03 14:00:26, Wladimir Palant wrote:
> On 2013/06/03 12:18:47, Felix H. Dahlke wrote:
> > Why GetUserNameW? We normally don't explicitly call the W functions.
> 
> We should however. We will fail if due to some macro misconfiguration we get
the
> ASCII function here - we want GetUserNameW and nothing else. In fact, the
tests
> don't set the UNICODE macro - so they will try to call the ASCII function and
> fail.

Agreed, we should do that with all the other calls in the future then.