66597 – [skia] BitmapImageSingleFrameSkia returns true to isBitmapImage() but does not inherit from BitmapImage

RESOLVED FIXED Bug 66597

[skia] BitmapImageSingleFrameSkia returns true to isBitmapImage() but does not inherit from BitmapImage

https://bugs.webkit.org/show_bug.cgi?id=66597

Summary [skia] BitmapImageSingleFrameSkia returns true to isBitmapImage() but does no...

James Robinson

Reported 2011-08-19 14:52:12 PDT

This leads to bugs since code in WebKit typically assumes that if a function returns true to isFoo() it is safe to static_cast<Foo*> and call functions. See https://bugs.webkit.org/show_bug.cgi?id=65063 for an example of where this bug caused real crashes.

Attachments
Patch (11.91 KB, patch) 2012-05-03 14:05 PDT, Tom Hudson	no flags	Details Formatted Diff Diff
Patch (13.52 KB, patch) 2012-05-04 08:48 PDT, Tom Hudson	no flags	Details Formatted Diff Diff
Patch (14.82 KB, patch) 2012-05-04 14:21 PDT, Tom Hudson	no flags	Details Formatted Diff Diff
Patch (18.48 KB, patch) 2012-05-18 13:14 PDT, Tom Hudson	webkit-ews: commit-queue-	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2011-08-19 15:09:26 PDT

I don't like the changes made for bug 65063, so I welcome cleanup here.

James Robinson

Comment 2 2011-08-19 15:25:04 PDT

For the record, I don't either. It was either that or turn off your optimization for all skia ports, though :/.

James Robinson

Comment 3 2011-08-19 17:30:52 PDT

The original motivation for BitmapImageSingleFrameSkia is to avoid having all of the animation overhead for simple 1-frame images, which seems like a useful optimization for all ports. I think the option 3 from https://bugs.webkit.org/show_bug.cgi?id=65063#c18 is the best way to go here - we should turn BitmapImage into a simple class that doesn't handle animation concerns, and use an AnimatedBitmapImage subclass to handle the animation case. This should have the nice benefit of reducing the memory use for non-animated images for all platforms and clean up the class hierarchy a lot for the skia case.

Tom Hudson

Comment 4 2012-05-01 12:56:46 PDT

It feels like a stretch for me, but it looks like the original class of bug may be triggering again in the Chromium port, so I'm going to try to do the refactoring James endorsed this week.

Tom Hudson

Comment 5 2012-05-03 14:05:52 PDT

Created attachment 140089 [details] Patch

Tom Hudson

Comment 6 2012-05-03 14:08:55 PDT

In the discussions around https://bugs.webkit.org/show_bug.cgi?id=65063#c18, one thing we didn't realize is not only does the Skia-specific single-frame class avoid all the implementation for multiple frames, it *also* uses a different type to store the image, so there doesn't seem to be any implementation that can be shared between the two classes. This patch introduces a lightweight class SimpleBitmapImage that does nothing more than mark that an object is some sort of Bitmap (rather than SVG). I've gone through the WebCore fixing up call sites that were using isBitmapImage() to use isSimpleBitmapImage() where that seems reasonable. If we're going to do something more comprehensive, I'll need pointers from people familiar with this part of the code.

Build Bot

Comment 7 2012-05-03 14:24:41 PDT

Comment on attachment 140089 [details] Patch Attachment 140089 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12545388

Philippe Normand

Comment 8 2012-05-03 14:30:07 PDT

Comment on attachment 140089 [details] Patch Attachment 140089 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12607303

Build Bot

Comment 9 2012-05-03 14:36:34 PDT

Comment on attachment 140089 [details] Patch Attachment 140089 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12545391

Early Warning System Bot

Comment 10 2012-05-03 14:42:29 PDT

Comment on attachment 140089 [details] Patch Attachment 140089 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12625094