Summary: | Move the BitmapImage platform dependent code to a new file named NativeImage[CG|Cairo].cpp | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, dbates, dino, sam, simon.fraser, thorton | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 155322, 155444 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2016-06-12 20:22:28 PDT
Created attachment 281150 [details]
Patch
Created attachment 281155 [details]
Patch
Created attachment 281156 [details]
Patch
Created attachment 281157 [details]
Patch
Created attachment 281158 [details]
Patch
Created attachment 281160 [details]
Patch
Created attachment 288280 [details]
Patch
Created attachment 288283 [details]
Patch
Created attachment 288291 [details]
Patch
Comment on attachment 288291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288291&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:402 > +NativeImagePtr BitmapImage::nativeImageOfSize(const IntSize& size) > +{ > + size_t count = frameCount(); > + > + for (size_t i = 0; i < count; ++i) { > + auto image = frameImageAtIndex(i); > + if (image && sizeOfImage(image) == size) > + return image; > + } > + > + // Fallback to the first frame image if we can't find the right size > + return frameImageAtIndex(0); > +} This was only used for CG for icons. I don't think you should generalize it for all platforms. > Source/WebCore/platform/graphics/BitmapImage.cpp:420 > +Vector<NativeImagePtr> BitmapImage::framesNativeImages() > +{ > + Vector<NativeImagePtr> images; > + size_t count = frameCount(); > + > + for (size_t i = 0; i < count; ++i) { > + if (auto image = frameImageAtIndex(i)) > + images.append(image); > + } > + > + return images; > +} Ditto. > Source/WebCore/platform/graphics/BitmapImage.cpp:475 > + if (!haveFrameImageAtIndex(0) && m_source.frameSizeAtIndex(0, 0) != IntSize(1, 1)) { Isn't that second 0 a subsampling level? > Source/WebCore/platform/graphics/BitmapImage.cpp:481 > + if (!ensureFrameIsCached(0)) What does this 0 mean? Would be nicer if the function name had "at index" in it. > Source/WebCore/platform/graphics/BitmapImage.cpp:552 > + if (!m_cachedImage) { Blank line above this please. > Source/WebCore/platform/graphics/BitmapImage.cpp:594 > + m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation); This is odd. Normally we store Timers by value. > Source/WebCore/platform/graphics/BitmapImage.h:62 > - , m_hasAlpha(true) > + , m_hasAlpha(false) Why? m_hasAlpha(false) is the "safe" value (if we get it wrong, we risk drawing garbage pixels). > Source/WebCore/platform/graphics/BitmapImage.h:72 > // Returns whether there was cached image data to clear. Comment is no longer accurate. > Source/WebCore/platform/graphics/BitmapImage.h:80 > + m_subsamplingLevel = 0; Don't we have enums for this? > Source/WebCore/platform/graphics/NativeImage.h:59 > +IntSize sizeOfImage(const NativeImagePtr&); > +bool hasAlphaOfImage(const NativeImagePtr&); > +Color singlePixelSolidColorOfImage(const NativeImagePtr&); Why the "OfImage"? Old names seem fine. > Source/WebCore/platform/graphics/NativeImage.h:64 > +void drawImage(const NativeImagePtr&, GraphicsContext&, const FloatRect&, const FloatRect&, const IntSize&, CompositeOperator, BlendMode, const ImageOrientation&); > +void clearImageSubImages(const NativeImagePtr&); Maybe drawNativeImage, clearNativeImageSubImages > Source/WebCore/platform/graphics/cg/NativeImageCG.cpp:52 > + // FIXME: Answer correctly the question: does the CGImageRef have alpha channnel? ImageDiff implements this as: CGImageAlphaInfo info = CGImageGetAlphaInfo(image); return (info >= kCGImageAlphaPremultipliedLast) && (info <= kCGImageAlphaFirst); Created attachment 288336 [details]
Patch
Created attachment 288361 [details]
Patch
Comment on attachment 288361 [details] Patch Clearing flags on attachment: 288361 Committed r205682: <http://trac.webkit.org/changeset/205682> All reviewed patches have been landed. Closing bug. Comment on attachment 288291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288291&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:402 >> +} > > This was only used for CG for icons. I don't think you should generalize it for all platforms. Done. nativeImageOfSize() and framesNativeImages() are now declared and implemented within if USE(CG) ... #endif. >> Source/WebCore/platform/graphics/BitmapImage.cpp:475 >> + if (!haveFrameImageAtIndex(0) && m_source.frameSizeAtIndex(0, 0) != IntSize(1, 1)) { > > Isn't that second 0 a subsampling level? Done. the SubsamplingLevel enum is defined in ImageSource.h >> Source/WebCore/platform/graphics/BitmapImage.cpp:481 >> + if (!ensureFrameIsCached(0)) > > What does this 0 mean? Would be nicer if the function name had "at index" in it. Done. This function is now named ensureFrameAtIndexIsCached(). >> Source/WebCore/platform/graphics/BitmapImage.cpp:552 >> + if (!m_cachedImage) { > > Blank line above this please. Done. A new line is added. >> Source/WebCore/platform/graphics/BitmapImage.cpp:594 >> + m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation); > > This is odd. Normally we store Timers by value. This is how it was implemented. I will clean it up in a follow-up patch. >> Source/WebCore/platform/graphics/BitmapImage.h:62 >> + , m_hasAlpha(false) > > Why? m_hasAlpha(false) is the "safe" value (if we get it wrong, we risk drawing garbage pixels). m_hasAlpha can be initialized with false because it is accessed only in BitmapImage::frameHasAlphaAtIndex() and it is guarded by m_haveMetadata. Both m_haveMetadata and m_hasAlpha are set in BitmapImage constructor and in BitmapImage::cacheFrame(). >> Source/WebCore/platform/graphics/BitmapImage.h:72 >> // Returns whether there was cached image data to clear. > > Comment is no longer accurate. The comment was removed. >> Source/WebCore/platform/graphics/BitmapImage.h:80 >> + m_subsamplingLevel = 0; > > Don't we have enums for this? Fixed. >> Source/WebCore/platform/graphics/NativeImage.h:59 >> +Color singlePixelSolidColorOfImage(const NativeImagePtr&); > > Why the "OfImage"? Old names seem fine. Fixed. Names were changed to: nativeImageSize() nativeImageHasAlpha() nativeImageSinglePixelSolidColor() >> Source/WebCore/platform/graphics/NativeImage.h:64 >> +void clearImageSubImages(const NativeImagePtr&); > > Maybe drawNativeImage, clearNativeImageSubImages Fixed. Names were changed as suggested. >> Source/WebCore/platform/graphics/cg/NativeImageCG.cpp:52 >> + // FIXME: Answer correctly the question: does the CGImageRef have alpha channnel? > > ImageDiff implements this as: > CGImageAlphaInfo info = CGImageGetAlphaInfo(image); > return (info >= kCGImageAlphaPremultipliedLast) && (info <= kCGImageAlphaFirst); Forgot to add in the <http://trac.webkit.org/changeset/205682>. I will add it in a follow up patch. |