Created attachment 82349 [details] Patch fixing the problem, needs png-1.5.1. png-1.5 hides structure members from public view. For this reason, some code doesn't compile any more. The attached patch, mostly from John Bowler <jbowler@acm.org> fixes the problem. Some discussion on the solution: https://sourceforge.net/mailarchive/forum.php?thread_name=000a01cbb6d5%241c18d5b0%24544a8110%24%40acm.org&forum_name=png-mng-implement Since webkit-gtk used internal data that wasn't available via the public interface, the interface has been extended. The patch already makes use of it, which requires png-1.5.1 though. I'm not sure how you want to handle that.
One more thing -- the patch is against webkit-gtk-1.2.7.
View in context: https://bugs.webkit.org/attachment.cgi?id=82349&action=review Welcome to the WebKit bugzilla and thx for the patch. Every patch requries a ChangeLog and should be based on the trunk version, so it can apply on the tree. For more information about contributing to WebKit see http://www.webkit.org/coding/contributing.html. FYI: If you set the review flag to "?" your patch gets better attention and the EWS (the bubbles beside the attachment) fill process it. > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:67 > +#if (PNG_LIBPNG_VER < 10500) > longjmp(JMPBUF(png), 1); > +#else > + png_longjmp(png, 1); > +#endif You use this many times in you code. IMHO sth like the following on top of the file would be better: #if (PNG_LIBPNG_VER >= 10500) static void png_longjmp(png_structp png, int value) { longjmp(JMPBUF(png), value) } #endif > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:292 > - m_reader->setReadOffset(m_reader->currentBufferSize() - png->buffer_size); > - png->buffer_size = 0; > + m_reader->setReadOffset(m_reader->currentBufferSize() - png_process_data_pause(png, 0/*do not save the data*/)); png_process_data_pause doesn't exist in the "old" libpng. > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:320 > - if (m_reader->pngPtr()->interlaced) > + if (png_get_interlace_type(m_reader->pngPtr(), m_reader->infoPtr()) > + != PNG_INTERLACE_NONE) > m_reader->createInterlaceBuffer((m_reader->hasAlpha() ? 4 : 3) * size().width() * size().height()); Please write the if in only one line.
Created attachment 83967 [details] Patch implementing review comments Thanks for your comments, Patrick. I've addressed all the points you raised, but I couldn't test it since my glib2 isn't new enough. Please review.
Attachment 83967 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:51: png_longjmp is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Re comment #4: The function is called png_longjmp in png-1.5, and the patch adds a replacement for it when compiling against older png versions, per Patrick's suggestion. Should I really handle this differently?
Attachment 83967 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8024767
Created attachment 83968 [details] Patch addressing review problems. Addresses compilation problem and style issue.
Attachment 83968 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8070043
Attachment 83968 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8031984
Created attachment 83969 [details] Patch fixing bug in previous.
Comment on attachment 83969 [details] Patch fixing bug in previous. View in context: https://bugs.webkit.org/attachment.cgi?id=83969&action=review Why does http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp#L134 compile? What's the exact compile error? I'm not sure it the longjmp part is really required. > Source/WebCore/ChangeLog:6 > + Fix compilation with png-1.5, and use new API to terminate data > + processing. Please include the bug number. If you have a look at the other changelogs the first line is usually the bug title, followed by the bug# and then a detailed description. Please also include more information about what's the new api and why it doesn't compile any more (some mebers moved from public to private). You can pass the bug# via "--bug" to preparechangelog. > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:50 > +static void pnglongjmp(png_structp png, int value) Please don't use png as prefix. This function isn't part of libpng. Maybe you can use the first approach with png_longjmp and ignore the style warning, or use a function name without a png prefix. Adding a inline keyword would be good too. > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:56 > +#if defined(PNG_LIBPNG_VER_MAJOR) && defined(PNG_LIBPNG_VER_MINOR) && (PNG_LIBPNG_VER_MAJOR > 1 || (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 5)) > + png_longjmp(png, value); > +#else > + longjmp(JMPBUF(png), value); > +#endif Can we merge this with the #define of JMPBUF ? > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:332 > + m_reader->setReadOffset(m_reader->currentBufferSize() - png_process_data_pause(png, 0/*do not save the data*/)); Please don't use this "inline" comment. If you think the comment is required, write it in the line above as a one line comment (//) as a full sentence.
Created attachment 83972 [details] Patch implementing review comments Thanks for the feedback, Patrick. I've taken out the longjmp changes; they are not necessary. I was following the new png style guide for this kind of code. I've also implemented all your other suggestions. New patch attached. (What does the color purple in the bubbles mean, like for "mac" for the previous patch?)
Comment on attachment 83972 [details] Patch implementing review comments (In reply to comment #12) > Thanks for the feedback, Patrick. Thanks for the patch! > What does the color purple in the bubbles mean, like for "mac" for the previous patch? You don't have commit rights for the SVN repository (no committer) and the mac bots cannot process patches from non-committers. You can click on them for more info. That's ok! Also setting cq?. So a reviewer will (usually) set r+ _and_ cq+ on the patch and the commit-queue will commit it without further work.
*** Bug 51041 has been marked as a duplicate of this bug. ***
Comment on attachment 83972 [details] Patch implementing review comments View in context: https://bugs.webkit.org/attachment.cgi?id=83972&action=review Just a minor fix up needed and then this can go in. > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:323 > + /* '0' argument to png_process_data_pause means: do not save the data */ Use C++ style comments and whole sentences. // '0' argument to png_process_data_pause means: Do not save the data. I was confused by what "saving the data" referred to. From reading http://libpng.git.sourceforge.net/git/gitweb.cgi?p=libpng/libpng;a=blobdiff;f=libpng-manual.txt;h=3caff8f9b7019891fd3d0b08ce134ea7e99140e2;hp=b34ffcc1343407687d6723f1132e69f0cdc34941;hb=0a5c9c02faf9c6ceb7acc40e6219e04194581287;hpb=155ce4021812da5629aaebcc9484094944e7d7ae, it means that png should not cache unprocessed bytes. Perhaps something like this instead? m_reader->setReadOffset(m_reader->currentBufferSize() - png_process_data_pause(png, 0 /* Don't cache unprocessed data. */ ));
Just adding myself to the cc line, so I'll see any new patch uploads.
Created attachment 87118 [details] Patch implementing review comments by David Levin David, thanks for your feedback. I've changed the comment as suggested. I didn't make it an inline comment again, since Patrick requested previously to avoid them.
Comment on attachment 87118 [details] Patch implementing review comments by David Levin Clearing flags on attachment: 87118 Committed r82198: <http://trac.webkit.org/changeset/82198>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/82198 might have broken Chromium Win Release
This got rolled out. Due to Chromium Windows build break: 5>..\platform\image-decoders\png\PNGImageDecoder.cpp(244) : error C3861: 'wk_png_get_image_width': identifier not found 4>ppb_pdf_impl.cc 5>..\platform\image-decoders\png\PNGImageDecoder.cpp(245) : error C3861: 'wk_png_get_image_height': identifier not found 4>ppb_font_impl.cc 4>ppb_video_decoder_impl.cc 5>..\platform\image-decoders\png\PNGImageDecoder.cpp(351) : error C3861: 'wk_png_get_interlace_type': identifier not found which could be seen here: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/11702/steps/compile-webkit/logs/stdio
We should reland this after http://codereview.chromium.org/6708113 lands and we change WebKit/chromium/DEPS to point to that revision of late.
The referenced chromium patch has landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=79710 What next?
Let me roll deps and land this in about 7 hours which should be off peak time for commits.
Comment on attachment 87118 [details] Patch implementing review comments by David Levin Since DEPS have been rolled, thanks to Tony. I'll r+, cq+ this. Thanks for your patience.
Comment on attachment 87118 [details] Patch implementing review comments by David Levin Rejecting attachment 87118 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1 Last 500 characters of output: autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error [Errno 60] Operation timed out> Full output: http://queues.webkit.org/results/8285495
Comment on attachment 87118 [details] Patch implementing review comments by David Levin Clearing flags on attachment: 87118 Committed r82344: <http://trac.webkit.org/changeset/82344>
FYI, I needed this too to avoid conversion error: #ifdef PNG_iCCP_SUPPORTED char* profileName; int compressionType; - char* profile; + png_byte* profile; png_uint_32 profileLength; if (png_get_iCCP(png, info, &profileName, &compressionType, &profile, &profileLength)) { ColorProfile colorProfile;
http://trac.webkit.org/changeset/82344 might have broken GTK Linux 32-bit Release The following tests are not passing: css1/basic/comments.html css1/basic/containment.html css1/basic/id_as_selector.html css1/basic/inheritance.html css1/box_properties/border.html css1/box_properties/border_bottom.html css1/box_properties/border_bottom_width.html css1/box_properties/border_left.html css1/box_properties/border_left_width.html css1/box_properties/border_right_inline.html css1/box_properties/border_right_width.html css1/box_properties/border_style.html css1/box_properties/border_top.html css1/box_properties/border_top_width.html css1/box_properties/border_width.html css1/box_properties/clear.html css1/box_properties/clear_float.html css1/box_properties/float_elements_in_series.html css1/box_properties/float_margin.html css1/box_properties/float_on_text_elements.html css1/box_properties/height.html css1/box_properties/margin.html css1/box_properties/margin_bottom.html css1/box_properties/margin_inline.html css1/box_properties/margin_left.html css1/box_properties/margin_right.html css1/box_properties/margin_top.html css1/box_properties/padding.html css1/box_properties/padding_bottom.html css1/box_properties/padding_inline.html css1/box_properties/padding_left.html css1/box_properties/padding_right.html css1/box_properties/padding_top.html css1/box_properties/width.html css1/cascade/cascade_order.html css1/classification/display.html css1/classification/list_style_type.html css1/classification/white_space.html css1/color_and_background/background.html css1/color_and_background/background_attachment.html css1/color_and_background/background_position.html css1/color_and_background/background_repeat.html css1/conformance/forward_compatible_parsing.html css1/font_properties/font.html css1/font_properties/font_family.html css1/font_properties/font_size.html css1/font_properties/font_weight.html css1/formatting_model/floating_elements.html css1/formatting_model/height_of_lines.html css1/formatting_model/horizontal_formatting.html css1/formatting_model/inline_elements.html css1/formatting_model/replaced_elements.html css1/formatting_model/vertical_formatting.html css1/pseudo/anchor.html css1/pseudo/firstletter.html css1/pseudo/firstline.html css1/pseudo/multiple_pseudo_elements.html css1/text_properties/letter_spacing.html css1/text_properties/line_height.html css1/text_properties/text_decoration.html css1/text_properties/text_indent.html css1/text_properties/text_transform.html css1/text_properties/vertical_align.html css1/text_properties/word_spacing.html css1/units/color_units.html css1/units/length_units.html css2.1/t0803-c5502-mrgn-r-02-c.html css2.1/t0803-c5505-mrgn-02-c.html css2.1/t080301-c411-vt-mrgn-00-b.html css2.1/t0905-c5525-fltclr-00-c-ag.html css2.1/t0905-c5525-fltmrgn-00-c-ag.html css2.1/t0905-c5525-fltwidth-00-c-g.html css2.1/t0905-c5526-fltclr-00-c-ag.html css2.1/t1002-c5523-width-02-b-g.html css2.1/t1202-counters-08-b.html css2.1/t1202-counters-09-b.html css2.1/t140201-c535-bg-fixd-00-b-g.html css2.1/t140201-c537-bgfxps-00-c-ag.html css2.1/t1508-c527-font-07-b.html editing/deleting/delete-after-span-ws-001.html editing/deleting/delete-after-span-ws-002.html editing/deleting/delete-after-span-ws-003.html editing/deleting/delete-line-end-ws-001.html editing/deleting/delete-line-end-ws-002.html editing/inserting/insert-div-023.html editing/selection/focus_editable_html.html editing/selection/select-all-001.html editing/selection/select-all-002.html editing/selection/select-all-003.html editing/selection/select-all-004.html editing/selection/unrendered-001.html editing/selection/unrendered-002.html editing/selection/unrendered-003.html editing/selection/unrendered-004.html editing/selection/unrendered-005.html fast/text/international/complex-character-based-fallback.html fast/text/international/thai-line-breaks.html fonts/cursive.html fonts/default.html fonts/fantasy.html fonts/monospace.html fonts/sans-serif.html fonts/serif.html http/tests/misc/link-rel-icon-beforeload.html media/video-controls-rendering.html media/video-zoom.html tables/mozilla/bugs/bug101674.html tables/mozilla/bugs/bug10269-2.html tables/mozilla/bugs/bug10296-1.html tables/mozilla/bugs/bug1055-1.html tables/mozilla/bugs/bug113235-1.html tables/mozilla/bugs/bug113235-3.html tables/mozilla/bugs/bug11944.html tables/mozilla/bugs/bug120364.html tables/mozilla/bugs/bug12384.html tables/mozilla/bugs/bug1302.html tables/mozilla/bugs/bug131020.html tables/mozilla/bugs/bug131020_iframe.html tables/mozilla/bugs/bug137388-2.html tables/mozilla/bugs/bug194024.html tables/mozilla/bugs/bug22019.html tables/mozilla/bugs/bug23151.html tables/mozilla/bugs/bug2479-1.html tables/mozilla/bugs/bug2479-3.html tables/mozilla/bugs/bug2479-4.html tables/mozilla/bugs/bug27038-2.html tables/mozilla/bugs/bug29314.html tables/mozilla/bugs/bug2947.html tables/mozilla/bugs/bug32205-2.html tables/mozilla/bugs/bug38916.html tables/mozilla/bugs/bug3977.html tables/mozilla/bugs/bug43039.html tables/mozilla/bugs/bug43854-1.html tables/mozilla/bugs/bug44505.html tables/mozilla/bugs/bug46480-1.html tables/mozilla/bugs/bug46480-2.html tables/mozilla/bugs/bug50695-1.html tables/mozilla/bugs/bug56405.html tables/mozilla/bugs/bug5797.html tables/mozilla/bugs/bug5835.html tables/mozilla/bugs/bug625.html tables/mozilla/bugs/bug650.html tables/mozilla/bugs/bug67915-1.html tables/mozilla/bugs/bug7112-1.html tables/mozilla/bugs/bug7112-2.html tables/mozilla/bugs/bug73321.html tables/mozilla/bugs/bug92143.html tables/mozilla/bugs/bug96334.html tables/mozilla/bugs/bug96343.html tables/mozilla/collapsing_borders/bug41262-3.html tables/mozilla/core/bloomberg.html tables/mozilla/core/captions.html tables/mozilla/core/cell_heights.html tables/mozilla/core/col_span.html tables/mozilla/core/col_widths_auto_fix.html tables/mozilla/core/col_widths_fix_fixPer.html tables/mozilla/core/nested1.html tables/mozilla/core/one_row.html tables/mozilla/core/row_span.html tables/mozilla/marvin/backgr_index.html tables/mozilla/marvin/backgr_layers-opacity.html tables/mozilla/marvin/backgr_position-table.html tables/mozilla/marvin/backgr_simple-table-cell.html tables/mozilla/marvin/backgr_simple-table-column-group.html tables/mozilla/marvin/backgr_simple-table-column.html tables/mozilla/marvin/backgr_simple-table-row-group.html tables/mozilla/marvin/backgr_simple-table-row.html tables/mozilla/marvin/backgr_simple-table.html tables/mozilla/marvin/x_table_bgcolor_name.xml tables/mozilla/marvin/x_table_bgcolor_rgb.xml tables/mozilla/marvin/x_td_bgcolor_name.xml tables/mozilla/marvin/x_td_bgcolor_rgb.xml tables/mozilla/marvin/x_td_height.xml tables/mozilla/marvin/x_td_nowrap.xml tables/mozilla/marvin/x_th_bgcolor_name.xml tables/mozilla/marvin/x_th_bgcolor_rgb.xml tables/mozilla/marvin/x_th_height.xml tables/mozilla/marvin/x_th_nowrap.xml tables/mozilla/marvin/x_tr_bgcolor_name.xml tables/mozilla/marvin/x_tr_bgcolor_rgb.xml tables/mozilla/other/cell_widths.html tables/mozilla/other/nestedTables.html tables/mozilla/other/test3.html tables/mozilla/other/test6.html tables/mozilla/other/wa_table_thtd_rowspan.html tables/mozilla/other/wa_table_tr_align.html tables/mozilla_expected_failures/bugs/bug10140.html tables/mozilla_expected_failures/bugs/bug101759.html tables/mozilla_expected_failures/bugs/bug10216.html tables/mozilla_expected_failures/bugs/bug1055-2.html tables/mozilla_expected_failures/bugs/bug106966.html tables/mozilla_expected_failures/bugs/bug131020-3.html tables/mozilla_expected_failures/bugs/bug14007-1.html tables/mozilla_expected_failures/bugs/bug14007-2.html tables/mozilla_expected_failures/bugs/bug19526.html tables/mozilla_expected_failures/bugs/bug220653.html tables/mozilla_expected_failures/bugs/bug22122.html tables/mozilla_expected_failures/bugs/bug2479-5.html tables/mozilla_expected_failures/bugs/bug32205-1.html tables/mozilla_expected_failures/bugs/bug67915-2.html tables/mozilla_expected_failures/bugs/bug7243.html tables/mozilla_expected_failures/bugs/bug80762-2.html tables/mozilla_expected_failures/bugs/bug85016.html tables/mozilla_expected_failures/bugs/bug89315.html tables/mozilla_expected_failures/bugs/bug91057.html tables/mozilla_expected_failures/core/backgrounds.html tables/mozilla_expected_failures/core/captions1.html tables/mozilla_expected_failures/core/captions2.html tables/mozilla_expected_failures/core/captions3.html tables/mozilla_expected_failures/core/col_span2.html tables/mozilla_expected_failures/core/columns.html tables/mozilla_expected_failures/core/conflicts.html tables/mozilla_expected_failures/core/standards1.html tables/mozilla_expected_failures/marvin/backgr_border-table-cell.html tables/mozilla_expected_failures/marvin/backgr_border-table-column-group.html tables/mozilla_expected_failures/marvin/backgr_border-table-column.html tables/mozilla_expected_failures/marvin/backgr_border-table-quirks.html tables/mozilla_expected_failures/marvin/backgr_border-table-row-group.html tables/mozilla_expected_failures/marvin/backgr_border-table-row.html tables/mozilla_expected_failures/marvin/backgr_border-table.html tables/mozilla_expected_failures/marvin/backgr_fixed-bg.html tables/mozilla_expected_failures/marvin/backgr_layers-hide.html tables/mozilla_expected_failures/marvin/backgr_layers-show.html tables/mozilla_expected_failures/marvin/backgr_position-table-cell.html tables/mozilla_expected_failures/marvin/backgr_position-table-column-group.html tables/mozilla_expected_failures/marvin/backgr_position-table-column.html tables/mozilla_expected_failures/marvin/backgr_position-table-row-group.html tables/mozilla_expected_failures/marvin/backgr_position-table-row.html tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_row.html tables/mozilla_expected_failures/marvin/table_overflow_style_reflow_row_sibling.html tables/mozilla_expected_failures/marvin/table_overflow_style_reflow_tbody_sibling.html tables/mozilla_expected_failures/other/empty_cells.html tables/mozilla_expected_failures/other/test4.html transforms/2d/hindi-rotated.html transforms/2d/transform-fixed-container.html
@Nezmer: usually this is only a warning. Unconditionally changing the type as you suggest will just shift the warning to users of png<1.5; you need to change it depending on libpng>=1.5 if you want to get rid of it.