Moved logging for NaCl environment from gpu/command_buffer/common/logging.h t...

Moved logging for NaCl environment from gpu/command_buffer/common/logging.h to base/nacl_logging.h. We need to compile command_buffer with nacl compiler. A few files in command_buffer include files in base which in turn include base/logging.h. So it was necessary to fix base/logging.h.

BUG=26104

[apatrick, 2010-01-04 19:04:10]

I think this should be reviewed by someone who understands what should and
should not go in base. I suggest brettw.

[brettw, 2010-01-04 20:18:37]

can you elaborate what the problem is with our logging.h under NaCl? I would not
expect the output to work since you're in a restricted environment, but I
wouldn't necessarily expect just the header file to cause problems.

[alokp, 2010-01-04 21:43:38]

This CL just moves gpu/comman_buffer/common/logging.h to base/nacl_logging.h. I
do not know why it was done in the first place. The history got lost when o3d to
chrome transition took place.

Al/Vangelis: Do you guys know why special type of logging was needed for nacl?
Who was the original author?

[apatrick, 2010-01-04 22:17:31]

It was probably piman.

+piman

On Mon, Jan 4, 2010 at 1:43 PM, <alokp@chromium.org> wrote:

> This CL just moves gpu/comman_buffer/common/logging.h to
> base/nacl_logging.h. I
> do not know why it was done in the first place. The history got lost when
> o3d to
> chrome transition took place.
>
> Al/Vangelis: Do you guys know why special type of logging was needed for
> nacl?
> Who was the original author?
>
>
> http://codereview.chromium.org/519020
>

[Antoine Labour, 2010-01-04 22:33:16]

On Mon, Jan 4, 2010 at 2:17 PM, Alastair Patrick <apatrick@google.com>wrote:

> It was probably piman.
>
> +piman
>
> On Mon, Jan 4, 2010 at 1:43 PM, <alokp@chromium.org> wrote:
>
>> This CL just moves gpu/comman_buffer/common/logging.h to
>> base/nacl_logging.h. I
>> do not know why it was done in the first place. The history got lost when
>> o3d to
>> chrome transition took place.
>>
>> Al/Vangelis: Do you guys know why special type of logging was needed for
>> nacl?
>> Who was the original author?
>
>
Back then, base/logging.h from google3 didn't compile under nacl. We moved
to chrome's base/logging.h but I don't think it trivially worked when we
made that switch. It's from about a year ago, so maybe it does compile now,
but since you can't access files from nacl, at least some features will not
work.

Antoine


>
>>
>> http://codereview.chromium.org/519020
>>
>
>

[alokp, 2010-01-04 22:58:40]

base/logging.h does not compile under nacl either. I am getting the following
errors:

logging.h:795: error: expected unqualified-id before '&' token
logging.h:795: error: expected ',' or '...' before '&' token
logging.h:795: error: 'std::ostream& operator<<(std::ostream&)' must take
exactly two arguments

I guess it is complaining about std::wstring. I might be able to fix compile
errors in logging.h but we would need a different logging.cc for nacl anyway.

David: Which OS should be defined under nacl - OS_POSIX?
Brett: What do you suggest? Should I try to make logging.h work or stubbed out
nacl_logging.h is fine?

[apatrick, 2010-01-04 23:10:50]

Ideally we don't want the client command_buffer code to be directly
dependent on base because it'll be linked with NaCl modules. We could break
the dependency with a callback function for reporting diagnostics, if
logging is the only issue:

typedef void LogFunc(const char* message, const char* file, int line);

Or something like that. I'm not sure where the best to put it would be.

On Mon, Jan 4, 2010 at 2:58 PM, <alokp@chromium.org> wrote:

> base/logging.h does not compile under nacl either. I am getting the
> following
> errors:
>
> logging.h:795: error: expected unqualified-id before '&' token
> logging.h:795: error: expected ',' or '...' before '&' token
> logging.h:795: error: 'std::ostream& operator<<(std::ostream&)' must take
> exactly two arguments
>
> I guess it is complaining about std::wstring. I might be able to fix
> compile
> errors in logging.h but we would need a different logging.cc for nacl
> anyway.
>
> David: Which OS should be defined under nacl - OS_POSIX?
> Brett: What do you suggest? Should I try to make logging.h work or stubbed
> out
> nacl_logging.h is fine?
>
>
>
> http://codereview.chromium.org/519020
>

[alokp, 2010-01-04 23:25:19]

Here are all the dependencies of client command buffer code on base:
base/basictypes.h
base/logging.h
base/task.h for Callback0

I think logging is the only problematic one. If we continue to use files in
base, we will have to patch base/logging.h because many files in base include
it.

[apatrick, 2010-01-04 23:52:48]

I suspect we don't much out of base/basictypes.h, probably just int32 and
the like. If that's true, you could just copy and paste them into the
command buffer code. For base/logging.h, I'd either do something like the
callback I described above or just find a simple way to disable logging
entirely in NaCl for now.

The client code probably doesn't need to dependent on base/task.h. It's part
of the signature of CommandBuffer::SetPutOffsetChangeCallback. The client
implementations of that interface (CommandBufferProxy and
CommandBufferPepper) do not actually implement that function so you might be
able to remove that function from CommandBuffer, CommandBufferProxy,
CommandBufferPepper and CommandBufferMock.

On Mon, Jan 4, 2010 at 3:25 PM, <alokp@chromium.org> wrote:

> Here are all the dependencies of client command buffer code on base:
> base/basictypes.h
> base/logging.h
> base/task.h for Callback0
>
> I think logging is the only problematic one. If we continue to use files in
> base, we will have to patch base/logging.h because many files in base
> include
> it.
>
>
> http://codereview.chromium.org/519020
>

[brettw, 2010-01-05 18:19:25]

On Mon, Jan 4, 2010 at 3:10 PM, Alastair Patrick <apatrick@google.com> wrote:
> Ideally we don't want the client command_buffer code to be directly
> dependent on base because it'll be linked with NaCl modules. We could break
> the dependency with a callback function for reporting diagnostics, if
> logging is the only issue:
>
> typedef void LogFunc(const char* message, const char* file, int line);
>
> Or something like that. I'm not sure where the best to put it would be.

This sounds fine to me. It sounds better than having a weird different
logging in base.

Brett