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

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

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.

[Simon Fraser (smfr)]

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

[James Robinson]

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

[James Robinson]

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]

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]

Created attachment 140089 [details]
Patch

[Tom Hudson]

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 on attachment 140089 [details]
Patch

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

[Philippe Normand]

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 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 on attachment 140089 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 140089 [details]
Patch

Attachment 140089 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12612385

[Gyuyoung Kim]

Comment on attachment 140089 [details]
Patch

Attachment 140089 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12611334

[Tom Hudson]

Created attachment 140244 [details]
Patch

[Build Bot]

Comment on attachment 140244 [details]
Patch

Attachment 140244 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12543344

[Build Bot]

Comment on attachment 140244 [details]
Patch

Attachment 140244 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12632170

[Early Warning System Bot]

Comment on attachment 140244 [details]
Patch

Attachment 140244 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12633176

[Gyuyoung Kim]

Comment on attachment 140244 [details]
Patch

Attachment 140244 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12631189

[Early Warning System Bot]

Comment on attachment 140244 [details]
Patch

Attachment 140244 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12627184

[Gustavo Noronha (kov)]

Comment on attachment 140244 [details]
Patch

Attachment 140244 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12629213

[Tom Hudson]

Created attachment 140324 [details]
Patch

[Build Bot]

Comment on attachment 140324 [details]
Patch

Attachment 140324 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12631284

[Early Warning System Bot]

Comment on attachment 140324 [details]
Patch

Attachment 140324 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12631286

[Early Warning System Bot]

Comment on attachment 140324 [details]
Patch

Attachment 140324 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12627265

[Build Bot]

Comment on attachment 140324 [details]
Patch

Attachment 140324 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12531283

[Tom Hudson]

Created attachment 142772 [details]
Patch

[Early Warning System Bot]

Comment on attachment 142772 [details]
Patch

Attachment 142772 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12720968

[Build Bot]

Comment on attachment 142772 [details]
Patch

Attachment 142772 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12722888

[Early Warning System Bot]

Comment on attachment 142772 [details]
Patch

Attachment 142772 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12728332

[Build Bot]

Comment on attachment 142772 [details]
Patch

Attachment 142772 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12728344