Fix gpu_video_decode_accelerator include order by renaming enum with X11 name collision

Currently, gpu_video_decode_accelerator.cc has a strange include order exception for the following reason:

(1) X11/Xlib.h contains "#define Status ..."
(2) base/tracked_objects.h contains "enum Status { ... }"
(3) gpu_video_decode_accelerator.cc indirectly includes (10 before (2)

After discussing possible fixes with danakj@, I've renamed the enum in (2).

R=danakj@chromium.org
BUG=608239
Committed: https://crrev.com/276736a4e1f1f2431a6433208ac8a3c75f2722a4
Cr-Commit-Position: refs/heads/master@{#392408}

[Mark Dittmer, 2016-05-02 14:37:54]

Fix include order that used to depend on X11 include ordering

[Mark Dittmer, 2016-05-02 14:38:08]

The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 14:38:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936093003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936093003/20001

[commit-bot: I haz the power, 2016-05-02 17:48:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-02 17:48:21]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-05-02 20:12:54]

https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:437: #undef Status  // Xlib.h #defines this, which
conflicts with enum Status below.
Do this at the places that include Xlib.h

[Mark Dittmer, 2016-05-04 14:00:07]

https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:437: #undef Status  // Xlib.h #defines this, which
conflicts with enum Status below.
On 2016/05/02 20:12:54, danakj wrote:
> Do this at the places that include Xlib.h

This seems less useful to me. Here, we are in the context where having #define'd
Status matters, and can explain (in context) why it should be #undef'd. Xlib.h
is included in >200 files and defines >50 unprefixed defines. If we are not
going to wrap it in a header that #undef's all of these, then I think putting
the #undef's in the place they're needed makes more sense than putting htem in
the, potentially unrelated, Chromium header that happens to precipitate the
issue.

Thoughts?

[danakj, 2016-05-04 20:17:57]

https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:437: #undef Status  // Xlib.h #defines this, which
conflicts with enum Status below.
On 2016/05/04 14:00:07, Mark Dittmer wrote:
> On 2016/05/02 20:12:54, danakj wrote:
> > Do this at the places that include Xlib.h
> 
> This seems less useful to me. Here, we are in the context where having
#define'd
> Status matters, and can explain (in context) why it should be #undef'd. Xlib.h
> is included in >200 files and defines >50 unprefixed defines. If we are not
> going to wrap it in a header that #undef's all of these, then I think putting
> the #undef's in the place they're needed makes more sense than putting htem in
> the, potentially unrelated, Chromium header that happens to precipitate the
> issue.
> 
> Thoughts?

Conversely this code has no idea about Xlib.h so putting it in here is strange.

If I was to do this from scratch I'd probably wrap Xlib.h in a header of our own
which undefs things, and ban including Xlib.h directly. Do you have some
pointers to context on why we don't do that?

[chromium-reviews, 2016-05-04 20:36:48]

To be honest, I'm not convinced that there's a compelling reason that we
haven't done this, other than that it's a bunch of work to get right (write
the header, ensure that it appears BEFORE other system paths in the include
path, etc.).
On 4 May 2016 4:17 p.m., <danakj@chromium.org> wrote:

>
>
> https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
> File base/tracked_objects.h (right):
>
>
>
https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
> base/tracked_objects.h:437: #undef Status // Xlib.h #defines this,
> which conflicts with enum Status below.
> On 2016/05/04 14:00:07, Mark Dittmer wrote:
> > On 2016/05/02 20:12:54, danakj wrote:
> > > Do this at the places that include Xlib.h
> >
> > This seems less useful to me. Here, we are in the context where having
> #define'd
> > Status matters, and can explain (in context) why it should be
> #undef'd. Xlib.h
> > is included in >200 files and defines >50 unprefixed defines. If we
> are not
> > going to wrap it in a header that #undef's all of these, then I think
> putting
> > the #undef's in the place they're needed makes more sense than putting
> htem in
> > the, potentially unrelated, Chromium header that happens to
> precipitate the
> > issue.
> >
> > Thoughts?
>
> Conversely this code has no idea about Xlib.h so putting it in here is
> strange.
>
> If I was to do this from scratch I'd probably wrap Xlib.h in a header of
> our own which undefs things, and ban including Xlib.h directly. Do you
> have some pointers to context on why we don't do that?
>
> https://codereview.chromium.org/1936093003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-05-04 21:19:52]

On Wed, May 4, 2016 at 1:36 PM, Mark Dittmer <markdittmer@google.com> wrote:

> To be honest, I'm not convinced that there's a compelling reason that we
> haven't done this, other than that it's a bunch of work to get right (write
> the header, ensure that it appears BEFORE other system paths in the include
> path, etc.).
>
Hm I was thinking to just add a PRESUBMIT rule to disallow Xlib.h, but it
could be included by other things we include..

I wouldn't expect to call our header Xlib.h, so I'm not sure path ordering
does much. Maybe this is just hard as you say.

It would probably catch most places to do something like in
chromium_xlib.h, assuming people did IWYU well.

#ifndef __HEAD_ERTHING
#define __HEAD_ERTHING

#ifdef Status
#error You included Xlib.h directly somehow, woops.
#endif

#include <Xlib.h>

#endif


> On 4 May 2016 4:17 p.m., <danakj@chromium.org> wrote:
>
>>
>>
>> https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
>> File base/tracked_objects.h (right):
>>
>>
>>
https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
>> base/tracked_objects.h:437: #undef Status // Xlib.h #defines this,
>> which conflicts with enum Status below.
>> On 2016/05/04 14:00:07, Mark Dittmer wrote:
>> > On 2016/05/02 20:12:54, danakj wrote:
>> > > Do this at the places that include Xlib.h
>> >
>> > This seems less useful to me. Here, we are in the context where having
>> #define'd
>> > Status matters, and can explain (in context) why it should be
>> #undef'd. Xlib.h
>> > is included in >200 files and defines >50 unprefixed defines. If we
>> are not
>> > going to wrap it in a header that #undef's all of these, then I think
>> putting
>> > the #undef's in the place they're needed makes more sense than putting
>> htem in
>> > the, potentially unrelated, Chromium header that happens to
>> precipitate the
>> > issue.
>> >
>> > Thoughts?
>>
>> Conversely this code has no idea about Xlib.h so putting it in here is
>> strange.
>>
>> If I was to do this from scratch I'd probably wrap Xlib.h in a header of
>> our own which undefs things, and ban including Xlib.h directly. Do you
>> have some pointers to context on why we don't do that?
>>
>> https://codereview.chromium.org/1936093003/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-05-04 21:22:34]

Seems like a reasonable strategy. I'll look into this tomorrow.

Do we have similar global presubmit rules that I could use as a model for
this kind of check?
On 4 May 2016 5:19 p.m., "Dana Jansens" <danakj@chromium.org> wrote:

> On Wed, May 4, 2016 at 1:36 PM, Mark Dittmer <markdittmer@google.com>
> wrote:
>
>> To be honest, I'm not convinced that there's a compelling reason that we
>> haven't done this, other than that it's a bunch of work to get right (write
>> the header, ensure that it appears BEFORE other system paths in the include
>> path, etc.).
>>
> Hm I was thinking to just add a PRESUBMIT rule to disallow Xlib.h, but it
> could be included by other things we include..
>
> I wouldn't expect to call our header Xlib.h, so I'm not sure path ordering
> does much. Maybe this is just hard as you say.
>
> It would probably catch most places to do something like in
> chromium_xlib.h, assuming people did IWYU well.
>
> #ifndef __HEAD_ERTHING
> #define __HEAD_ERTHING
>
> #ifdef Status
> #error You included Xlib.h directly somehow, woops.
> #endif
>
> #include <Xlib.h>
>
> #endif
>
>
>> On 4 May 2016 4:17 p.m., <danakj@chromium.org> wrote:
>>
>>>
>>>
>>>
https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
>>> File base/tracked_objects.h (right):
>>>
>>>
>>>
https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
>>> base/tracked_objects.h:437: #undef Status // Xlib.h #defines this,
>>> which conflicts with enum Status below.
>>> On 2016/05/04 14:00:07, Mark Dittmer wrote:
>>> > On 2016/05/02 20:12:54, danakj wrote:
>>> > > Do this at the places that include Xlib.h
>>> >
>>> > This seems less useful to me. Here, we are in the context where having
>>> #define'd
>>> > Status matters, and can explain (in context) why it should be
>>> #undef'd. Xlib.h
>>> > is included in >200 files and defines >50 unprefixed defines. If we
>>> are not
>>> > going to wrap it in a header that #undef's all of these, then I think
>>> putting
>>> > the #undef's in the place they're needed makes more sense than putting
>>> htem in
>>> > the, potentially unrelated, Chromium header that happens to
>>> precipitate the
>>> > issue.
>>> >
>>> > Thoughts?
>>>
>>> Conversely this code has no idea about Xlib.h so putting it in here is
>>> strange.
>>>
>>> If I was to do this from scratch I'd probably wrap Xlib.h in a header of
>>> our own which undefs things, and ban including Xlib.h directly. Do you
>>> have some pointers to context on why we don't do that?
>>>
>>> https://codereview.chromium.org/1936093003/
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-05-04 21:30:32]

