Bug 54522 - Fix platform/image-encoders/JPEGImageEncoder.cpp empty_output_buffer() behaviour
Summary: Fix platform/image-encoders/JPEGImageEncoder.cpp empty_output_buffer() behaviour
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Alice Boxhall
URL:
Keywords:
Depends on:
Blocks: 54915
  Show dependency treegraph
 
Reported: 2011-02-15 19:34 PST by Alice Boxhall
Modified: 2011-03-07 19:16 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.67 KB, patch)
2011-02-15 20:06 PST, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (1.82 KB, patch)
2011-03-07 15:16 PST, Alice Boxhall
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Boxhall 2011-02-15 19:34:43 PST
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.
Comment 1 Alice Boxhall 2011-02-15 20:06:26 PST
Created attachment 82580 [details]
Patch
Comment 2 Adam Barth 2011-02-21 12:51:55 PST
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.
Comment 3 Alice Boxhall 2011-02-21 14:40:55 PST
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.
Comment 4 Adam Barth 2011-02-21 15:03:43 PST
If it's not used by anything, then we should delete it, right?
Comment 5 Alice Boxhall 2011-02-21 15:08:34 PST
Ok, I'll prepare a changelist which deletes it.
Comment 6 Peter Kasting 2011-02-21 15:43:34 PST
(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.
Comment 7 Alice Boxhall 2011-02-21 15:59:37 PST
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.
Comment 8 Alice Boxhall 2011-02-21 16:26:58 PST
Created https://bugs.webkit.org/show_bug.cgi?id=54915 with a patch to remove this code.
Comment 9 Peter Kasting 2011-02-21 16:51:14 PST
Closing this, then.
Comment 10 Ojan Vafai 2011-02-21 21:38:11 PST
Comment on attachment 82580 [details]
Patch

r- to remove from the review queue.
Comment 11 Adam Barth 2011-03-01 00:38:21 PST
Re-opening because we're not ready to remove this code yet.
Comment 12 Adam Barth 2011-03-01 00:43:44 PST
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.
Comment 13 Alice Boxhall 2011-03-07 15:16:07 PST
Created attachment 84987 [details]
Patch
Comment 14 Alice Boxhall 2011-03-07 15:20:42 PST
(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.
Comment 15 WebKit Commit Bot 2011-03-07 18:19:38 PST
Comment on attachment 84987 [details]
Patch

Clearing flags on attachment: 84987

Committed r80521: <http://trac.webkit.org/changeset/80521>
Comment 16 WebKit Commit Bot 2011-03-07 18:19:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2011-03-07 19:16:16 PST
http://trac.webkit.org/changeset/80521 might have broken GTK Linux 32-bit Release