Expose setting API to set ImageAnimationPolicy(Allowed, Once, None)

Expose setting API to set ImageAnimationPolicy(Allowed, Once, None).

This patch makes blink support three types(Allowed, Once, None)
for animated image setting and adds two new APIs, ImageObserver::imageAnimationPolicy and 
ImageResourceClient::getImageAnimationPolicy to get animation policy setting.

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

[je_julie(Not used), 2014-11-24 16:37:19]

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

[je_julie(Not used), 2014-11-24 16:38:56]

Hello Mike, tkent and Dominic,

This patch is to support suspending animation and you can see the history from
issue 3690.
My first patchiest checked just suspended or not but I decided to support three
types for animated image setting after I checked Firefox working[1].
Currently Firefox doesn’t support suspending animation with ESC key but it
supports animation type setting with config.
You can also find history from the link[2].

If we also support suspending animation with three options, we can support
config setting like firefox and support suspend/resume with the specific key
with NONE and NORMAL options as well.

I’ve verified with adding test code with none, once and normal types in
render_view_imple.cc.

[1]http://www.ghacks.net/2014/01/23/block-animated-gifs-firefox-playing-automatically/
[2]http://kb.mozillazine.org/Animated_images

Could you take a look?

[dmazzoni, 2014-11-24 17:50:48]

Thanks for this great contribution!

I have a naming suggestion, see below.

My only other concern is if maybe the setting should be cached, but if this
doesn't end up regressing any benchmarks then I think it's fine.

We'll talk about how to enable this setting on the Chromium side in a separate
thread.

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
File Source/core/frame/Settings.h (right):

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
Source/core/frame/Settings.h:36: #include "core/rendering/RenderObject.h"
Is this necessary? Settings.h is included in a lot of files, so it's nice to
avoid including more files. Try to forward-declare, or move the include to the
.cpp file.

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
Source/core/frame/Settings.in:251: animatedImageType type=AnimatedImageType,
initial=AnimatedImageTypeNormal
The name "type" doesn't sound right. How about imageAnimationPolicy with values
ImageAnimationPolicyAllowed, ImageAnimationPolicyAnimateOnce, and
ImageAnimationPolicyNoAnimation.

https://codereview.chromium.org/754813002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/754813002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2803: // We will not render a new
imagePweb
Typo?

https://codereview.chromium.org/754813002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/754813002/diff/20001/Source/platform/graphics...
Source/platform/graphics/BitmapImage.cpp:586: if (!skippingFrames &&
imageObserver()->shouldPauseAnimation(this, m_currentFrame == frameCount()-1))
spaces around the minus sign: frameCount() - 1

https://codereview.chromium.org/754813002/diff/20001/Source/web/WebSettingsIm...
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/754813002/diff/20001/Source/web/WebSettingsIm...
Source/web/WebSettingsImpl.h:104: virtual void
setAnimatedImageType(AnimatedImageType) override;
Please sort this alphabetically.

I know a small handful are not sorted but I think that's a bug.

[Mike West, 2014-11-27 09:08:48]

mkwst@chromium.org changed required reviewers:
+ dmazzoni@chromium.org

[Mike West, 2014-11-27 09:08:49]

public/, web/, and platform/ LGTM. Please don't land this CL until Dominic is
happy with your response to his feedback.

[je_julie(Not used), 2014-11-27 17:22:50]

@Mike, Thanks!

@Dominic,
I updated patch to follow your comment and I also fixed some problems with
advanced display for animation.
Here are the lists I updated from the first patch.
1) Updated enum type name with ImageAnimationPolicy.
2) I created new header file for definition of ImageAnimationPolicy like other
types included in Settings.h. 
For instance, ScriptStreamingMode, EditingBehaviorTypes or EditingBehaviorTypes.
3) Advanced display problem. It is to display animation if it can't meet correct
time interval because of several reasons.
In order to handle it correctly with policy setting, it should be checked before
'internalAdvanceAnimation' in BitmapImage. 
So, I moved policy checking out of 'internalAdvanceAnimation'.
4) There is loop information from animation file format and we can check it with
repetitionCount() in BitmapImage.
It's similar to animation policy 

const int cAnimationLoopOnce = 0;
const int cAnimationLoopInfinite = -1;
const int cAnimationNone = -2;

but it has also real loop count info.
So, I think we can't use it directly for policy but we can refer to that
handling.
I added policy handling like repetitionCount() handling.
5) Getting policy setting info.
There is no any direct connection for setting from BitmapImage and animation
loop is decided in BitmapImage.
So, we need to get policy setting here.
BitmapImage has ImageObserver(ImageResource) and ImageObserver has
ImageResourceClient(RenderObject).
If we use ImageResourceClient, we can get policy setting from page.
So, I added new APIs in ImageObserver and ImageResourceClient.
ImageObserver::imageAnimationPolicy
ImageResourceClient::getImageAnimationPolicy