On Wed, May 4, 2016 at 2:22 PM, Mark Dittmer <markdittmer@google.com> wrote:

> Seems like a reasonable strategy. I'll look into this tomorrow.
>
> Do we have similar global presubmit rules that I could use as a model for
> this kind of check?
>
src/PRESUBMIT.py are the rules. They are basically regexes over the diff.


> On 4 May 2016 5:19 p.m., "Dana Jansens" <danakj@chromium.org> wrote:
>
>> On Wed, May 4, 2016 at 1:36 PM, Mark Dittmer <markdittmer@google.com>
>> wrote:
>>
>>> To be honest, I'm not convinced that there's a compelling reason that we
>>> haven't done this, other than that it's a bunch of work to get right (write
>>> the header, ensure that it appears BEFORE other system paths in the include
>>> path, etc.).
>>>
>> Hm I was thinking to just add a PRESUBMIT rule to disallow Xlib.h, but it
>> could be included by other things we include..
>>
>> I wouldn't expect to call our header Xlib.h, so I'm not sure path
>> ordering does much. Maybe this is just hard as you say.
>>
>> It would probably catch most places to do something like in
>> chromium_xlib.h, assuming people did IWYU well.
>>
>> #ifndef __HEAD_ERTHING
>> #define __HEAD_ERTHING
>>
>> #ifdef Status
>> #error You included Xlib.h directly somehow, woops.
>> #endif
>>
>> #include <Xlib.h>
>>
>> #endif
>>
>>
>>> On 4 May 2016 4:17 p.m., <danakj@chromium.org> wrote:
>>>
>>>>
>>>>
>>>>
https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h
>>>> File base/tracked_objects.h (right):
>>>>
>>>>
>>>>
https://codereview.chromium.org/1936093003/diff/20001/base/tracked_objects.h#...
>>>> base/tracked_objects.h:437: #undef Status // Xlib.h #defines this,
>>>> which conflicts with enum Status below.
>>>> On 2016/05/04 14:00:07, Mark Dittmer wrote:
>>>> > On 2016/05/02 20:12:54, danakj wrote:
>>>> > > Do this at the places that include Xlib.h
>>>> >
>>>> > This seems less useful to me. Here, we are in the context where having
>>>> #define'd
>>>> > Status matters, and can explain (in context) why it should be
>>>> #undef'd. Xlib.h
>>>> > is included in >200 files and defines >50 unprefixed defines. If we
>>>> are not
>>>> > going to wrap it in a header that #undef's all of these, then I think
>>>> putting
>>>> > the #undef's in the place they're needed makes more sense than putting
>>>> htem in
>>>> > the, potentially unrelated, Chromium header that happens to
>>>> precipitate the
>>>> > issue.
>>>> >
>>>> > Thoughts?
>>>>
>>>> Conversely this code has no idea about Xlib.h so putting it in here is
>>>> strange.
>>>>
>>>> If I was to do this from scratch I'd probably wrap Xlib.h in a header of
>>>> our own which undefs things, and ban including Xlib.h directly. Do you
>>>> have some pointers to context on why we don't do that?
>>>>
>>>> https://codereview.chromium.org/1936093003/
>>>>
>>>
>>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Dittmer, 2016-05-05 14:49:14]

