Bug 58181 - [chromium] Image layers don't update when their contents change
Summary: [chromium] Image layers don't update when their contents change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-08 18:21 PDT by Vangelis Kokkevis
Modified: 2011-04-11 23:04 PDT (History)
3 users (show)

See Also:


Attachments
Patch (17.75 KB, patch)
2011-04-08 18:35 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (23.24 KB, patch)
2011-04-11 09:42 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2011-04-08 18:21:27 PDT
When a new Image is passed to an ImageLayerChromium, the contents of the layer aren't properly invalidated and the new image doesn't register.  The same problem exists if the image contains an animation (e.g. an animated gif).
Comment 1 Vangelis Kokkevis 2011-04-08 18:35:22 PDT
Created attachment 88911 [details]
Patch
Comment 2 James Robinson 2011-04-08 19:00:19 PDT
Comment on attachment 88911 [details]
Patch

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

Code change looks good, test not quite

- the test should go in LayoutTests/compositing/, there's nothing chromium-specific about it
- since the test is set up to show red if it fails and green if it passes there's no need for text in the actual output.  remove it and put it in an HTML comment in the test in case anyone is unsure
- the setTimeout()s seem very wrong - can you get the same result by using layoutTestController.display() to force composites?

> LayoutTests/platform/chromium/compositing/image-content-change.html:29
> +                }, 20);
> +            }
> +        }, 2000);

these timeouts are suspicious.  where do 20 and 2000 come from?  why does this test have to take 2 seconds to complete?
Comment 3 Vangelis Kokkevis 2011-04-11 09:42:38 PDT
Created attachment 89017 [details]
Patch
Comment 4 Vangelis Kokkevis 2011-04-11 09:47:00 PDT
(In reply to comment #2)
> (From update of attachment 88911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88911&action=review
> 
> Code change looks good, test not quite
> 
> - the test should go in LayoutTests/compositing/, there's nothing chromium-specific about it

Good point. Moved it to LayoutTests/compositing/images

> - since the test is set up to show red if it fails and green if it passes there's no need for text in the actual output.  remove it and put it in an HTML comment in the test in case anyone is unsure

Done.

> - the setTimeout()s seem very wrong - can you get the same result by using layoutTestController.display() to force composites?
> 
> > LayoutTests/platform/chromium/compositing/image-content-change.html:29
> > +                }, 20);
> > +            }
> > +        }, 2000);
> 
> these timeouts are suspicious.  where do 20 and 2000 come from?  why does this test have to take 2 seconds to complete?

You're right, the intervals were kind of arbitrary. I changed them to 0.  The timeouts are needed to make sure that the compositor gets triggered before and after the change to make sure we're testing the right thing.  Using zero-interval timeouts seems to be common practice in the rest of the layout tests.  Also, using timeouts rather than layoutTestController specific functionality has the advantage that the test can be run directly in the browser.
Comment 5 James Robinson 2011-04-11 17:04:29 PDT
Comment on attachment 89017 [details]
Patch

R=me
Comment 6 WebKit Commit Bot 2011-04-11 23:04:13 PDT
Comment on attachment 89017 [details]
Patch

Clearing flags on attachment: 89017

Committed r83558: <http://trac.webkit.org/changeset/83558>
Comment 7 WebKit Commit Bot 2011-04-11 23:04:19 PDT
All reviewed patches have been landed.  Closing bug.