Summary: | Handle broken images more correctly in the open-source image decoders | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Kasting <pkasting> | ||||||
Component: | Images | Assignee: | Peter Kasting <pkasting> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | agl, eric, hamaji, levin | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Peter Kasting
2010-01-15 16:14:32 PST
Created attachment 46716 [details]
patch v1
It seems the title of this bug wasn't good. Why did you remove Adam and Eric, when they were the people on bug 32176? (In reply to comment #3) > Why did you remove Adam and Eric, when they were the people on bug 32176? I think you put their email addresses in the subject of this bug. But sorry, I must put them into the CC list when I changed the subject. LGTM. (Note, I am not a WebKit reviewer. You also need a real review.) Comment on attachment 46716 [details]
patch v1
What does this do, and why? How do we test it?
(In reply to comment #6) > (From update of attachment 46716 [details]) > What does this do, and why? Comment 0 explained this in detail, was part of it unclear? > How do we test it? There is already a test in the tree (added as part of bug 32176) that we can handle invalid images without crashing. I could perhaps add a pixel test for an invalid but mostly-decodable BMP (with different expected results for Safari and Chromium) although it would be easier to make a small tweak to my pre-existing Chromium-side image decoder tests to check the buffer output in cases where the image partially decoded. That's about a one-line change. Comment on attachment 46716 [details] patch v1 I really don't know who should review this simple patch. I'm confused, even with your explanation in comment 0 Comment on attachment 46716 [details] patch v1 > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 53348) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,19 @@ > +2010-01-15 Peter Kasting <pkasting@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Handle broken images more correctly in the open-source image decoders. > + https://bugs.webkit.org/show_bug.cgi?id=33747 > + There should be either be tests in this patch or a note about why there are no tests. > + * platform/graphics/ImageSource.cpp: > + (WebCore::ImageSource::createFrameAtIndex): It is nice to add per function level comments when possible. For example, for this function you might say: Removed a condition added to avoid calling size because it is now always ok to call size here. > + * platform/image-decoders/ImageDecoder.h: > + (WebCore::ImageDecoder::size): > + * platform/image-decoders/qt/RGBA32BufferQt.cpp: > + (WebCore::RGBA32Buffer::setSize): > + * platform/image-decoders/skia/ImageDecoderSkia.cpp: > + (WebCore::RGBA32Buffer::setSize): > + Created attachment 47459 [details]
patch v2
I don't think a patch should be r-ed because the reviewer doesn't think the ChangeLog has enough detail (why not r+ and ask me to fix?), but whatever.
(In reply to comment #10) > Created an attachment (id=47459) [details] > patch v2 > > I don't think a patch should be r-ed It was r- for lack of a test. This is pretty typical and is expected of WebKit reviewers. (In reply to comment #11) > (In reply to comment #10) > > Created an attachment (id=47459) [details] [details] > > patch v2 > > > > I don't think a patch should be r-ed > > It was r- for lack of a test. This is pretty typical and is expected of WebKit > reviewers. Oh sorry. I missed that the test issue was discussed in the comments of the bug. Comment on attachment 47459 [details]
patch v2
Very minor nit: It would be nice to wrap the comments in the ChangeLog (one of the few places where WebKit seems to try to maintain ~80 columns) and add periods.
Thanks!
Fixed in 53884. Scrolling through ChangeLog I didn't see any pattern of sticking to 80 columns, but I did add periods on my comments. |