Bug 54522 – Fix platform/image-encoders/JPEGImageEncoder.cpp empty_output_buffer() behaviour

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Alice Boxhall]

The libjpeg documentation (e.g. http://trac.imagemagick.org/browser/jpeg/trunk/libjpeg.txt) specifies:

[the empty_output_buffer method of a JPEGDestinationManager] "should write out the *entire* buffer (use the saved start address and buffer length; ignore the current state of next_output_byte and free_in_buffer)."

Fix jpegEmptyOutputBuffer() by ignoring free_in_buffer value as required.

[Alice Boxhall]

Created attachment 82580 [details]
Patch

[Adam Barth]

Comment on attachment 82580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82580&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

This line will prevent this patch from landing (because of the OOPS).  Can we test this change?  If not, please explain why we can't.

[Alice Boxhall]

As noted in http://trac.webkit.org/changeset/67606, there is no existing layout test for this file, as it's not actually used by anything yet.

[Adam Barth]

If it's not used by anything, then we should delete it, right?

[Alice Boxhall]

Ok, I'll prepare a changelist which deletes it.

[Peter Kasting]

(In reply to comment #3)
> As noted in http://trac.webkit.org/changeset/67606, there is no existing layout test for this file, as it's not actually used by anything yet.

Does "yet" mean "this is going to be used soon"?  Deleting it doesn't make sense if we're about to re-add it, but if there aren't firm immediate plans, it should go away.

[Alice Boxhall]

The file has been untouched since the initial commit 5 months ago, and still nothing is using it; I think it's unlikely to be used soon.

[Alice Boxhall]

Created https://bugs.webkit.org/show_bug.cgi?id=54915 with a patch to remove this code.

[Peter Kasting]

Closing this, then.

[Ojan Vafai]

Comment on attachment 82580 [details]
Patch

r- to remove from the review queue.

[Adam Barth]

Re-opening because we're not ready to remove this code yet.

[Adam Barth]

Comment on attachment 82580 [details]
Patch

I think this patch is fine, but I'm not sure excited about it.  The problem with fixing dead code is that we can't test the change, which means we're likely to regress.

We'll need to explain why we can't test this change (and remove the "no new tests" OOPS) before landing.

[Alice Boxhall]

Created attachment 84987 [details]
Patch

[Alice Boxhall]

(In reply to comment #12)
> (From update of attachment 82580 [details])
> I think this patch is fine, but I'm not sure excited about it.  The problem with fixing dead code is that we can't test the change, which means we're likely to regress.

Once this code starts to be used (see bug 39230), the canvas.toDataUrl() tests will test it.

> We'll need to explain why we can't test this change (and remove the "no new tests" OOPS) before landing.

Done.

[WebKit Commit Bot]

Comment on attachment 84987 [details]
Patch

Clearing flags on attachment: 84987

Committed r80521: <http://trac.webkit.org/changeset/80521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80521 might have broken GTK Linux 32-bit Release