Mojo: First stab at making MessagePipes work across OS pipes.

Mojo: First stab at making MessagePipes work across OS pipes.

Given a running RawChannel, one can set up MessagePipes that have one endpoint
available locally (in the usual way) and the other endpoint proxied to the other
side of the OS-level "pipe" (which presumably has a symmetrical setup -- i.e.,
another RawChannel, etc.).

Currently, this has only been tested in-process, but apart from possible
synchronization/bootstrapping issues there's no reason it shouldn't work across
processes. (Whatever launches the process will have to begin the bootstrapping
by getting an OS pipe between processes and making sure things are appropriately
synchronized.)

Still to do:
  - Properly handle errors (e.g., due to the pipe/process dying).
  - Figure out how to start processes and bootstrap in that situation (and test
    this).

R=darin@chromium.org, darin

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233638

[viettrungluu, 2013-11-05 22:03:13]

I'm not super-happy with the way this is designed (it feels very complicated),
but it'll have to do for now....

[darin (slow to review), 2013-11-06 18:25:59]

This all looks reasonable. LGTM w/ nits picked. I'm curious what you are unhappy
about with this design. I'm also curious how CoreImpl will go about arranging
for AttachMessagePipeEndpoint and RunMessagePipeEndpoint to get called in the
cross-process case.

Perhaps the primordial MessagePipe is just special (setup using a private API),
and then after that MessagePipes transition into being non-local by virtue of
being passed over a non-local MessagePipe?

