RESOLVED FIXED 96135
[chromium] WebImageSkia shouldn't use ImageSource
https://bugs.webkit.org/show_bug.cgi?id=96135
Summary [chromium] WebImageSkia shouldn't use ImageSource
Hin-Chung Lam
Reported 2012-09-07 11:59:01 PDT
Preparing for deferred image decoding for Chromium port. ImageSource is used to provide deferred image decoder for BitmapImage. However WebImageSkia also uses it Chromium WebKit API. WebImageSkia should use ImageDecoder directly.
Attachments
Patch (10.50 KB, patch)
2012-09-07 15:46 PDT, Hin-Chung Lam
no flags
Patch for landing (10.46 KB, patch)
2012-09-18 13:36 PDT, Hin-Chung Lam
no flags
Patch for landing (10.46 KB, patch)
2012-09-18 13:44 PDT, Hin-Chung Lam
no flags
Patch for landing (10.48 KB, patch)
2012-09-18 13:50 PDT, Hin-Chung Lam
no flags
Patch (10.92 KB, patch)
2012-09-18 17:26 PDT, Hin-Chung Lam
no flags
Patch (10.96 KB, patch)
2012-09-18 17:27 PDT, Hin-Chung Lam
no flags
Patch for landing (10.99 KB, patch)
2012-09-19 11:40 PDT, Hin-Chung Lam
no flags
Hin-Chung Lam
Comment 1 2012-09-07 15:46:32 PDT
Hin-Chung Lam
Comment 2 2012-09-14 18:07:26 PDT
Review ping.
Adam Barth
Comment 3 2012-09-17 11:06:36 PDT
Comment on attachment 162887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162887&action=review > Source/WebKit/chromium/src/WebImageSkia.cpp:55 > + OwnPtr<ImageDecoder> decoder(adoptPtr(ImageDecoder::create(*buffer.get(), ImageSource::AlphaPremultiplied, ImageSource::GammaAndColorProfileApplied))); ImageDecoder::create should call adoptPtr itself and return a PassOwnPtr. You don't need to fix that in this patch, but I just wanted to mention it. (Also, we tend to prefer the assignment form of the constructor for readability. > Source/WebKit/chromium/src/WebImageSkia.cpp:87 > + OwnPtr<NativeImageSkia> image = adoptPtr(frame->asNewNativeImage()); asNewNativeImage should probably also return a PassOwnPtr.
Hin-Chung Lam
Comment 4 2012-09-18 13:29:15 PDT
(In reply to comment #3) > (From update of attachment 162887 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162887&action=review > > > Source/WebKit/chromium/src/WebImageSkia.cpp:55 > > + OwnPtr<ImageDecoder> decoder(adoptPtr(ImageDecoder::create(*buffer.get(), ImageSource::AlphaPremultiplied, ImageSource::GammaAndColorProfileApplied))); > > ImageDecoder::create should call adoptPtr itself and return a PassOwnPtr. You don't need to fix that in this patch, but I just wanted to mention it. (Also, we tend to prefer the assignment form of the constructor for readability. > > > Source/WebKit/chromium/src/WebImageSkia.cpp:87 > > + OwnPtr<NativeImageSkia> image = adoptPtr(frame->asNewNativeImage()); > > asNewNativeImage should probably also return a PassOwnPtr. Those are all platform specific implementations. Particularly ImageDecoder::create() on CG is not open-sourced (I can't find an implementation of it on CG). I'll file a bug to track this.
WebKit Review Bot
Comment 5 2012-09-18 13:31:12 PDT
Comment on attachment 162887 [details] Patch Rejecting attachment 162887 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: it/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/chromium/WebKit.gypi Hunk #1 FAILED at 104. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/WebKit.gypi.rej patching file Source/WebKit/chromium/src/WebImageSkia.cpp patching file Source/WebKit/chromium/tests/WebImageTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13896303
Hin-Chung Lam
Comment 6 2012-09-18 13:36:45 PDT
Created attachment 164613 [details] Patch for landing
WebKit Review Bot
Comment 7 2012-09-18 13:39:22 PDT
Comment on attachment 164613 [details] Patch for landing Rejecting attachment 164613 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: (s). patching file Source/WebKit/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/chromium/WebKit.gypi Hunk #1 FAILED at 101. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/WebKit.gypi.rej patching file Source/WebKit/chromium/src/WebImageSkia.cpp patching file Source/WebKit/chromium/tests/WebImageTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13903245
Hin-Chung Lam
Comment 8 2012-09-18 13:44:20 PDT
Created attachment 164614 [details] Patch for landing
WebKit Review Bot
Comment 9 2012-09-18 13:46:28 PDT
Comment on attachment 164614 [details] Patch for landing Rejecting attachment 164614 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: (s). patching file Source/WebKit/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/chromium/WebKit.gypi Hunk #1 FAILED at 101. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/WebKit.gypi.rej patching file Source/WebKit/chromium/src/WebImageSkia.cpp patching file Source/WebKit/chromium/tests/WebImageTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13900292
Hin-Chung Lam
Comment 10 2012-09-18 13:50:00 PDT
Created attachment 164615 [details] Patch for landing
WebKit Review Bot
Comment 11 2012-09-18 15:29:31 PDT
Comment on attachment 164615 [details] Patch for landing Clearing flags on attachment: 164615 Committed r128939: <http://trac.webkit.org/changeset/128939>
WebKit Review Bot
Comment 12 2012-09-18 15:29:35 PDT
All reviewed patches have been landed. Closing bug.
Hin-Chung Lam
Comment 13 2012-09-18 16:19:50 PDT
Reverted r128939 for reason: Failing test_shell_tests Committed r128942: <http://trac.webkit.org/changeset/128942>
Hin-Chung Lam
Comment 14 2012-09-18 17:26:20 PDT
Hin-Chung Lam
Comment 15 2012-09-18 17:27:50 PDT
Hin-Chung Lam
Comment 16 2012-09-18 17:29:29 PDT
Please have another look. Patch was reverted because of a crash and bug is fixed in latest patch. I also added a test case to catch the bug.
Adam Barth
Comment 17 2012-09-19 11:22:49 PDT
> Particularly ImageDecoder::create() on CG is not open-sourced (I can't find an implementation of it on CG). I'll file a bug to track this. Ports that use CG don't use ImageDecoder::create. Thanks for filing the bugs about changing these functions.
Hin-Chung Lam
Comment 18 2012-09-19 11:40:49 PDT
Created attachment 164759 [details] Patch for landing
WebKit Review Bot
Comment 19 2012-09-19 12:17:24 PDT
Comment on attachment 164759 [details] Patch for landing Clearing flags on attachment: 164759 Committed r129032: <http://trac.webkit.org/changeset/129032>
WebKit Review Bot
Comment 20 2012-09-19 12:17:28 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.