I think I may have discovered why this abstraction hasn't been done. Several
places that #include <X11/Xlib.h> live in third_party/*/. My understanding is
that these dependencies are oblivious to chromium and would not, for example,
admit '#include "base/chroimum_x11.h"'. Since, I assume, we may end up including
<X11/Xlib.h> via one of these third_party modules it is not straightforward to
avoid including an unwrapped version of the header.

It could still be done by running compilation units through the C pre-processor
and pattern matching against that, but this seems both expensive and
error-prone.

In light of this, should we continue to patch up issues as they arise, and leave
this patch as-is?

[danakj, 2016-05-05 19:00:00]

On Thu, May 5, 2016 at 7:49 AM, <markdittmer@chromium.org> wrote:

> I think I may have discovered why this abstraction hasn't been done.
> Several
> places that #include <X11/Xlib.h> live in third_party/*/. My understanding
> is
> that these dependencies are oblivious to chromium and would not, for
> example,
> admit '#include "base/chroimum_x11.h"'. Since, I assume, we may end up
> including
> <X11/Xlib.h> via one of these third_party modules it is not
> straightforward to
> avoid including an unwrapped version of the header.
>
> It could still be done by running compilation units through the C
> pre-processor
> and pattern matching against that, but this seems both expensive and
> error-prone.
>
> In light of this, should we continue to patch up issues as they arise, and
> leave
> this patch as-is?
>

