1188462 - We can't update to latest upstream Skia because we lose Skia's include directory structure while exporting headers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

The latest upstream version of Skia uses relative paths in its include directory for headers. These also include paths that use "../foo" to access other components and "../../src" to access header files living in the source directory.

However, the way we have been importing Skia into the tree is to flatten and export the include structure in moz.build, which both requires more work on our part and is no longer compatible with Skia. To work around this while using the current scheme, we would either need to manually export the entire include structure including headers in the src directory, or by modifying the Skia headers to work around this.

A far better solution is to just add the Skia include directory to the search path so all this just works, and we are more future-proof against Skia updates. This way we don't have to keep updating moz.build or the Skia headers and they should just work from now on.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: part 1 - add SKIA_CFLAGS list for including skia headers into CXXFLAGS

This patch removes the moz.build kluges we do to export the Skia headers and instead adds a SKIA_CFLAGS moz.build config var that can be used to add the tree Skia include directory to CXXFLAGS on an opt-in basis.

This both adds the include directory itself, as well as some subdirs that are required given the pathnames used in the Skia header files themselves.

All Skia headers are prefixed as SkFoo.h or GrFoo.h, so this should not unduly pollute our header namespace and is otherwise quite safe.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: part 2 - fix inclusion of skia headers to use correct directory prefixes

This mostly just fixes the directory prefix of files that include Skia to use the Skia subpaths as the "skia/" prefix will no longer work.

Within Skia itself, the following files had to be modified to work with this change:

core/SkShader.h: "GrColor.h" -> "../gpu/GrColor.h", this header file is used regardless of whether Skia GPU is used, so to avoid adding gpu/ to the global search path unless we are using Skia GPU, this was changed to a relative path. This change is also in the upstream Skia so it's a safe modification on our part.

config/SkUserConfig.h: "skia/SkAtomics_foo.h" -> "../../src/ports/SkAtomics_foo.h", these files actually live in the src dir, which we can now reference, and this file probably did that before we modified it to use "skia/" paths. In any case, the upstream no longer has these living in src/, so we won't need this anymore once we update to latest upstream.

Comment 3

[Lee Salzman [:lsalzman]]

Try run for build changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f82c86448e

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8639954 [details] [diff] [review]
part 2 - fix inclusion of skia headers to use correct directory prefixes

Review of attachment 8639954 [details] [diff] [review]:
-----------------------------------------------------------------

We agreed to switch to 'include/skia/core/'

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8639948 [details] [diff] [review]
part 1 - add SKIA_CFLAGS list for including skia headers into CXXFLAGS

Review of attachment 8639948 [details] [diff] [review]:
-----------------------------------------------------------------

I don't feel qualified to review this.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: part 1 - rename gfx/skia/trunk directory to gfx/skia/skia

This patch renames gfx/skia/trunk to gfx/skia/skia, so that when gfx/skia is added to our include search dirs, then we can use as the following as our Skia include convention:

#include "skia/include/foo/SkFoo.h"

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: part 2 - fix up any references to the gfx/skia/trunk dir to use gfx/skia/skia instead

After the directory rename, various references to the trunk directory just need to be renamed to point to skia instead.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: part 3 - add SKIA_CFLAGS list for including skia headers into CXXFLAGS

Revised the CFLAGS patch as a result of our plan to use "skia/include/foo/SkFoo.h".

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: part 4 - fix inclusion of skia headers to use correct directory prefixes

Revised to use the new "skia/include/foo/SkFoo.h" convention.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: part 4 - fix inclusion of skia headers to use correct directory prefixes

Fix one more leftover include.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8640686 [details] [diff] [review]
part 2 - fix up any references to the gfx/skia/trunk dir to use gfx/skia/skia instead

Review of attachment 8640686 [details] [diff] [review]:
-----------------------------------------------------------------

This should be merged into a single patch with part 1 so we don't break people's builds during bisection.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8640688 [details] [diff] [review]
part 3 - add SKIA_CFLAGS list for including skia headers into CXXFLAGS

Review of attachment 8640688 [details] [diff] [review]:
-----------------------------------------------------------------

Instead of generating CXXFLAGS, could we just make this a list of paths and use LOCAL_INCLUDES in moz.build instead? That seems more semantically useful. I think you'd just have to change this to be like:
SKIA_INCLUDES = "/gfx/skia /gfx/skia/includes/config ..."
AC_SUBST_LIST(SKIA_INCLUDES)

then in moz.build:
LOCAL_INCLUDES += SKIA_INCLUDES

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: parts 1 and 2 - rename gfx/skia/trunk directory to gfx/skia/skia

Just combined the part 1 and part 2 patches as requested but didn't change anything else in them.

Comment 14

[Lee Salzman [:lsalzman]]

Attachment: part 3 - add SKIA_INCLUDES list for adding Skia to header search path

This version exports the SKIA_INCLUDES var instead and uses it to add on to LOCAL_INCLUDES, as requested.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 8641155 [details] [diff] [review]
part 3 - add SKIA_INCLUDES list for adding Skia to header search path

Review of attachment 8641155 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 16

[Lee Salzman [:lsalzman]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48b7d6563ccf

Note to sheriff: these patches require a clobber due to the directory rename.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8802b591ce2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d74c410fed8
https://hg.mozilla.org/integration/mozilla-inbound/rev/370dc3fc7cc3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8802b591ce2
https://hg.mozilla.org/mozilla-central/rev/2d74c410fed8
https://hg.mozilla.org/mozilla-central/rev/370dc3fc7cc3