Closed Bug 338407 Opened 15 years ago Closed 15 years ago

move image encoder support to 1.8

Categories

(Core :: ImageLib, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch img encoders patch for 1.8 (obsolete) — Splinter Review
The patch for image encoders for 1.8.
Attachment #222483 - Flags: review?(pavlov)
We've stated that we aren't changing _any_ core interfaces for FF2.  What about imgIEncoder?
Did we ship imgIEncoder in 1.8.0.x?  If so, no changes to it on 1.8 branch, yeah.
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.
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?
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.
So, was imgIEncoder shipped in FF1.5 (or 1.5.0.x)?
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.
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.
> 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.
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).
Attachment #222483 - Attachment is obsolete: true
Attachment #222635 - Flags: review?(pavlov)
Attachment #222483 - Flags: review?(pavlov)
Attachment #222635 - Flags: review?(pavlov) → review+
Attachment #222635 - Flags: approval-branch-1.8.1+
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
I accidentally put back these ifdefs in the patch for 338477; they need to not be there.
Attachment #223802 - Flags: review+
Attachment #223802 - Flags: approval-branch-1.8.1+
You need to log in before you can comment on or make changes to this bug.