[webkit-reviews] review granted: [Bug 169944] If an image appears more than once on a page, decoding for painting one instance repaints them all : [Attachment 314298] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 169944: If an image appears more than once on a page, decoding for painting
one instance repaints them all
https://bugs.webkit.org/show_bug.cgi?id=169944

Attachment 314298: Patch

https://bugs.webkit.org/attachment.cgi?id=314298&action=review




--- Comment #32 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 314298
  --> https://bugs.webkit.org/attachment.cgi?id=314298
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314298&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:553
> +	   if (animatingState == ImageAnimatingState::No &&
m_pendingImageDrawingClients.find(client) ==
m_pendingImageDrawingClients.end())

I think this can use m_pendingImageDrawingClients.find.contains()

> Source/WebCore/loader/cache/CachedImage.h:157
> +    HashSet<CachedImageClient*> m_pendingImageDrawingClients;

Do we remove CachedImageClients from this set in all the code paths where those
clients are destroyed? I.e. are we sure we never leave a dangling pointer in
here?

> Source/WebCore/platform/graphics/Image.cpp:186
> +	   while (result != ImageDrawResult::DidRequestDecoding && toY <
destRect.maxY()) {
>	       currentTileRect.shiftXEdgeTo(destRect.x());
>	       float toX = currentTileRect.x();
> -	       while (toX < destRect.maxX()) {
> +	       while (result != ImageDrawResult::DidRequestDecoding && toX <
destRect.maxX()) {
>		   FloatRect toRect(toX, toY, currentTileRect.width(),
currentTileRect.height());
>		   FloatRect fromRect(toFloatPoint(currentTileRect.location() -
oneTileRect.location()), currentTileRect.size());
>		   fromRect.scale(1 / scale.width(), 1 / scale.height());
>  
> -		   draw(ctxt, toRect, fromRect, op, BlendModeNormal,
decodingMode, ImageOrientationDescription());
> +		   result = draw(ctxt, toRect, fromRect, op, BlendModeNormal,
decodingMode, ImageOrientationDescription());

I think it would be much clearer to just break from these loops if result ==
DidRequestDecoding