Implements getImageAnimationPolicy() at ImageLoader.

Implements getImageAnimationPolicy() at ImageLoader.

AnimationPolicy is generally updated at notifyFinished().
But if it does not have LayoutObject yet at notifyFinished(),
AnimationPolicy couldn't be updated and keeps default value,
ImageAnimationPolicyAllowed because another ImageResourceClient,
ImageLoader, doesn't have implementation for
getImageAnimationPolicy().

This patch implements getImageAnimationPolicy() at ImageLoader
and makes ImageResource have correct animation policy.

BUG=483474
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196068

[je_julie(Not used), 2015-05-01 21:53:15]

je_julie.kim@samsung.com changed reviewers:
+ dmazzoni@chromium.org, mkwst@chromium.org

[je_julie(Not used), 2015-05-01 21:53:16]

PTAL.

[Mike West, 2015-05-01 22:43:23]

mkwst@chromium.org changed reviewers:
+ esprehn@chromium.org

[Mike West, 2015-05-01 22:43:23]

Adding esprehn@, as I think he knows more about animation than I do.

https://codereview.chromium.org/1120963003/diff/1/Source/core/fetch/ImageReso...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1120963003/diff/1/Source/core/fetch/ImageReso...
Source/core/fetch/ImageResource.cpp:443: }
Nit: Can you `ASSERT(new_policy != ImageAnimationPolicyInvalid)` here?

[je_julie(Not used), 2015-05-03 09:13:12]

On 2015/05/01 22:43:23, Mike West (traveling. slow.) wrote:
> Adding esprehn@, as I think he knows more about animation than I do.
> 
>
https://codereview.chromium.org/1120963003/diff/1/Source/core/fetch/ImageReso...
> File Source/core/fetch/ImageResource.cpp (right):
> 
>
https://codereview.chromium.org/1120963003/diff/1/Source/core/fetch/ImageReso...
> Source/core/fetch/ImageResource.cpp:443: }
> Nit: Can you `ASSERT(new_policy != ImageAnimationPolicyInvalid)` here?

@Mike,
Thanks for your review even while travelling.

@esprehn, @Dominic and @Mike,
The problem is that animation policy for image resource could not be updated
correctly at some pages because ImageResource doesn't have LayoutObjects
as a resource client yet. Currently LayoutObject is the only one which has
implementation for getImageAnimationPolicy().

At the first patchset, I thought ImageResource could not update animation policy
even if I added implementation at ImageLoader because I saw debug message
that Document().Settings() at ImageLoader is null on reloading.
But, after more investigation, I realized that animation policy at ImageLoader
is updated correctly. The reason why I saw the debug message that
document().Settings()
is null is that ImageLoader at stopped state of Document is still a Resource
client of that image.

At the second patchset, I tried to add ASSERT as Mike commented but
ImageBitmapTest
from webkit_unit_tests is failed because the Document is created without
Settings
at ImageBitmapTest.
So, I removed ASSERT at the third patchset.

PTAL.

[je_julie(Not used), 2015-05-08 11:33:20]

je_julie.kim@samsung.com changed reviewers:
+ haraken@chromium.org

[je_julie(Not used), 2015-05-08 11:39:01]

@haraken,
Could you take a look at this CL?
Because you reviewed the CL, 892783003, related to animationPolicy previously, I
think you could help me to get a review.

[haraken, 2015-05-08 23:13:52]

On 2015/05/08 11:39:01, je_julie wrote:
> @haraken,
> Could you take a look at this CL?
> Because you reviewed the CL, 892783003, related to animationPolicy previously,
I
> think you could help me to get a review.

Can you add a test case for this?

Also it seems better this CL could be reviewed by animation experts.

[je_julie(Not used), 2015-05-09 04:27:48]

On 2015/05/08 23:13:52, haraken wrote:
> On 2015/05/08 11:39:01, je_julie wrote:
> > @haraken,
> > Could you take a look at this CL?
> > Because you reviewed the CL, 892783003, related to animationPolicy
previously,
> I
> > think you could help me to get a review.
> 
> Can you add a test case for this?
> 
> Also it seems better this CL could be reviewed by animation experts.

@haraken, Thanks for your comment.
Animation Policy is only working with SVG animation and animated gif with
reference to FF.
(BTW, I know there is a discussion about removing SVG SMIL @blink-dev.)
For SVG animation, there are svg-animation-policy-none.html and
svg-animation-policy-once.html.
For animated gif, I'm not sure how to add a layout test.
If you have any idea, please let me know.

@esprehn, could you take a look at this CL?

[esprehn, 2015-05-22 22:01:26]

No test?

[je_julie(Not used), 2015-05-24 14:32:34]

On 2015/05/22 22:01:26, esprehn wrote:
> No test?

I realised that I can try to run pixel test for this CL.

@esprehn,
I added LayoutTest for this CL.
I created animated gif with 2 frames and one loop.
If animation policy with none works correctly, the result from pixel test will
have the screen shot with the first frame because gif is not animated.
If the policy works not correctly, the result from pixel test will have the
screen shot with the second frame.
PTAL.

[esprehn, 2015-05-26 00:27:56]

We can't put a 100ms delay in a test, you need some way to do this without a
forced delay.

[je_julie(Not used), 2015-05-26 23:20:01]

On 2015/05/26 00:27:56, esprehn wrote:
> We can't put a 100ms delay in a test, you need some way to do this without a
> forced delay.

Hi esprehn,
I updated TC without setTimeout. PTAL.
As to Trybot fail, it is not related to this change.

[esprehn, 2015-05-26 23:29:16]

https://codereview.chromium.org/1120963003/diff/80001/LayoutTests/accessibili...
File LayoutTests/accessibility/animation-policy.html (right):

