Check the condition for animation policy of bitmapImage on notifyFinished.

Check the condition for animation policy of BitmapImage on notifyFinished.

Animation Policy was checked every time when BitmapImage is updated and
it caused performance degrade.
This patch makes that animation policy is updated when RenderImage
gets notifyFinished and removes the part to check animation policy
at BitmapImage on every drawing.

BUG=452256, 445921
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189487

[je_julie(Not used), 2015-01-31 14:10:04]

je_julie.kim@samsung.com changed reviewers:
+ fs@opera.com, haraken@chromium.org, mkwst@chromium.org, tkent@chromium.org

[je_julie(Not used), 2015-01-31 14:26:07]

Hi reviewers,

There is an performance issue caused by http://crrev.com/754813002.
The reason is that BitmapImage checks the condition of animation policy every
time when it is updated.
So, this patch makes that animation policy is updated when RenderImage gets
notifyFinished and removes the part to check animation policy at BitmapImage on
every drawing.

The issue was reported on http://crbug.com/452256.
PTAL.

[haraken, 2015-02-01 00:37:40]

LGTM

[Mike West, 2015-02-01 08:00:15]

Source/platform LGTM.

[je_julie(Not used), 2015-02-02 09:48:52]

On 2015/02/01 08:00:15, Mike West wrote:
> Source/platform LGTM.

I updated bug number because I got report from another issue as well.

@fs, @tkent
Could you take a look at this patch?
I need additional approval from rendering OWNERS.

Thanks,

[fs, 2015-02-02 10:31:26]

https://codereview.chromium.org/892783003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/892783003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderImage.cpp:214: image->updateImageAnimationPolicy();
This doesn't appear to cover all the cases compared to previously - the old way,
all RenderObject ImageResourceClients would "weigh in" to the outcome, but now
only RenderImage will (I'd expect at least RenderSVGImage to weigh in too, but
there could be other non-RenderImage-derived cases I'm missing...)

Also, if image != newImage, wouldn't you want this to be newImage? (I see the
issue with the type here.)

Additionally, if transforming "pull" to "push" it seems the value could be
looked up and passed directly, and hence avoid looping at all?

[je_julie(Not used), 2015-02-02 10:57:32]

On 2015/02/02 10:31:26, fs wrote:
> Additionally, if transforming "pull" to "push" it seems the value could be
> looked up and passed directly, and hence avoid looping at all?

@fs,
Thanks for the comment.
I have a question about "pull" to "push".
I don't get it clearly. Sorry but could you describe it in more detail?

[fs, 2015-02-02 11:50:36]

On 2015/02/02 10:57:32, je_julie wrote:
> On 2015/02/02 10:31:26, fs wrote:
> > Additionally, if transforming "pull" to "push" it seems the value could be
> > looked up and passed directly, and hence avoid looping at all?
> 
> @fs,
> Thanks for the comment.
> I have a question about "pull" to "push".
> I don't get it clearly. Sorry but could you describe it in more detail?

What I meant by that was that previously the flow was basically:

BitmapImage -> ImageObserver (ImageResource) -> ImageResourceClient

while now it is:

BitmapImage <- ImageResource <- ImageResourceClient

i.e previously the value was "pulled" from the ImageResourceClient, while now
it's "pushed" (i.e. moving/propagated the other direction) - and then pulled.

I hope that explains what I meant by that.

[je_julie(Not used), 2015-02-02 14:31:33]

On 2015/02/02 11:50:36, fs wrote:
> What I meant by that was that previously the flow was basically:
> 
> BitmapImage -> ImageObserver (ImageResource) -> ImageResourceClient
> 
> while now it is:
> 
> BitmapImage <- ImageResource <- ImageResourceClient
> 
> i.e previously the value was "pulled" from the ImageResourceClient, while now
> it's "pushed" (i.e. moving/propagated the other direction) - and then pulled.
> 
> I hope that explains what I meant by that.

@fs,
Thanks for your kind explanation as always.

The point from this patch is that updating policy is just once after loading
done and BitmapImage can keep and use the policy.
This feature refers to FF's 'image.animation_mode' and the policy is only
updated after loading done, not dynamically on drawing.
For example, users can change animation policy on some menu. For now I'm planing
to create a kind of extensions. After changing animation policy, user can reload
or open a new page and the images from the page can work according to that
animation policy.
So, I thought it's enough to check it after loading, not every drawing.

From the patchset#2, I moved updating policy to ImageLoader because there are
other types images except RenderImage as you pointed and ImageLoader can cover
all kinds of images and can get notification for loading finished.
PTAL.

@Mike,
I updated ImageLoader. Could you take another look at it?

[fs, 2015-02-02 14:48:42]

On 2015/02/02 14:31:33, je_julie wrote:
> On 2015/02/02 11:50:36, fs wrote:
> > What I meant by that was that previously the flow was basically:
> > 
> > BitmapImage -> ImageObserver (ImageResource) -> ImageResourceClient
> > 
> > while now it is:
> > 
> > BitmapImage <- ImageResource <- ImageResourceClient
> > 
> > i.e previously the value was "pulled" from the ImageResourceClient, while
now
> > it's "pushed" (i.e. moving/propagated the other direction) - and then
pulled.
> > 
> > I hope that explains what I meant by that.
> 
> @fs,
> Thanks for your kind explanation as always.
> 
> The point from this patch is that updating policy is just once after loading
> done and BitmapImage can keep and use the policy.
> This feature refers to FF's 'image.animation_mode' and the policy is only
> updated after loading done, not dynamically on drawing.
> For example, users can change animation policy on some menu. For now I'm
planing
> to create a kind of extensions. After changing animation policy, user can
reload
> or open a new page and the images from the page can work according to that
> animation policy.
> So, I thought it's enough to check it after loading, not every drawing.

Yes, that sounds fine.
 
> From the patchset#2, I moved updating policy to ImageLoader because there are
> other types images except RenderImage as you pointed and ImageLoader can cover
> all kinds of images and can get notification for loading finished.

That ought to cover all the cases I could think of.

[je_julie(Not used), 2015-02-03 01:29:46]

On 2015/02/02 14:48:42, fs wrote:
> > From the patchset#2, I moved updating policy to ImageLoader because there
are
> > other types images except RenderImage as you pointed and ImageLoader can
cover
> > all kinds of images and can get notification for loading finished.
> 
> That ought to cover all the cases I could think of.
Yes. I've checked that every element which has images has ImageLoader.
Thanks for your review.

@Mike,
Could you take a look at the patchset#2?
Thanks,

[Mike West, 2015-02-04 08:40:18]

Still LGTM, thanks.

[je_julie(Not used), 2015-02-04 08:46:01]

The CQ bit was checked by je_julie.kim@samsung.com

[commit-bot: I haz the power, 2015-02-04 08:46:46]

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

[commit-bot: I haz the power, 2015-02-04 11:35:50]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189487