Bug 48494 - fix setSize() order in WebP image decoding
Summary: fix setSize() order in WebP image decoding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-27 19:17 PDT by Pascal Massimino
Modified: 2010-10-28 01:22 PDT (History)
4 users (show)

See Also:


Attachments
fix setSize() calls in WEBPImageDecoder.cpp (978 bytes, patch)
2010-10-27 19:18 PDT, Pascal Massimino
abarth: review-
Details | Formatted Diff | Diff
fix setSize() calls in WEBPImageDecoder.cpp (1.56 KB, patch)
2010-10-27 21:48 PDT, Pascal Massimino
abarth: review-
Details | Formatted Diff | Diff
fix setSize() calls in WEBPImageDecoder.cpp (1.62 KB, patch)
2010-10-27 22:08 PDT, Pascal Massimino
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
fix setSize() calls in WEBPImageDecoder.cpp (1.76 KB, patch)
2010-10-27 22:24 PDT, Pascal Massimino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pascal Massimino 2010-10-27 19:17:23 PDT
Hi,

in WEBPImageDecoder.cpp, setSize() was only called
when onlySize was true, causing refresh problems.
This patch makes sure setSize() is always called
appropriately.

detective work: pkasting@

Pascal
Comment 1 Pascal Massimino 2010-10-27 19:18:08 PDT
Created attachment 72130 [details]
fix setSize() calls in WEBPImageDecoder.cpp
Comment 2 Adam Barth 2010-10-27 19:29:15 PDT
Comment on attachment 72130 [details]
fix setSize() calls in WEBPImageDecoder.cpp

Looks good, but we need a ChangeLog and (ideally) a test.
Comment 3 Pascal Massimino 2010-10-27 21:48:56 PDT
Created attachment 72139 [details]
fix setSize() calls in WEBPImageDecoder.cpp

added the missing ChangeLog
Comment 4 WebKit Review Bot 2010-10-27 21:51:08 PDT
Attachment 72139 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Adam Barth 2010-10-27 21:52:23 PDT
Comment on attachment 72139 [details]
fix setSize() calls in WEBPImageDecoder.cpp

You need to fill out all those fields in the ChangeLog (except the Reviewed by NOBODY line).  Also, we can't land a patch with tabs.
Comment 6 Pascal Massimino 2010-10-27 21:56:31 PDT
Adam,

oops... added the missing ChangeLog...

(In reply to comment #2)
> (From update of attachment 72130 [details])
> Looks good, but we need a ChangeLog and (ideally) a test.

to exercise the bug (lack of refresh) there's some manual intervention required:
 - load a big picture
 - switch to another tab
 - switch back to the picture tab
 - scroll up/down
=> some parts were blank.

I'm not sure how a test can reproduce that...

Pascal
Comment 7 Pascal Massimino 2010-10-27 22:08:39 PDT
Created attachment 72144 [details]
fix setSize() calls in WEBPImageDecoder.cpp

... and now, without the tabs
Comment 8 Adam Barth 2010-10-27 22:18:49 PDT
Comment on attachment 72144 [details]
fix setSize() calls in WEBPImageDecoder.cpp

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

> WebCore/ChangeLog:3
> +        Reviewed by NOBODY (OOPS!).

This line is ok because the tools fill in the reviewer automatically for you.

> WebCore/ChangeLog:6
> +        fix setSize() call flow: it was only called the first
> +        time (when onlySize is true)

Also, you should link to the bug from the ChangeLog so folks can find it from the SVN revision history.

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

There's an SVN pre-commit hook that rejects OOPS in files.  This line will prevent the patch from being landed.  You should replace this line with an explanation of why there is no test.
Comment 9 Pascal Massimino 2010-10-27 22:24:27 PDT
Created attachment 72145 [details]
fix setSize() calls in WEBPImageDecoder.cpp

fix the changeLog: add bug URL, explain the missing test.
Comment 10 Adam Barth 2010-10-27 22:28:10 PDT
Comment on attachment 72145 [details]
fix setSize() calls in WEBPImageDecoder.cpp

Thanks.  Your TPS reports are now in order!
Comment 11 WebKit Commit Bot 2010-10-28 01:22:07 PDT
Comment on attachment 72145 [details]
fix setSize() calls in WEBPImageDecoder.cpp

Clearing flags on attachment: 72145

Committed r70758: <http://trac.webkit.org/changeset/70758>
Comment 12 WebKit Commit Bot 2010-10-28 01:22:13 PDT
All reviewed patches have been landed.  Closing bug.