I have one more comment related to applying policy Setting.
I tried to add policyChange delegate and frameview->invalidate(rect) if policy
is updated 
but I found it's not updated without any change.
Currently there is no invalidate triggering from SettingDelegate.
And I also checked FF with config:about and image.animation_mode setting.
FF should be re-started after changing animation_mode.
If you don't mind, can I handle direct applying with another CL?
Please take a look at the updated patchset.
Thanks,

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
File Source/core/frame/Settings.h (right):

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
Source/core/frame/Settings.h:36: #include "core/rendering/RenderObject.h"
On 2014/11/24 17:50:47, dmazzoni wrote:
> Is this necessary? Settings.h is included in a lot of files, so it's nice to
> avoid including more files. Try to forward-declare, or move the include to the
> .cpp file.

I created new header file for definition of ImageAnimationPolicy like other
types included in Settings.h. For instance, ScriptStreamingMode,
EditingBehaviorTypes or EditingBehaviorTypes.

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/754813002/diff/20001/Source/core/frame/Settin...
Source/core/frame/Settings.in:251: animatedImageType type=AnimatedImageType,
initial=AnimatedImageTypeNormal
On 2014/11/24 17:50:47, dmazzoni wrote:
> The name "type" doesn't sound right. How about imageAnimationPolicy with
values
> ImageAnimationPolicyAllowed, ImageAnimationPolicyAnimateOnce, and
> ImageAnimationPolicyNoAnimation.

Thanks for suggestion. Done.

https://codereview.chromium.org/754813002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/754813002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2803: // We will not render a new
imagePweb
On 2014/11/24 17:50:47, dmazzoni wrote:
> Typo?
Done.

https://codereview.chromium.org/754813002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/754813002/diff/20001/Source/platform/graphics...
Source/platform/graphics/BitmapImage.cpp:586: if (!skippingFrames &&
imageObserver()->shouldPauseAnimation(this, m_currentFrame == frameCount()-1))
On 2014/11/24 17:50:48, dmazzoni wrote:
> spaces around the minus sign: frameCount() - 1
Done.

https://codereview.chromium.org/754813002/diff/20001/Source/web/WebSettingsIm...
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/754813002/diff/20001/Source/web/WebSettingsIm...
Source/web/WebSettingsImpl.h:104: virtual void
setAnimatedImageType(AnimatedImageType) override;
On 2014/11/24 17:50:48, dmazzoni wrote:
> Please sort this alphabetically.
> 
> I know a small handful are not sorted but I think that's a bug.

Yes. you're right. I updated it.

[dmazzoni, 2014-11-29 07:56:33]

lgtm

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
File Source/platform/graphics/ImageAnimationPolicy.h (right):

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:36: // ImageAnimationPolicy is
used for controling image animation
nit: controling -> controlling

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:41: ImageAnimationPolicyAllowed,
I'd write this as:

// Animate the image (the default).

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:44: // Animation is not allowed
and keep still image.
How about this?

// Show the first frame and do not animate.

[tkent, 2014-12-01 01:45:34]

https://codereview.chromium.org/754813002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/754813002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2787: if (document().settings()) {
nit: We prefer early return.

if (!document().settings())
    return false;

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
File Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/BitmapImage.cpp:447: animated &= m_animationPolicy !=
ImageAnimationPolicyNoAnimation;
&= is not a logical operation. Using it for bool values is inappropriate.

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
File Source/platform/graphics/ImageAnimationPolicy.h (right):

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:1: /*
Please use the 3-line header.

http://www.chromium.org/blink/coding-style#TOC-License

[je_julie(Not used), 2014-12-01 05:29:24]

@Dominc and @tkent,

Thanks for review.
I updated code. PTAL.

https://codereview.chromium.org/754813002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/754813002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2787: if (document().settings()) {
On 2014/12/01 01:45:34, tkent wrote:
> nit: We prefer early return.
> 
> if (!document().settings())
>     return false;

Done.

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
File Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/BitmapImage.cpp:447: animated &= m_animationPolicy !=
ImageAnimationPolicyNoAnimation;
On 2014/12/01 01:45:34, tkent wrote:
> &= is not a logical operation. Using it for bool values is inappropriate.

I changed it with && inside if condition.

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
File Source/platform/graphics/ImageAnimationPolicy.h (right):

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:1: /*
On 2014/12/01 01:45:34, tkent wrote:
> Please use the 3-line header.
> 
> http://www.chromium.org/blink/coding-style#TOC-License

Done

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:36: // ImageAnimationPolicy is
used for controling image animation
On 2014/11/29 07:56:33, dmazzoni wrote:
> nit: controling -> controlling

Done

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:41: ImageAnimationPolicyAllowed,
On 2014/11/29 07:56:33, dmazzoni wrote:
> I'd write this as:
> 
> // Animate the image (the default).

Done

https://codereview.chromium.org/754813002/diff/60001/Source/platform/graphics...
Source/platform/graphics/ImageAnimationPolicy.h:44: // Animation is not allowed
and keep still image.
On 2014/11/29 07:56:33, dmazzoni wrote:
> How about this?
> 
> // Show the first frame and do not animate.

Done

[tkent, 2014-12-01 05:34:51]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-12-01 05:34:52]

lgtm

[commit-bot: I haz the power, 2014-12-01 05:35:29]

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

[commit-bot: I haz the power, 2014-12-01 07:43:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-01 07:43:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/39106)

[tkent, 2014-12-01 07:43:47]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-12-01 07:44:20]

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

[commit-bot: I haz the power, 2014-12-01 08:23:10]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186217