Bug 58181 – [chromium] Image layers don't update when their contents change

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vangelis Kokkevis]

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).

[Vangelis Kokkevis]

Created attachment 88911 [details]
Patch

[James Robinson]

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?

[Vangelis Kokkevis]

Created attachment 89017 [details]
Patch

[Vangelis Kokkevis]

(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.

[James Robinson]

Comment on attachment 89017 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 89017 [details]
Patch

Clearing flags on attachment: 89017

Committed r83558: <http://trac.webkit.org/changeset/83558>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.