Closed Bug 1188462 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

21.55 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
411.31 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
20.01 KB, patch
ted
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8639948 - Flags: review?(ted)
Attachment #8639948 - Flags: review?(jmuizelaar)
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.
Attachment #8639954 - Flags: review?(jmuizelaar)
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/'
Attachment #8639954 - Flags: review?(jmuizelaar) → review-
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.
Attachment #8639948 - Flags: review?(jmuizelaar)
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"
Attachment #8640685 - Flags: review?(jmuizelaar)
After the directory rename, various references to the trunk directory just need to be renamed to point to skia instead.
Attachment #8640686 - Flags: review?(jmuizelaar)
Revised the CFLAGS patch as a result of our plan to use "skia/include/foo/SkFoo.h".
Attachment #8639948 - Attachment is obsolete: true
Attachment #8639948 - Flags: review?(ted)
Attachment #8640688 - Flags: review?(ted)
Revised to use the new "skia/include/foo/SkFoo.h" convention.
Attachment #8639954 - Attachment is obsolete: true
Attachment #8640689 - Flags: review?(jmuizelaar)
Fix one more leftover include.
Attachment #8640689 - Attachment is obsolete: true
Attachment #8640689 - Flags: review?(jmuizelaar)
Attachment #8640707 - Flags: review?(jmuizelaar)
Attachment #8640685 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8640686 - Flags: review?(jmuizelaar) → review+
Attachment #8640707 - Flags: review?(jmuizelaar) → review+
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
Attachment #8640688 - Flags: review?(ted)
Just combined the part 1 and part 2 patches as requested but didn't change anything else in them.
Attachment #8640685 - Attachment is obsolete: true
Attachment #8640686 - Attachment is obsolete: true
Attachment #8641153 - Flags: review+
This version exports the SKIA_INCLUDES var instead and uses it to add on to LOCAL_INCLUDES, as requested.
Attachment #8640688 - Attachment is obsolete: true
Attachment #8641155 - Flags: review?(ted)
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!
Attachment #8641155 - Flags: review?(ted) → review+
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48b7d6563ccf

Note to sheriff: these patches require a clobber due to the directory rename.
Keywords: checkin-needed
Blocks: skia-updates
You need to log in before you can comment on or make changes to this bug.