Improve Skia configuration.

Improve Skia configuration.

This is the Chromium side of https://codereview.chromium.org/19808007 .
This change would be required  after landing the Skia change, and
should not produce any observable effects as things currently stand.

[djsollen, 2013-07-25 15:30:18]

https://codereview.chromium.org/19477005/diff/4001/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/19477005/diff/4001/skia/skia.gyp#newcode8
skia/skia.gyp:8: 'target_name': 'skia_configuration',
your going to need to rebase this change on the new skia.gyp as this entire file
was recently refactored by robert and I.

[bungeman-chromium, 2013-09-26 22:42:46]

I've built this at patch set 3 with
https://codereview.chromium.org/19808007/#ps37001 . The changes to skia_common
and skia_library_opts would go in before the Skia changes, and the changes to
skia_library would go in with the roll to the revision of Skia with the Skia
changes.

https://codereview.chromium.org/19477005/diff/11001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_common.gypi#new...
skia/skia_common.gypi:37: }, {
djsollen: is there something that needs to go here for android framework?

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library.gypi
File skia/skia_library.gypi (left):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library.gypi#ol...
skia/skia_library.gypi:101:
'../third_party/skia/src/ports/SkThread_pthread.cpp',
The changes to this file would have to be done with the roll which includes
https://codereview.chromium.org/19808007/ .

[djsollen, 2013-09-27 13:52:25]

instead of doing a two stage change around the Skia roll, you should check with
Robert about doing the change as part of the DEPS roll.

https://codereview.chromium.org/19477005/diff/11001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_common.gypi#new...
skia/skia_common.gypi:37: }, {
On 2013/09/26 22:42:46, bungeman2 wrote:
> djsollen: is there something that needs to go here for android framework?

If the OS is android and use_system_skia is defined to 1 then you'll want to use
the android headers.

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library.gypi#ne...
skia/skia_library.gypi:388:
'../third_party/skia/src/ports/SkFontMgr_default_dw.cpp',
seems like this is unrelated to the rest of the CL.

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library_opts.gy...
skia/skia_library_opts.gyp:34: '..',
if '..' is now a requirement due to the something defined in skia_common.gypi
shouldn't it be added to the include list in that file instead of all the
targets that use it?

[bungeman-skia, 2013-09-27 17:59:12]

https://codereview.chromium.org/19477005/diff/11001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_common.gypi#new...
skia/skia_common.gypi:37: }, {
On 2013/09/27 13:52:25, djsollen wrote:
> On 2013/09/26 22:42:46, bungeman2 wrote:
> > djsollen: is there something that needs to go here for android framework?
> 
> If the OS is android and use_system_skia is defined to 1 then you'll want to
use
> the android headers.

Done.

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library.gypi#ne...
skia/skia_library.gypi:388:
'../third_party/skia/src/ports/SkFontMgr_default_dw.cpp',
On 2013/09/27 13:52:25, djsollen wrote:
> seems like this is unrelated to the rest of the CL.

Yes, I suppose I should remove these for this CL. Done.

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/19477005/diff/11001/skia/skia_library_opts.gy...
skia/skia_library_opts.gyp:34: '..',
On 2013/09/27 13:52:25, djsollen wrote:
> if '..' is now a requirement due to the something defined in skia_common.gypi
> shouldn't it be added to the include list in that file instead of all the
> targets that use it?

Hmmm... it was kind of a requirement before, but this one got away without it.
However, seeing as how all of these need '..' and 'config', that should probably
move to common. There are actually many more things that should move to common,
but I'm trying to keep this change a bit smaller. Done.

https://codereview.chromium.org/19477005/diff/19001/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/19477005/diff/19001/skia/skia.gyp#newcode17
skia/skia.gyp:17: 'skia_common.gypi',
Needed to make skia_common first in order to have it's includes come first.

https://codereview.chromium.org/19477005/diff/19001/skia/skia_chrome.gypi
File skia/skia_chrome.gypi (left):

https://codereview.chromium.org/19477005/diff/19001/skia/skia_chrome.gypi#old...
skia/skia_chrome.gypi:51: 'ext/SkThread_chrome.cc',
Note that we're already not using this file, as it is excluded on all platforms
below.

https://codereview.chromium.org/19477005/diff/19001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/19477005/diff/19001/skia/skia_library_opts.gy...
skia/skia_library_opts.gyp:144: 'config',
'config' still needs to be listed here in this case since includes are expanded
into the containing dict and the contents appended on list merges. We need to
ensure that 'config' comes before ../third_party/skia/include/config to get the
right SkUserConfig.h. I've talked to reed about this, and he's ok with a future
change to have a (mostly required) compile define like SK_USER_CONFIG_H so that
the include order is less important. With that change, we can remove the
'config' includes in this file as well.

[djsollen, 2013-09-27 18:17:10]

https://codereview.chromium.org/19477005/diff/19001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/19477005/diff/19001/skia/skia_common.gypi#new...
skia/skia_common.gypi:68: '<@(skia_threading_defines)',
should this be in an all_dependent_settings block so that if chrome references
those values nothing breaks?

[bungeman-skia, 2013-09-27 18:39:43]

https://codereview.chromium.org/19477005/diff/19001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/19477005/diff/19001/skia/skia_common.gypi#new...
skia/skia_common.gypi:68: '<@(skia_threading_defines)',
On 2013/09/27 18:17:10, djsollen wrote:
> should this be in an all_dependent_settings block so that if chrome references
> those values nothing breaks?

Good question with a complex answer. So, as a policy thing, if a target has
sources which include Skia headers, it should be directly depending on Skia. If
it has (public) headers which include Skia headers, then it needs to
'export_dependent_settings'. So I would argue that the answer to this is 'no'

Note that in skia_library.gyp the defines in the skia_export_defines variable
are also in a 'direct_dependent_settings' block. If we change this we would also
need to change that, since we certainly don't want to be able to include headers
and have a different set of defines.

[bungeman-skia, 2013-09-27 19:35:00]

On 2013/09/27 18:39:43, bungeman1 wrote:
> https://codereview.chromium.org/19477005/diff/19001/skia/skia_common.gypi
> File skia/skia_common.gypi (right):
> 
>
https://codereview.chromium.org/19477005/diff/19001/skia/skia_common.gypi#new...
> skia/skia_common.gypi:68: '<@(skia_threading_defines)',
> On 2013/09/27 18:17:10, djsollen wrote:
> > should this be in an all_dependent_settings block so that if chrome
references
> > those values nothing breaks?
> 
> Good question with a complex answer. So, as a policy thing, if a target has
> sources which include Skia headers, it should be directly depending on Skia.
If
> it has (public) headers which include Skia headers, then it needs to
> 'export_dependent_settings'. So I would argue that the answer to this is 'no'
> 
> Note that in skia_library.gyp the defines in the skia_export_defines variable
> are also in a 'direct_dependent_settings' block. If we change this we would
also
> need to change that, since we certainly don't want to be able to include
headers
> and have a different set of defines.

For example see
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/172883...
(one of the mac_rel failures above). The users are already wrong in that they're
including Skia headers without having the Skia defines. This change is just
makeing these things show up.

[bungeman-chromium, 2013-09-27 22:23:46]

Opened https://codereview.chromium.org/19477005/ for the
'export_dependent_settings' issue.

The 'android_aosp' bot does not apply patches the same way the other bots do, so
I can't figure out how to get it to run with the entire change.

[bungeman-chromium, 2013-10-09 20:45:05]

On 2013/09/27 22:23:46, bungeman2 wrote:
> Opened https://codereview.chromium.org/19477005/ for the
> 'export_dependent_settings' issue.
> 
> The 'android_aosp' bot does not apply patches the same way the other bots do,
so
> I can't figure out how to get it to run with the entire change.


Correction: the 'export_dependent_settings' issue is
https://codereview.chromium.org/25015005/ .

[bungeman-chromium, 2013-10-09 21:35:06]

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library.gypi
File skia/skia_library.gypi (left):

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library.gypi#ol...
skia/skia_library.gypi:98: '../third_party/skia/src/ports/SkThread_pthread.cpp',
Note that aside from these two lines (and the mention of SkThread_pthread.cpp
below), all of this change can go in now, with these three lines being part of
the roll after https://codereview.chromium.org/19808007/ .

[djsollen, 2013-10-11 15:05:40]

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library_opts.gy...
skia/skia_library_opts.gyp:184: 'config',
is config still needed here?

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library_opts.gy...
skia/skia_library_opts.gyp:211: 'config',
same

[bungeman-skia, 2013-10-11 15:14:30]

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library_opts.gy...
skia/skia_library_opts.gyp:184: 'config',
On 2013/10/11 15:05:40, djsollen wrote:
> is config still needed here?

Yes, it is unfortunately. The way this works in gyp is that when dictionaries
are merged, the list entries in the 'nested' dictionary are appended to the list
in the 'parent' dictionary. Since this 'parent' dictionary has an include_dirs
list, the one in skia_common.gypi will be appended to the one here, resulting in
'config' coming after '../third_party/skia/include/config' (since the latter is
listed in this 'parent' dictionary).

[djsollen, 2013-10-11 15:24:08]

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/19477005/diff/48001/skia/skia_library.gypi#ne...
skia/skia_library.gypi:150: '../third_party/skia/include/config',
why do we ever want to include this?

[bungeman-chromium, 2013-10-18 20:49:10]

I've brought this down (at patch set 9) to what should be done to clean things
up before any related Skia changes. The most important part of this change is
that Skia's SkUserConfig.h is no longer on the include path. The secondary
changes are that the opts targets now include the common gypi (which will, in
the future, need to contain more of the common defines) and the removal of the
unused SkThread_chrome.cc (I can find no platform which uses it, and even the
bsd port patches add themselves to the list of ports which don't use it).

[djsollen, 2013-10-19 20:56:08]

lgtm, but you'll need a chrome committer to sign off as well.

[bungeman-chromium, 2013-10-21 20:47:41]

Committed patchset #9 manually as r229905 (presubmit successful).