I think my POV is that if you are going to undef Status, I still think do
that at random places that include Xlib.h as needed.

If you're going to change tracked_objects.h, then rename the Status enum.


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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Dittmer, 2016-05-06 18:23:16]

Rename Status->ThreadStatus

[Mark Dittmer, 2016-05-06 18:26:58]

Description was changed from

==========
undef Status when it may conflict with tracked_objects enum

R=danakj@chromium.org
BUG=608239
==========

to

==========
Currently, gpu_video_decode_accelerator.cc has a strange include order exception
for the following reason:

(1) X11/Xlib.h contains "#define Status ..."
(2) base/tracked_objects.h contains "enum Status { ... }"
(3) gpu_video_decode_accelerator.cc indirectly includes (10 before (2)

After discussing possible fixes with danakj@, I've renamed the enum in (2).

R=danakj@chromium.org
BUG=608239
==========

[Mark Dittmer, 2016-05-06 18:32:44]

markdittmer@chromium.org changed reviewers:
+ dcheng@chromium.org

[Mark Dittmer, 2016-05-06 18:32:44]

dcheng@chromium.org: Please review messages

[Mark Dittmer, 2016-05-06 18:33:58]

markdittmer@chromium.org changed reviewers:
+ sky@chromium.org

[Mark Dittmer, 2016-05-06 18:33:59]

sky@ PTAL at chrome/browser/chrome_browser_main.cc

[Mark Dittmer, 2016-05-06 18:34:49]

markdittmer@chromium.org changed reviewers:
+ piman@chromium.org

[Mark Dittmer, 2016-05-06 18:34:50]

sky@chromium.org: Please review changes in chrome/browser/chrome_browser_main.cc

piman@chromium.org: Please review changes in content

[Mark Dittmer, 2016-05-06 18:36:04]

markdittmer@chromium.org changed reviewers:
+ sandersd@chromium.org

[Mark Dittmer, 2016-05-06 18:36:05]

sandersd@chromium.org: Please review changes in media

[Mark Dittmer, 2016-05-06 18:36:19]

The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-06 18:36:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936093003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936093003/40001

[dcheng, 2016-05-06 19:24:01]

https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:12: #include
"ipc/ipc_message_macros.h"
I'm not sure which header is doing it, but that header probably shouldn't be
including X11. We try to remove X11 #includes from public headers as much as
possible, to prevent this very problem.

[danakj, 2016-05-06 19:41:10]

base LGTM

[piman, 2016-05-06 19:46:47]

https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:12: #include
"ipc/ipc_message_macros.h"
On 2016/05/06 19:24:00, dcheng wrote:
> I'm not sure which header is doing it, but that header probably shouldn't be
> including X11. We try to remove X11 #includes from public headers as much as
> possible, to prevent this very problem.

Agreed - do we know what includes Xlib.h here?

[sky, 2016-05-06 21:49:01]

LGTM

[commit-bot: I haz the power, 2016-05-06 22:02:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-06 22:02:40]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-05-06 22:28:23]

