688333 - [Skia] Import skia source files and get them building on mac

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We should import Skia into our source tree and get it building so we can use it as an Azure backend.

Not sure who I should be getting to review this, and what else I need to do regarding license issues etc.

The cairo makefile change shouldn't be in here, but we fail to link on mac without (and fail to link everywhere else with it). For some reason this patch is stopping the cairo deflate code from being able to link with zlib. It's looking for the original symbols, and we only export prefixed ones (like _MOZ_Z_deflateInit) from our copy of zlib. I really have no idea how this patch could affect this :(

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Build system changes for Skia

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Skia import (bz2)

This was too big to attach to the bugzilla as a patch file. This patch just imports all the files we need into gfx/skia.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

So it seems unlikely that we'll be able to import Skia without patching it. I'd like the update process to be better than we have with cairo. The third-party libraries we have in media/ are good example of the way to do this. i.e the README_MOZILLA and update.sh scripts

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Skia Import v2

Added update.sh/README_MOZILLA file for easier updating of skia source code

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Build system changes for Skia v2

Added android build support

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Created attachment 562337 [details] [diff] [review] [diff] [details] [review]
> Build system changes for Skia v2
> 
> Added android build support

It seems like the shell script could do better than a long list of files?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> > Created attachment 562337 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Build system changes for Skia v2
> > 
> > Added android build support
> 
> It seems like the shell script could do better than a long list of files?

Also, I wouldn't bother uploading the actual skia parts of the patch because I won't be reviewing them.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> 
> It seems like the shell script could do better than a long list of files?

This is what the media/ script files were doing, what would you suggest instead?

Copy the whole folders?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> > 
> > It seems like the shell script could do better than a long list of files?
> 
> This is what the media/ script files were doing, what would you suggest
> instead?

I think the number of files in the media scripts is a lot more manageable and they likely need to copy some files and leave others.
 
> Copy the whole folders?

Doing that and using globs would do the job.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Skia update.sh script

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Import of Skia files

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Skia Makefile

Not sure if you want to review this Jeff, seems like a good start though.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 567641 [details] [diff] [review]
Skia Makefile

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

Looks fine, a few nits.

::: gfx/Makefile.in
@@ +55,5 @@
> +endif
> +
> +ifeq (android,$(MOZ_WIDGET_TOOLKIT))
> +DIRS        += skia
> +endif

Collapse these two conditions to

ifeq (,$(filter-out cocoa android,$(MOZ_WIDGET_TOOLKIT)))

::: gfx/skia/Makefile.in
@@ +48,5 @@
> +EXPORT_LIBRARY  = 1
> +
> +EXPORTS_NAMESPACES = skia
> +
> +DEFINES += -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0

Should these really all be hardcoded here?

@@ +52,5 @@
> +DEFINES += -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0
> +
> +LOCAL_INCLUDES += -I$(srcdir)/include/core -I$(srcdir)/include/config -I$(srcdir)/include/ports -I$(srcdir)/src/core -I$(srcdir)/include/images -I$(srcdir)/include/utils/mac -I$(srcdir)/include/views -I$(srcdir)/include/effects
> +
> +VPATH += $(srcdir)/src/core $(srcdir)/src/ports $(srcdir)/src/opts $(srcdir)/src/effects

Use multiple lines for these please.  e.g.

FOO = \
  BAR \
  BAZ \
  $(NULL)

@@ +55,5 @@
> +
> +VPATH += $(srcdir)/src/core $(srcdir)/src/ports $(srcdir)/src/opts $(srcdir)/src/effects
> +
> +EXPORTS_skia = \
> +	include/core/Sk64.h \

If you're VPATHing all of this above, you shouldn't need to say include/core on all of these, right?

@@ +60,5 @@
> +	include/core/SkAutoKern.h \
> +	include/core/SkBitmap.h \
> +	include/core/SkBlitRow.h \
> +	include/core/SkBlitter.h \
> +    include/core/SkBounder.h \

Consistent indentation please.

::: toolkit/library/libxul-config.mk
@@ +330,5 @@
>  	$(MOZ_APP_EXTRA_LIBS) \
>  	$(SQLITE_LIBS) \
>  	$(NULL)
>  
> +

Don't add this extra line.

@@ +365,5 @@
> +endif
> +
> +ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> +EXTRA_DSO_LDOPTS += $(MOZ_SKIA_LIBS)
> +endif

Same about the collapsed conditional.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Thanks Kyle!

Gerv: Is there anything I need to do wrt licenses etc before landing this?

Comment 15

[Gervase Markham [:gerv]]

Skia is under the Apache License 2.0, according to its Wikipedia page. Is that correct?

We will need to update about:license to account for this. We don't have any other Apache-licensed code yet, but recently when we made the decision to move to MPL 2, that made it possible to take Apache-2-licensed code.

Once you have done the import, can you file a mozilla.org/Licensing bug on me to update about:licence?

Gerv

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to Gervase Markham [:gerv] from comment #15)
> Skia is under the Apache License 2.0, according to its Wikipedia page. Is
> that correct?

Nope. It has recently been changed to a BSD style license. See:
http://code.google.com/p/skia/source/browse/trunk/LICENSE

Comment 17

[Gervase Markham [:gerv]]

OK :-) Still, we need to update about:licence. If you could produce a patch for me to review (it should be obvious what to do - just make sure they are in alphabetical order) that would be awesome. 

Gerv

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: License Changes

Comment 19

[Gervase Markham [:gerv]]

Comment on attachment 569899 [details] [diff] [review]
License Changes

Looks great, thanks :-)

Gerv

Comment 20

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/656a231abf23

Comment 21

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99297e426f81
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebed635187a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2fce6b656e1

Comment 22

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/656a231abf23
https://hg.mozilla.org/mozilla-central/rev/99297e426f81
https://hg.mozilla.org/mozilla-central/rev/ebed635187a2
https://hg.mozilla.org/mozilla-central/rev/d2fce6b656e1

Comment 23

[[e-mail]]

The bug summary says mac although my Linux gcc 4.3.2 build breaks because of trailing commas in various header files (core/SkAdvancedTypefaceMetrics.h, core/SkCanvas.h, core/SkDevice.h, core/SkFlattenable.h, core/SkMaskFilter.h, core/SkPaint.h, core/SkTypes.h, effects/SkLayerDrawLooper.h)...