Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: wlitwinczyk, Assigned: gw280)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

1.82 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.24 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.69 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
963 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.44 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
983 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
46.77 KB, patch
snorp
: review+
Details | Diff | Splinter Review
2.09 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.46 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.12 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Posted patch skia_update_patch (obsolete) — Splinter Review
Updated skia to newer version. Hopefully it's not too messed up.
Attachment #8430178 - Flags: review?(gwright)
Depends on: 1023732
As we discussed on IRC:

Please split up this patch into separate patches:

- One that simply updates the Skia code to the latest SVN revision, and the associated moz.build changes (this doesn't really need a review, r=upstream should be fine)
- One that changes any moz2d code to cater for Skia API changes etc
- Separate patches that deal with any further changes necessitated by the Skia update. They should be split into as many patches as makes sense.

Take a look at bug 985217 for an example of what I mean.

Of course, this update will be slightly more involved because we're also planning to split out local files into gfx/skia/local and gfx/skia/trunk will be a full checkout of upstream.
Attachment #8430178 - Flags: review?(gwright)
Please address bug 1023732 before further updating skia.
Summary: [Skia] Update to revision 293a4b367ae5b89384c364737ef76099fd3f0101 → [Skia] Update to 2014-07
Woo, after about a dozen attempts I finally managed to push a new try build:

https://tbpl.mozilla.org/?tree=Try&rev=06269b546389
Attachment #8430178 - Attachment is obsolete: true
Assignee: wlitwinczyk → gwright
Depends on: 1043745
This is Skia from 2014-07-28 for what it's worth. They don't use SVN anymore so we don't have meaningful revision numbers.
Attachment #8469017 - Flags: review?(matt.woodrow) → review+
Attachment #8469018 - Flags: review?(matt.woodrow) → review+
Attachment #8469019 - Flags: review?(matt.woodrow) → review+
Attachment #8469020 - Flags: review?(matt.woodrow) → review+
Attachment #8469021 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8469022 [details] [diff] [review]
0007-Bug-1017113-Add-some-legacy-defines-to-SkUserConfig..patch

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

::: gfx/skia/trunk/include/config/SkUserConfig.h
@@ +206,5 @@
>  #define SK_ALLOW_STATIC_GLOBAL_INITIALIZERS 0
> +
> +#define SK_SUPPORT_LEGACY_GETDEVICE
> +#define SK_SUPPORT_LEGACY_GETTOPDEVICE
> +#define SK_SUPPORT_LEGACY_BITMAP_CONFIG

Can we get rid of this with bug 1043745?
Attachment #8469022 - Flags: review?(matt.woodrow) → review+
Attachment #8469024 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Comment on attachment 8469022 [details] [diff] [review]
> 0007-Bug-1017113-Add-some-legacy-defines-to-SkUserConfig..patch
> 
> Review of attachment 8469022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/trunk/include/config/SkUserConfig.h
> @@ +206,5 @@
> >  #define SK_ALLOW_STATIC_GLOBAL_INITIALIZERS 0
> > +
> > +#define SK_SUPPORT_LEGACY_GETDEVICE
> > +#define SK_SUPPORT_LEGACY_GETTOPDEVICE
> > +#define SK_SUPPORT_LEGACY_BITMAP_CONFIG
> 
> Can we get rid of this with bug 1043745?

I think it's necessary but not sufficient.
Comment on attachment 8469025 [details] [diff] [review]
0014-Bug-1017113-Add-RefPtrSkia-to-replace-SkRefPtr-funct.patch

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

Fine code wise, but get someone to confirm this is ok license-wise.
Attachment #8469025 - Flags: review?(matt.woodrow) → review+
Attachment #8469026 - Flags: review?(matt.woodrow) → review+
Attachment #8469023 - Flags: review?(snorp) → review+
I'm sure every changeset before 883cd6be06d2 broke the build even more than bug 1043745 did.

883cd6be06d2 itself also breaks the build because moz.build doesn't match the new sources. That's fixed in the subsequent 6ffb58cad19a, but still...

In fact, none of the changesets from that push except the last one build.

883cd6be06d2 also touches CLOBBER, but there's no reason it needed to.

Bonus fact, the changeset summaries contain useless [PATCH] headers, probably coming from git-format-patch. Please use -k.
And two .orig files, as well as a .rej file were added:
  gfx/skia/trunk/src/pathops/SkLineParameters.h.orig
  gfx/skia/trunk/src/opts/SkBlitRow_opts_arm.cpp.orig
  gfx/skia/trunk/src/pathops/SkLineParameters.h.rej
Depends on: 1053652
Depends on: 1061241
Depends on: 1149318
You need to log in before you can comment on or make changes to this bug.