Bug 96135 – [chromium] WebImageSkia shouldn't use ImageSource

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 162887 [details]
Patch

[Hin-Chung Lam]

Review ping.

[Adam Barth]

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]

(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 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]

Created attachment 164613 [details]
Patch for landing

[WebKit Review Bot]

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]

Created attachment 164614 [details]
Patch for landing

[WebKit Review Bot]

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]

Created attachment 164615 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164615 [details]
Patch for landing

Clearing flags on attachment: 164615

Committed r128939: <http://trac.webkit.org/changeset/128939>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Reverted r128939 for reason:

Failing test_shell_tests

Committed r128942: <http://trac.webkit.org/changeset/128942>

[Hin-Chung Lam]

Created attachment 164637 [details]
Patch

[Hin-Chung Lam]

Created attachment 164638 [details]
Patch

[Hin-Chung Lam]

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]

> 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]

Created attachment 164759 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164759 [details]
Patch for landing

Clearing flags on attachment: 164759

Committed r129032: <http://trac.webkit.org/changeset/129032>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.