https://codereview.chromium.org/60103005/diff/1/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/60103005/diff/1/base/compiler_specific.h#newc...
base/compiler_specific.h:88: #define STATIC_CONST_MEMBER_DEFINITION
__declspec(selectany)
I'm surprised we haven't needed this until now. Take a look at
src/base/vlog.{h,cc} and VlogInfo::kDefaultVlogLevel.

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc
File mojo/system/channel.cc (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc#newcode45
mojo/system/channel.cc:45: // TODO(vtl): Should there be an explicit |Init()|
instead?
Do you mean Channel::Init()? That might be wise. It looks like
RawChannelPosix::Init() can actually encounter errors. Maybe it should even
return bool?

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h
File mojo/system/channel.h (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h#newcode119
mojo/system/channel.h:119: base::MessageLoop* creation_thread_message_loop_;
Maybe you want to use base::ThreadChecker here instead?

https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transit.h
File mojo/system/message_in_transit.h (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transi...
mojo/system/message_in_transit.h:23: static const Type
TYPE_MESSAGE_PIPE_ENDPOINT = 0;
nit: shouldn't these be formatted using kFooBar style?

https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transi...
mojo/system/message_in_transit.h:73: void set_type(Type type) { type_ = type; }
note: set_type and set_subtype do not appear to be called in this CL. maybe they
are not needed?

https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
File mojo/system/proxy_message_pipe_endpoint.cc (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
mojo/system/proxy_message_pipe_endpoint.cc:73: (message->type() ==
MessageInTransit::TYPE_MESSAGE_PIPE));
perhaps it would be gratuitous to also check that the subtype is
SUBTYPE_MESSAGE_PIPE_PEER_CLOSED

https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
mojo/system/proxy_message_pipe_endpoint.cc:81: rv =
MOJO_RESULT_FAILED_PRECONDITION;
What does this error code mean in this context? It seems like WriteMessage could
fail if there was an error on the underlying pipe (RawChannel). Maybe it is a
precondition of this function that the pipe will accept writes?

In general, it would be helpful if our error codes helps people debug issues.
The more specific they are the more useful they will be in debugging problems.

[viettrungluu, 2013-11-06 18:55:00]

On 2013/11/06 18:25:59, darin wrote:
> This all looks reasonable. LGTM w/ nits picked. I'm curious what you are
unhappy
> about with this design. I'm also curious how CoreImpl will go about arranging
> for AttachMessagePipeEndpoint and RunMessagePipeEndpoint to get called in the
> cross-process case.
> 
> Perhaps the primordial MessagePipe is just special (setup using a private
API),
> and then after that MessagePipes transition into being non-local by virtue of
> being passed over a non-local MessagePipe?

Yes, I think the first MessagePipe has to be special (in exactly the way you
describe). (It's also intimately tied to the way new processes are launched.)

> 
> https://codereview.chromium.org/60103005/diff/1/base/compiler_specific.h
> File base/compiler_specific.h (right):
> 
>
https://codereview.chromium.org/60103005/diff/1/base/compiler_specific.h#newc...
> base/compiler_specific.h:88: #define STATIC_CONST_MEMBER_DEFINITION
> __declspec(selectany)
> I'm surprised we haven't needed this until now. Take a look at
> src/base/vlog.{h,cc} and VlogInfo::kDefaultVlogLevel.

You only run into this problem if you want to define the value of the static
const member in the header file itself (which is good, since it allows
optimization).

Currently the solution is either to not define the value in the header (like in
src/base/vlog.h) or to manually ifdef it whenever someone encounters/remembers
the problem (e.g., see src/gpu/command_buffer/service/test_helper.cc).

Mostly you don't notice the problem until you try to do something like pushing
the constant into an std::vector (whose methods take reference arguments).
Strictly speaking, using __declspec(selectany) should be more
standards-compliant since then &Foo::kBar should always give the same address
with MSVC (I think).

(I'll look at/respond to your other comments later ... since I can't save this
reply in rietveld.)

> 
> https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc
> File mojo/system/channel.cc (right):
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc#newcode45
> mojo/system/channel.cc:45: // TODO(vtl): Should there be an explicit |Init()|
> instead?
> Do you mean Channel::Init()? That might be wise. It looks like
> RawChannelPosix::Init() can actually encounter errors. Maybe it should even
> return bool?
> 
> https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h
> File mojo/system/channel.h (right):
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h#newcode119
> mojo/system/channel.h:119: base::MessageLoop* creation_thread_message_loop_;
> Maybe you want to use base::ThreadChecker here instead?
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transit.h
> File mojo/system/message_in_transit.h (right):
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transi...
> mojo/system/message_in_transit.h:23: static const Type
> TYPE_MESSAGE_PIPE_ENDPOINT = 0;
> nit: shouldn't these be formatted using kFooBar style?
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transi...
> mojo/system/message_in_transit.h:73: void set_type(Type type) { type_ = type;
}
> note: set_type and set_subtype do not appear to be called in this CL. maybe
they
> are not needed?
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
> File mojo/system/proxy_message_pipe_endpoint.cc (right):
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
> mojo/system/proxy_message_pipe_endpoint.cc:73: (message->type() ==
> MessageInTransit::TYPE_MESSAGE_PIPE));
> perhaps it would be gratuitous to also check that the subtype is
> SUBTYPE_MESSAGE_PIPE_PEER_CLOSED
> 
>
https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
> mojo/system/proxy_message_pipe_endpoint.cc:81: rv =
> MOJO_RESULT_FAILED_PRECONDITION;
> What does this error code mean in this context? It seems like WriteMessage
could
> fail if there was an error on the underlying pipe (RawChannel). Maybe it is a
> precondition of this function that the pipe will accept writes?
> 
> In general, it would be helpful if our error codes helps people debug issues.
> The more specific they are the more useful they will be in debugging problems.

[darin (slow to review), 2013-11-06 20:23:11]

On Wed, Nov 6, 2013 at 10:55 AM, <viettrungluu@chromium.org> wrote:

> On 2013/11/06 18:25:59, darin wrote:
>
>> This all looks reasonable. LGTM w/ nits picked. I'm curious what you are
>>
> unhappy
>
>> about with this design. I'm also curious how CoreImpl will go about
>> arranging
>> for AttachMessagePipeEndpoint and RunMessagePipeEndpoint to get called in
>> the
>> cross-process case.
>>
>
>  Perhaps the primordial MessagePipe is just special (setup using a private
>>
> API),
>
>> and then after that MessagePipes transition into being non-local by
>> virtue of
>> being passed over a non-local MessagePipe?
>>
>
> Yes, I think the first MessagePipe has to be special (in exactly the way
> you
> describe). (It's also intimately tied to the way new processes are
> launched.)
>
>
OK


>
>
>  https://codereview.chromium.org/60103005/diff/1/base/compiler_specific.h
>> File base/compiler_specific.h (right):
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/base/
> compiler_specific.h#newcode88
>
>> base/compiler_specific.h:88: #define STATIC_CONST_MEMBER_DEFINITION
>> __declspec(selectany)
>> I'm surprised we haven't needed this until now. Take a look at
>> src/base/vlog.{h,cc} and VlogInfo::kDefaultVlogLevel.
>>
>
> You only run into this problem if you want to define the value of the
> static
> const member in the header file itself (which is good, since it allows
> optimization).
>
> Currently the solution is either to not define the value in the header
> (like in
> src/base/vlog.h) or to manually ifdef it whenever someone
> encounters/remembers
> the problem (e.g., see src/gpu/command_buffer/service/test_helper.cc).
>
> Mostly you don't notice the problem until you try to do something like
> pushing
> the constant into an std::vector (whose methods take reference arguments).
> Strictly speaking, using __declspec(selectany) should be more
> standards-compliant since then &Foo::kBar should always give the same
> address
> with MSVC (I think).
>

Ah, OK. Thanks for the explanation. This particular change might get the
attention of others on the broader Chromium team. You might want to send a
PSA about this macro to chromium-dev@.

-Darin



>
> (I'll look at/respond to your other comments later ... since I can't save
> this
> reply in rietveld.)
>
>
>
>  https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc
>> File mojo/system/channel.cc (right):
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/channel.cc#newcode45
>
>> mojo/system/channel.cc:45: // TODO(vtl): Should there be an explicit
>> |Init()|
>> instead?
>> Do you mean Channel::Init()? That might be wise. It looks like
>> RawChannelPosix::Init() can actually encounter errors. Maybe it should
>> even
>> return bool?
>>
>
>  https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h
>> File mojo/system/channel.h (right):
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/channel.h#newcode119
>
>> mojo/system/channel.h:119: base::MessageLoop*
>> creation_thread_message_loop_;
>> Maybe you want to use base::ThreadChecker here instead?
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/message_in_transit.h
>
>> File mojo/system/message_in_transit.h (right):
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/message_in_transit.h#newcode23
>
>> mojo/system/message_in_transit.h:23: static const Type
>> TYPE_MESSAGE_PIPE_ENDPOINT = 0;
>> nit: shouldn't these be formatted using kFooBar style?
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/message_in_transit.h#newcode73
>
>> mojo/system/message_in_transit.h:73: void set_type(Type type) { type_ =
>> type;
>>
> }
>
>> note: set_type and set_subtype do not appear to be called in this CL.
>> maybe
>>
> they
>
>> are not needed?
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/proxy_message_pipe_endpoint.cc
>
>> File mojo/system/proxy_message_pipe_endpoint.cc (right):
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/proxy_message_pipe_endpoint.cc#newcode73
>
>> mojo/system/proxy_message_pipe_endpoint.cc:73: (message->type() ==
>> MessageInTransit::TYPE_MESSAGE_PIPE));
>> perhaps it would be gratuitous to also check that the subtype is
>> SUBTYPE_MESSAGE_PIPE_PEER_CLOSED
>>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/proxy_message_pipe_endpoint.cc#newcode81
>
>> mojo/system/proxy_message_pipe_endpoint.cc:81: rv =
>> MOJO_RESULT_FAILED_PRECONDITION;
>> What does this error code mean in this context? It seems like WriteMessage
>>
> could
>
>> fail if there was an error on the underlying pipe (RawChannel). Maybe it
>> is a
>> precondition of this function that the pipe will accept writes?
>>
>
>  In general, it would be helpful if our error codes helps people debug
>> issues.
>> The more specific they are the more useful they will be in debugging
>> problems.
>>
>
>
> https://codereview.chromium.org/60103005/
>

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

[viettrungluu, 2013-11-06 21:13:39]

Thanks for the review.

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc
File mojo/system/channel.cc (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc#newcode45
mojo/system/channel.cc:45: // TODO(vtl): Should there be an explicit |Init()|
instead?
On 2013/11/06 18:26:00, darin wrote:
> Do you mean Channel::Init()? That might be wise. It looks like
> RawChannelPosix::Init() can actually encounter errors. Maybe it should even
> return bool?

Done.

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h
File mojo/system/channel.h (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h#newcode119
mojo/system/channel.h:119: base::MessageLoop* creation_thread_message_loop_;
On 2013/11/06 18:26:00, darin wrote:
> Maybe you want to use base::ThreadChecker here instead?

Done.

https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transit.h
File mojo/system/message_in_transit.h (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transi...
mojo/system/message_in_transit.h:23: static const Type
TYPE_MESSAGE_PIPE_ENDPOINT = 0;
On 2013/11/06 18:26:00, darin wrote:
> nit: shouldn't these be formatted using kFooBar style?

Fixed. (They were enum values before, until I decided they should be properly
typed. Maybe that's an argument for the current Google C++ style of kFooBar even
for enum values.)

https://codereview.chromium.org/60103005/diff/1/mojo/system/message_in_transi...
mojo/system/message_in_transit.h:73: void set_type(Type type) { type_ = type; }
On 2013/11/06 18:26:00, darin wrote:
> note: set_type and set_subtype do not appear to be called in this CL. maybe
they
> are not needed?

Thanks, I deleted them (they were left over from when |Create()| didn't take
|type| and |subtype| arguments).

https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
File mojo/system/proxy_message_pipe_endpoint.cc (right):

https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
mojo/system/proxy_message_pipe_endpoint.cc:73: (message->type() ==
MessageInTransit::TYPE_MESSAGE_PIPE));
On 2013/11/06 18:26:00, darin wrote:
> perhaps it would be gratuitous to also check that the subtype is
> SUBTYPE_MESSAGE_PIPE_PEER_CLOSED

I'm not asserting that because there may be more control messages that could be
sent in this case (maybe even in the near future).

https://codereview.chromium.org/60103005/diff/1/mojo/system/proxy_message_pip...
mojo/system/proxy_message_pipe_endpoint.cc:81: rv =
MOJO_RESULT_FAILED_PRECONDITION;
On 2013/11/06 18:26:00, darin wrote:
> What does this error code mean in this context? It seems like WriteMessage
could
> fail if there was an error on the underlying pipe (RawChannel). Maybe it is a
> precondition of this function that the pipe will accept writes?

Yes, and it indicates that this won't succeed without something being fixed.

> 
> In general, it would be helpful if our error codes helps people debug issues.
> The more specific they are the more useful they will be in debugging problems.

I have a general TODO to add some more specific error codes. There's a balance
to be struck though. On the one hand, to handle errors, it's convenient to have
error codes grouped by the action you should take (and, in some sense, the fewer
error codes the better). On the other, there's help for debugging (though
perhaps some of that could be alleviated by suitable logging facilities).

I *do* intend to add some more specific error codes, but I don't want to get too
specific since that would make actually handling errors in a sensible way
impossible.

[commit-bot: I haz the power, 2013-11-06 21:32:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/210001

[commit-bot: I haz the power, 2013-11-06 22:02:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-11-06 22:24:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/410001

[commit-bot: I haz the power, 2013-11-07 00:03:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-07 00:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/620001

[commit-bot: I haz the power, 2013-11-07 00:50:53]

Failed to trigger a try job on win
HTTP Error 400: Bad Request

[darin (slow to review), 2013-11-07 00:51:25]

On Wed, Nov 6, 2013 at 1:13 PM, <viettrungluu@chromium.org> wrote:

> Thanks for the review.
>
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.cc
> File mojo/system/channel.cc (right):
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/channel.cc#newcode45
> mojo/system/channel.cc:45: // TODO(vtl): Should there be an explicit
> |Init()| instead?
> On 2013/11/06 18:26:00, darin wrote:
>
>> Do you mean Channel::Init()? That might be wise. It looks like
>> RawChannelPosix::Init() can actually encounter errors. Maybe it should
>>
> even
>
>> return bool?
>>
>
> Done.
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/system/channel.h
> File mojo/system/channel.h (right):
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/channel.h#newcode119
> mojo/system/channel.h:119: base::MessageLoop*
> creation_thread_message_loop_;
> On 2013/11/06 18:26:00, darin wrote:
>
>> Maybe you want to use base::ThreadChecker here instead?
>>
>
> Done.
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/message_in_transit.h
> File mojo/system/message_in_transit.h (right):
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/message_in_transit.h#newcode23
> mojo/system/message_in_transit.h:23: static const Type
> TYPE_MESSAGE_PIPE_ENDPOINT = 0;
> On 2013/11/06 18:26:00, darin wrote:
>
>> nit: shouldn't these be formatted using kFooBar style?
>>
>
> Fixed. (They were enum values before, until I decided they should be
> properly typed. Maybe that's an argument for the current Google C++
> style of kFooBar even for enum values.)
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/message_in_transit.h#newcode73
> mojo/system/message_in_transit.h:73: void set_type(Type type) { type_ =
> type; }
> On 2013/11/06 18:26:00, darin wrote:
>
>> note: set_type and set_subtype do not appear to be called in this CL.
>>
> maybe they
>
>> are not needed?
>>
>
> Thanks, I deleted them (they were left over from when |Create()| didn't
> take |type| and |subtype| arguments).
>
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/proxy_message_pipe_endpoint.cc
> File mojo/system/proxy_message_pipe_endpoint.cc (right):
>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/proxy_message_pipe_endpoint.cc#newcode73
> mojo/system/proxy_message_pipe_endpoint.cc:73: (message->type() ==
> MessageInTransit::TYPE_MESSAGE_PIPE));
> On 2013/11/06 18:26:00, darin wrote:
>
>> perhaps it would be gratuitous to also check that the subtype is
>> SUBTYPE_MESSAGE_PIPE_PEER_CLOSED
>>
>
> I'm not asserting that because there may be more control messages that
> could be sent in this case (maybe even in the near future).
>
>
Makes sense.


>
> https://codereview.chromium.org/60103005/diff/1/mojo/
> system/proxy_message_pipe_endpoint.cc#newcode81
> mojo/system/proxy_message_pipe_endpoint.cc:81: rv =
> MOJO_RESULT_FAILED_PRECONDITION;
> On 2013/11/06 18:26:00, darin wrote:
>
>> What does this error code mean in this context? It seems like
>>
> WriteMessage could
>
>> fail if there was an error on the underlying pipe (RawChannel). Maybe
>>
> it is a
>
>> precondition of this function that the pipe will accept writes?
>>
>
> Yes, and it indicates that this won't succeed without something being
> fixed.
>
>
>
>  In general, it would be helpful if our error codes helps people debug
>>
> issues.
>
>> The more specific they are the more useful they will be in debugging
>>
> problems.
>
> I have a general TODO to add some more specific error codes. There's a
> balance to be struck though. On the one hand, to handle errors, it's
> convenient to have error codes grouped by the action you should take
> (and, in some sense, the fewer error codes the better). On the other,
> there's help for debugging (though perhaps some of that could be
> alleviated by suitable logging facilities).
>
> I *do* intend to add some more specific error codes, but I don't want to
> get too specific since that would make actually handling errors in a
> sensible way impossible.
>

Good point.



>
> https://codereview.chromium.org/60103005/
>

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

[commit-bot: I haz the power, 2013-11-07 00:52:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/770001

[commit-bot: I haz the power, 2013-11-07 02:13:42]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-07 02:25:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/910001

[commit-bot: I haz the power, 2013-11-07 02:54:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-07 06:55:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/110...

[commit-bot: I haz the power, 2013-11-07 07:04:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-11-07 17:29:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/60103005/110...

[viettrungluu, 2013-11-07 17:49:57]

Committed patchset #8 manually as r233638 (presubmit successful).

[Nico, 2013-11-07 19:50:34]

https://codereview.chromium.org/60103005/diff/1100001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/60103005/diff/1100001/base/compiler_specific....
base/compiler_specific.h:91: #endif
Ugh, can we just use an enum? That's what we've done so far and it works well
enough. Do you really need this?

[viettrungluu, 2013-11-08 19:05:40]

https://codereview.chromium.org/60103005/diff/1100001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/60103005/diff/1100001/base/compiler_specific....
base/compiler_specific.h:91: #endif
On 2013/11/07 19:50:35, Nico (ooo until Nov 12) wrote:
> Ugh, can we just use an enum? That's what we've done so far and it works well
> enough. Do you really need this?

a) enums aren't typed, which is unfortunate.
b) It isn't what we've done so far. So far, people mostly don't encounter the
problem (if they don't pass their static const members as arguments to things
taking reference arguments) or manually ifdef out based on COMPILER_MSVC if they
do encounter the problem. (Or, maybe sometimes people don't declare the values
of their static const members in the header when they should.)

[Nico, 2013-11-09 06:34:41]

On Fri, Nov 8, 2013 at 11:05 AM, <viettrungluu@chromium.org> wrote:

>
> https://codereview.chromium.org/60103005/diff/1100001/
> base/compiler_specific.h
> File base/compiler_specific.h (right):
>
> https://codereview.chromium.org/60103005/diff/1100001/
> base/compiler_specific.h#newcode91
> base/compiler_specific.h:91: #endif
> On 2013/11/07 19:50:35, Nico (ooo until Nov 12) wrote:
>
>> Ugh, can we just use an enum? That's what we've done so far and it
>>
> works well
>
>> enough. Do you really need this?
>>
>
> a) enums aren't typed, which is unfortunate.
> b) It isn't what we've done so far.


https://code.google.com/p/chromium/codesearch#search/&q=%5C%20%2Benum&sq=pack...



> So far, people mostly don't
> encounter the problem (if they don't pass their static const members as
> arguments to things taking reference arguments) or manually ifdef out
> based on COMPILER_MSVC if they do encounter the problem. (Or, maybe
> sometimes people don't declare the values of their static const members
> in the header when they should.)
>
> https://codereview.chromium.org/60103005/
>

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

[Nico, 2013-12-02 21:47:54]

On 2013/11/09 06:34:41, Nico wrote:
> On Fri, Nov 8, 2013 at 11:05 AM, <mailto:viettrungluu@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/60103005/diff/1100001/
> > base/compiler_specific.h
> > File base/compiler_specific.h (right):
> >
> > https://codereview.chromium.org/60103005/diff/1100001/
> > base/compiler_specific.h#newcode91
> > base/compiler_specific.h:91: #endif
> > On 2013/11/07 19:50:35, Nico (ooo until Nov 12) wrote:
> >
> >> Ugh, can we just use an enum? That's what we've done so far and it
> >>
> > works well
> >
> >> enough. Do you really need this?
> >>
> >
> > a) enums aren't typed, which is unfortunate.
> > b) It isn't what we've done so far.

One month later, this macro is still only used in the two files added in this
patch. I'm not sure if it belongs in base. And we use enums for this in many
other places.

[Nico, 2015-08-12 17:24:06]

Another 20 months later, it's used in even fewer places. I'm removing it again
in https://codereview.chromium.org/1291623002/