https://codereview.chromium.org/1120963003/diff/80001/LayoutTests/accessibili...
LayoutTests/accessibility/animation-policy.html:19: if (Date.now() - prevTime >
100) {
This is still a 100ms delay, you wan't put a 100ms timeout inside a test.

[esprehn, 2015-05-26 23:30:21]

On 2015/05/26 at 23:29:16, esprehn wrote:
>
https://codereview.chromium.org/1120963003/diff/80001/LayoutTests/accessibili...
> File LayoutTests/accessibility/animation-policy.html (right):
> 
>
https://codereview.chromium.org/1120963003/diff/80001/LayoutTests/accessibili...
> LayoutTests/accessibility/animation-policy.html:19: if (Date.now() - prevTime
> 100) {
> This is still a 100ms delay, you wan't put a 100ms timeout inside a test.

can't rather. Tests must be fast, we have 40k of them. :)

[dmazzoni, 2015-05-26 23:52:30]

I did a quick search and I can't find any existing tests for animated gifs or
animated svg that *don't* use setTimeout.

So, this will be somewhat uncharted territory.

I think you'll want to add an Internals API that advances time, so that when
BitmapImage.cpp calls monotonicallyIncreasingTime(), it gets that time, rather
than the real time.

[je_julie(Not used), 2015-05-27 15:05:38]

On 2015/05/26 23:52:30, dmazzoni wrote:
> I did a quick search and I can't find any existing tests for animated gifs or
> animated svg that *don't* use setTimeout.
> 
> So, this will be somewhat uncharted territory.
> 
> I think you'll want to add an Internals API that advances time, so that when
> BitmapImage.cpp calls monotonicallyIncreasingTime(), it gets that time, rather
> than the real time.

esprehn and Dominic,
Thanks for your kind mention.
I added one Internals API like
[RaisesException] void advanceTimeWithImage(Element image, double advancedTime);
It modifies m_desiredFrameStartTime which is a decision point to move next frame
of animation.
PTAL.

[dmazzoni, 2015-05-27 15:15:30]

https://codereview.chromium.org/1120963003/diff/100001/Source/core/testing/In...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1120963003/diff/100001/Source/core/testing/In...
Source/core/testing/Internals.h:114: // Advances Time to test whether animated
images work properly.
Advances Time -> Advances time

https://codereview.chromium.org/1120963003/diff/100001/Source/core/testing/In...
Source/core/testing/Internals.h:115: void advanceTimeWithImage(Element* image,
double advancedTime, ExceptionState&);
Instead of advancedTime, how about deltaTime to make it clear that this is the
amount of time to be added rather than a time to set it to.

It's always good to be precise, so maybe deltaTimeInSeconds

[je_julie(Not used), 2015-05-27 16:48:38]

I updated API name with deltaTimeInSeconds.
PTAL.

https://codereview.chromium.org/1120963003/diff/100001/Source/core/testing/In...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1120963003/diff/100001/Source/core/testing/In...
Source/core/testing/Internals.h:114: // Advances Time to test whether animated
images work properly.
On 2015/05/27 15:15:30, dmazzoni wrote:
> Advances Time -> Advances time

I updated comment with new API name.

https://codereview.chromium.org/1120963003/diff/100001/Source/core/testing/In...
Source/core/testing/Internals.h:115: void advanceTimeWithImage(Element* image,
double advancedTime, ExceptionState&);
On 2015/05/27 15:15:30, dmazzoni wrote:
> Instead of advancedTime, how about deltaTime to make it clear that this is the
> amount of time to be added rather than a time to set it to.
> 
> It's always good to be precise, so maybe deltaTimeInSeconds

Thanks for suggestion.

[esprehn, 2015-05-28 05:51:14]

Plea fix the naming so its clear it has side effects so either setDeltaSeconds
or updateNextFrameTime? It needs a verb in the name.

Lgtm otherwise.

[dmazzoni, 2015-05-28 06:06:46]

Yeah, sorry if I wasn't clear in my feedback. I wanted deltaTimeInSeconds
to be the name of the variable, not the function. I suggest:

void advanceTimeForImage(Element* image, double deltaTimeInSeconds,
ExceptionState&);

On Wed, May 27, 2015 at 10:51 PM, <esprehn@chromium.org> wrote:

> Plea fix the naming so its clear it has side effects so either
> setDeltaSeconds
> or updateNextFrameTime? It needs a verb in the name.
>
> Lgtm otherwise.
>
> https://codereview.chromium.org/1120963003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[je_julie(Not used), 2015-05-28 11:03:19]

Dominic and esprehn,
Thanks for helping me to solve the problem.
I renamed the API with advanceTimeForImage as Dominic suggested.
WDYT?

On 2015/05/28 06:06:46, dmazzoni wrote:
> Yeah, sorry if I wasn't clear in my feedback. I wanted deltaTimeInSeconds
> to be the name of the variable, not the function. I suggest:
> 
> void advanceTimeForImage(Element* image, double deltaTimeInSeconds,
> ExceptionState&);
Sorry that I misunderstood your mention.
> 
> On Wed, May 27, 2015 at 10:51 PM, <mailto:esprehn@chromium.org> wrote:
> 
> > Plea fix the naming so its clear it has side effects so either
> > setDeltaSeconds
> > or updateNextFrameTime? It needs a verb in the name.
> >
> > Lgtm otherwise.
> >
> > https://codereview.chromium.org/1120963003/

[dmazzoni, 2015-05-28 14:48:37]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-05-28 14:48:38]

lgtm

Looks great now, thanks!

[dmazzoni, 2015-05-28 14:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1120963003/#ps140001
(title: "Update API name.")

[commit-bot: I haz the power, 2015-05-28 14:49:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120963003/140001

[commit-bot: I haz the power, 2015-05-28 15:55:24]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196068