media/ lgtm

[Mark Dittmer, 2016-05-09 15:12:43]

Identified the offending include chain. Each step appears to be genuinely
necessary.

https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_decode_accelerator.cc:12: #include
"ipc/ipc_message_macros.h"
On 2016/05/06 19:46:47, piman wrote:
> On 2016/05/06 19:24:00, dcheng wrote:
> > I'm not sure which header is doing it, but that header probably shouldn't be
> > including X11. We try to remove X11 #includes from public headers as much as
> > possible, to prevent this very problem.
> 
> Agreed - do we know what includes Xlib.h here?

The relevant preprocessor trace is as follows:

# 19 "../../media/gpu/ipc/service/gpu_video_decode_accelerator.h" 2
# 22 "../../gpu/command_buffer/service/texture_manager.h" 2
# 14 "../../gpu/command_buffer/service/gl_utils.h"
# 22 "../../ui/gl/gl_bindings.h" 2
# 39 "../../third_party/khronos/EGL/egl.h"
# 106 "../../third_party/khronos/EGL/eglplatform.h"
# 1 "/usr/include/X11/Xlib.h" 1 3 4

This shows up on GYP chromeos builds (e.g., build [1] on issue [2]).

[1]
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
[2] https://codereview.chromium.org/1958903004

[Mark Dittmer, 2016-05-09 15:13:00]

The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-09 15:13:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936093003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936093003/40001

[commit-bot: I haz the power, 2016-05-09 16:34:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-09 16:34:58]

Dry run: This issue passed the CQ dry run.

[piman, 2016-05-09 16:39:33]

On Mon, May 9, 2016 at 8:12 AM, <markdittmer@chromium.org> wrote:

> Identified the offending include chain. Each step appears to be genuinely
> necessary.
>
>
>
>
https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
> File media/gpu/ipc/service/gpu_video_decode_accelerator.cc (left):
>
>
>
https://codereview.chromium.org/1936093003/diff/40001/media/gpu/ipc/service/g...
> media/gpu/ipc/service/gpu_video_decode_accelerator.cc:12: #include
> "ipc/ipc_message_macros.h"
> On 2016/05/06 19:46:47, piman wrote:
> > On 2016/05/06 19:24:00, dcheng wrote:
> > > I'm not sure which header is doing it, but that header probably
> shouldn't be
> > > including X11. We try to remove X11 #includes from public headers as
> much as
> > > possible, to prevent this very problem.
> >
> > Agreed - do we know what includes Xlib.h here?
>
> The relevant preprocessor trace is as follows:
>
> # 19 "../../media/gpu/ipc/service/gpu_video_decode_accelerator.h" 2
> # 22 "../../gpu/command_buffer/service/texture_manager.h" 2
> # 14 "../../gpu/command_buffer/service/gl_utils.h"
> # 22 "../../ui/gl/gl_bindings.h" 2
> # 39 "../../third_party/khronos/EGL/egl.h"
> # 106 "../../third_party/khronos/EGL/eglplatform.h"
> # 1 "/usr/include/X11/Xlib.h" 1 3 4
>

