The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla10
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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 :(
(Assignee)

Comment 1

6 years ago
Created attachment 561624 [details] [diff] [review]
Build system changes for Skia
(Assignee)

Comment 2

6 years ago
Created attachment 561625 [details]
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.
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
(Assignee)

Comment 4

6 years ago
Created attachment 562336 [details] [diff] [review]
Skia Import v2

Added update.sh/README_MOZILLA file for easier updating of skia source code
Attachment #561625 - Attachment is obsolete: true
Attachment #562336 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

6 years ago
Created attachment 562337 [details] [diff] [review]
Build system changes for Skia v2

Added android build support
Attachment #561624 - Attachment is obsolete: true
Attachment #562337 - Flags: review?
(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?
(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.
(Assignee)

Comment 8

6 years ago
(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?
(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.
Attachment #562336 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 10

6 years ago
Created attachment 567638 [details] [diff] [review]
Skia update.sh script
Attachment #562336 - Attachment is obsolete: true
Attachment #562337 - Attachment is obsolete: true
Attachment #562337 - Flags: review?
Attachment #567638 - Flags: review?(jmuizelaar)
(Assignee)

Comment 11

6 years ago
Created attachment 567640 [details] [diff] [review]
Import of Skia files
(Assignee)

Comment 12

6 years ago
Created attachment 567641 [details] [diff] [review]
Skia Makefile

Not sure if you want to review this Jeff, seems like a good start though.
Attachment #567641 - Flags: review?
Attachment #567638 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

6 years ago
Attachment #567641 - Flags: review? → review?(khuey)
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.
Attachment #567641 - Flags: review?(khuey) → review+
(Assignee)

Comment 14

6 years ago
Thanks Kyle!

Gerv: Is there anything I need to do wrt licenses etc before landing this?
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
(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
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
(Assignee)

Comment 18

6 years ago
Created attachment 569899 [details] [diff] [review]
License Changes
Attachment #569899 - Flags: review?(gerv)
Comment on attachment 569899 [details] [diff] [review]
License Changes

Looks great, thanks :-)

Gerv
Attachment #569899 - Flags: review?(gerv) → review+
(Assignee)

Comment 20

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/656a231abf23
Assignee: nobody → matt.woodrow
(Assignee)

Comment 21

6 years ago
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
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
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)...
Blocks: 731384
You need to log in before you can comment on or make changes to this bug.