338407 - move image encoder support to 1.8

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This patch moves the jpeg and png image encoding support to the 1.8 branch; the majority of this patch is in libpr0n, which hasn't really changed between trunk and 1.8.  This support is required to be able to implement canvas toDataURL, which extension and web app developers are interested in to be able to serialize images (e.g. thumbnails).

Note that this regresses bug 223909 (which affects thunderbird only), which is something that was lifted from aviary to 1.8, and was never fixed on the trunk with either the old jpeg encoder hack or with the new encoding interfaces.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: img encoders patch for 1.8

The patch for image encoders for 1.8.

Comment 2

[Darin Fisher]

We've stated that we aren't changing _any_ core interfaces for FF2.  What about imgIEncoder?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Did we ship imgIEncoder in 1.8.0.x?  If so, no changes to it on 1.8 branch, yeah.

Comment 4

[Stuart Parmenter]

the imgIEncoder on the branch is not the right one.  It has always been a branch-only interface that was done specifically for thunderbird.  It needs to be renamed to imgIClipboardEncoder or somesuch and the callers should be updated.  There are only 1 or 2 callers, so this shouldn't be a problem.

Comment 5

[Darin Fisher]

Stuart, the point of not changing interfaces is because we cannot predict what third-party extensions may be using. (There may be _many_ more than just 1 or 2 callers!)  We've been really surprised before to find out what wacky things extension authors do, so we've been erring on the side of extreme caution.  If that interface is not shipped with FF 1.5, then it can be changed.  If it is shipped with FF 1.5, then it cannot change.  Make sense?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It does make sense, except where it makes a lot more sense to just go ahead and make the change.  I'd say this is one of those cases, but like I said before -- I don't really care what path we take, as long as everyone agrees on a path so that I can implement it.

Comment 7

[Darin Fisher]

So, was imgIEncoder shipped in FF1.5 (or 1.5.0.x)?

Comment 8

[Darin Fisher]

It looks like it was shipped in FF 1.5.0.3 at least, so I think you are best off not changing the interface.  imgIEncoder_MOZILLA_1_8_BRANCH has been the convention for cases like this even though that is really ugly.

Comment 9

[Stuart Parmenter]

I tried to make these changes for Firefox 1.5 but it happened too late and they wouldn't let me land the encoder stuff for it.  You can find the patch where I renamed the interface and updated the callers in bug 345684.

I'm never going to ever take the imgIEncoder that is on the branch on the trunk in any fashion -- it needs to be rebuilt on top of this version/the trunk version.  I'm not going to rename the one on the trunk and having it have a different name on the trunk and branch seems like a bad plan.

The interface currently on the branch is windows-only, and has only ever been on the branch -- usually landed late in the cycles at that.

I think this is a situation where we have to be willing to make the interface change.  Having conflicting names and the on-the-branch-but-not-on-the-trunk makes this a different situation than everywhere else imho.

Comment 10

[Darin Fisher]

> having it have a different name on the trunk and branch 
> seems like a bad plan.

Many interfaces differ between trunk and branch.  What makes this one special?  Non-frozen interfaces can change on the trunk.  The idea with not changing these interfaces on the branch is to retain compatibility with FF 1.5, so extension authors do not have to do much to support FF 2.

Just LXR for MOZILLA_1_8_BRANCH to see all the interfaces that differ between trunk and 1.8 branch.  We've been down this exact same road before.

> The interface currently on the branch is windows-only

It appears to exist in my Linux build of FF 1.5.0.3.  Maybe we ship the XPT file for it, but no implementation of it on Linux?

I think I'm just pointing out the rules that were stated (not by me) for 1.8 branch management.  If you think this is bad in this case, then appeal to shaver (or drivers?) since he's the one managing Gecko-uplift to the 1.8 branch.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: new patch with changed interface name

Ok, here's a new patch; it adds a new imgIEncoder_MOZILLA_1_8_BRANCH interface and implements that, and leaves the old one as-is.

The implementation of the old one is removed, but will return in terms of the new interface (on the branch only; there is only one imgIEncoder on the trunk, and it's the same as imgIEncoder_MOZILLA_1_8_BRANCH).

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: remove ifdefs that snuck back in

I accidentally put back these ifdefs in the patch for 338477; they need to not be there.