I don't think we need gl_utils.h in texture_manager.h
Separately, I don't think texture_manager.h is needed in
gpu_video_decode_accelerator.h (can we forward-declare gpu::gles2::TextureRef
?)
We can cut headers in smaller pieces if needed, but bottom line, there's no
good reason egl.h should be included in gpu_video_decode_accelerator.h

Antoine


>
> This shows up on GYP chromeos builds (e.g., build [1] on issue [2]).
>
> [1]
>
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> [2] https://codereview.chromium.org/1958903004
>
> https://codereview.chromium.org/1936093003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Dittmer, 2016-05-09 18:13:12]

Move misplaced #endif to preserve #undefs across platforms

[Mark Dittmer, 2016-05-09 18:13:35]

The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-09 18:14:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936093003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936093003/60001

[Mark Dittmer, 2016-05-09 18:16:53]

On 2016/05/09 16:39:33, piman wrote:
> We can cut headers in smaller pieces if needed, but bottom line, there's no
> good reason egl.h should be included in gpu_video_decode_accelerator.h

Very good. Created https://bugs.chromium.org/p/chromium/issues/detail?id=610387
to track.

[Mark Dittmer, 2016-05-09 18:18:09]

Latest patch fixes issue by recognizing that all platforms that included egl
headers should also #undef pollution from those headers. Previously, this was
only done in a particular #if branch.

[commit-bot: I haz the power, 2016-05-09 19:31:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-09 19:31:32]

Dry run: This issue passed the CQ dry run.

[Mark Dittmer, 2016-05-09 20:08:47]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-05-09 20:08:48]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1936093003/#ps60001
(title: "Move misplaced #endif to preserve #undefs across platforms")

[commit-bot: I haz the power, 2016-05-09 20:09:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936093003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936093003/60001

[commit-bot: I haz the power, 2016-05-09 20:13:34]

Description was changed from

==========
Currently, gpu_video_decode_accelerator.cc has a strange include order exception
for the following reason:

(1) X11/Xlib.h contains "#define Status ..."
(2) base/tracked_objects.h contains "enum Status { ... }"
(3) gpu_video_decode_accelerator.cc indirectly includes (10 before (2)

After discussing possible fixes with danakj@, I've renamed the enum in (2).

R=danakj@chromium.org
BUG=608239
==========

to

==========
Currently, gpu_video_decode_accelerator.cc has a strange include order exception
for the following reason:

(1) X11/Xlib.h contains "#define Status ..."
(2) base/tracked_objects.h contains "enum Status { ... }"
(3) gpu_video_decode_accelerator.cc indirectly includes (10 before (2)

After discussing possible fixes with danakj@, I've renamed the enum in (2).

R=danakj@chromium.org
BUG=608239
==========

[commit-bot: I haz the power, 2016-05-09 20:13:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-09 20:14:32]

Description was changed from

==========
Currently, gpu_video_decode_accelerator.cc has a strange include order exception
for the following reason:

(1) X11/Xlib.h contains "#define Status ..."
(2) base/tracked_objects.h contains "enum Status { ... }"
(3) gpu_video_decode_accelerator.cc indirectly includes (10 before (2)

After discussing possible fixes with danakj@, I've renamed the enum in (2).

R=danakj@chromium.org
BUG=608239
==========

to

==========
Currently, gpu_video_decode_accelerator.cc has a strange include order exception
for the following reason:

(1) X11/Xlib.h contains "#define Status ..."
(2) base/tracked_objects.h contains "enum Status { ... }"
(3) gpu_video_decode_accelerator.cc indirectly includes (10 before (2)

After discussing possible fixes with danakj@, I've renamed the enum in (2).

R=danakj@chromium.org
BUG=608239

Committed: https://crrev.com/276736a4e1f1f2431a6433208ac8a3c75f2722a4
Cr-Commit-Position: refs/heads/master@{#392408}
==========

[commit-bot: I haz the power, 2016-05-09 20:14:33]

Patchset 4 (id:??) landed as
https://crrev.com/276736a4e1f1f2431a6433208ac8a3c75f2722a4
Cr-Commit-Position: refs/heads/master@{#392408}