Include SkTypes so that SK_SUPPORT_GPU is meaningful.

Include SkTypes so that SK_SUPPORT_GPU is meaningful.

For the Android framework build, we get our defines from
SkUserConfig, rather than from the makefile, so we need to
include it (via SkTypes) before we can use our defines.

Fixes Android framework build.

Committed: https://skia.googlesource.com/skia/+/ccae588932ac7966948ae9291f496fdb04901985

[scroggo, 2014-11-04 15:48:43]

scroggo@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2014-11-04 15:50:08]

lgtm

[scroggo, 2014-11-04 15:51:19]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-11-04 15:52:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/700893002/1

[reed1, 2014-11-04 15:52:34]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-11-04 15:52:34]

or we could just move the 4-5 includes (e.g. SkCanvas.h) above the #ifdef
check... Perhaps w/ a comment why they are there before the check.

[reed1, 2014-11-04 15:52:38]

The CQ bit was unchecked by reed@google.com

[scroggo, 2014-11-04 15:59:56]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-11-04 16:00:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/700893002/20001

[reed1, 2014-11-04 16:05:06]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-11-04 16:05:29]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-04 16:05:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/700893002/20001

[commit-bot: I haz the power, 2014-11-04 16:11:10]

Committed patchset #2 (id:20001) as ccae588932ac7966948ae9291f496fdb04901985

[scroggo, 2014-11-04 16:29:27]

On 2014/11/04 15:52:34, reed1 wrote:
> or we could just move the 4-5 includes (e.g. SkCanvas.h) above the #ifdef
> check... Perhaps w/ a comment why they are there before the check.

Was that really worth removing from the CQ? I'm trying to fix a build...

I've taken your advice, but I'm not sure it's better (except for the comment -
in general
I'm in favor of more comments).

FWIW, in https://codereview.chromium.org/394533003/ and
https://codereview.chromium.org/294393011/
we decided to include SkTypes, rather than reordering includes. Also, the style
guide says,
"the correct way to get access to the config system is to #include SkTypes.h."
It's tucked away
in a section on memory management, but if defining SK_SUPPORT_GPU is a part of
the "config system"
(which it is for the Android framework), then that seems to apply here.

I was about to update the style guide and send an email to the team with advice
on this issue.
Do you think it should say to reorder includes? Should we open up this
discussion to the team
as a whole?

We'll still get breakages like this though, unless we do some kind of presubmit
check. I don't
think it's feasible to add an Android framework builder to the CQ, but maybe it
would be possible
to do a special build where we read all our defines from a custom SkUserConfig
rather than from
the makefiles. I'm guessing this would be tricky - we'd need to write some code
to do some gyp
processing along the way (perhaps similar to what's done in gyp_to_android.py).
There was a
discussion recently on skia-team about using a bot to check/enforce our include
ordering. It did
not seem to get much traction, but we could use something like that for this
problem as well.
(And it would likely be simpler than the other approach I suggested.)

[reed1, 2014-11-04 17:25:32]

On 2014/11/04 16:29:27, scroggo wrote:
> On 2014/11/04 15:52:34, reed1 wrote:
> > or we could just move the 4-5 includes (e.g. SkCanvas.h) above the #ifdef
> > check... Perhaps w/ a comment why they are there before the check.
> 
> Was that really worth removing from the CQ? I'm trying to fix a build...
> 
> I've taken your advice, but I'm not sure it's better (except for the comment -
> in general
> I'm in favor of more comments).
> 
> FWIW, in https://codereview.chromium.org/394533003/ and
> https://codereview.chromium.org/294393011/
> we decided to include SkTypes, rather than reordering includes. Also, the
style
> guide says,
> "the correct way to get access to the config system is to #include SkTypes.h."
> It's tucked away
> in a section on memory management, but if defining SK_SUPPORT_GPU is a part of
> the "config system"
> (which it is for the Android framework), then that seems to apply here.
> 
> I was about to update the style guide and send an email to the team with
advice
> on this issue.
> Do you think it should say to reorder includes? Should we open up this
> discussion to the team
> as a whole?
> 
> We'll still get breakages like this though, unless we do some kind of
presubmit
> check. I don't
> think it's feasible to add an Android framework builder to the CQ, but maybe
it
> would be possible
> to do a special build where we read all our defines from a custom SkUserConfig
> rather than from
> the makefiles. I'm guessing this would be tricky - we'd need to write some
code
> to do some gyp
> processing along the way (perhaps similar to what's done in
gyp_to_android.py).
> There was a
> discussion recently on skia-team about using a bot to check/enforce our
include
> ordering. It did
> not seem to get much traction, but we could use something like that for this
> problem as well.
> (And it would likely be simpler than the other approach I suggested.)

I stopped it, not seeing any comments about my comments, and then restarted it
about a minute later. Sorry for the delay.