Prevent synchronous layout while painting the last frame of a gif

Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skipping
frames" and it should not cause a synchronous layout because it will
occur during paint. This patch fixes a bug when skipping frames hit
the last frame of a non-looping gif. Before this patch, we would
synchronously notify observers when the last frame was hit which would
end up causing a layout. With this patch, we now defer the
last-frame-needs-paint notification until paint is done.

An enum has been added for the skippingFrames mode which may make it
easier to understand.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798
Committed: https://crrev.com/80ec545afbc7a952f201d6146612dffd4014fa18
Cr-Commit-Position: refs/heads/master@{#413784}

[pdr., 2016-08-19 01:40:11]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 01:40:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2016-08-19 01:48:42]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[pdr., 2016-08-19 01:48:49]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[commit-bot: I haz the power, 2016-08-19 01:49:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2016-08-19 02:37:09]

Description was changed from

==========
Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skippingFrames"
and it should not cause a synchronous layout. This patch fixes a bug
when skipping frames hit the last frame of a non-looping gif. Before
this patch, we would synchronously notify observers when the last frame
was hit which would end up causing a layout. With this patch, we now
defer the last-frame-needs-paint notification until paint is done.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798
==========

to

==========
Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skipping
frames" and it should not cause a synchronous layout because it will
occur durring paint. This patch fixes a bug when skipping frames hit
the last frame of a non-looping gif. Before this patch, we would
synchronously notify observers when the last frame was hit which would
end up causing a layout. With this patch, we now defer the
last-frame-needs-paint notification until paint is done.

An enum has been added for the skippingFrames mode which may make it
easier to understand.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798
==========

[pdr., 2016-08-19 02:37:16]

Description was changed from

==========
Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skipping
frames" and it should not cause a synchronous layout because it will
occur durring paint. This patch fixes a bug when skipping frames hit
the last frame of a non-looping gif. Before this patch, we would
synchronously notify observers when the last frame was hit which would
end up causing a layout. With this patch, we now defer the
last-frame-needs-paint notification until paint is done.

An enum has been added for the skippingFrames mode which may make it
easier to understand.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798
==========

to

==========
Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skipping
frames" and it should not cause a synchronous layout because it will
occur during paint. This patch fixes a bug when skipping frames hit
the last frame of a non-looping gif. Before this patch, we would
synchronously notify observers when the last frame was hit which would
end up causing a layout. With this patch, we now defer the
last-frame-needs-paint notification until paint is done.

An enum has been added for the skippingFrames mode which may make it
easier to understand.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798
==========

[pdr., 2016-08-19 02:37:21]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 02:37:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 04:06:24]

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

[commit-bot: I haz the power, 2016-08-19 04:06:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2016-08-22 18:27:08]

https://codereview.chromium.org/2262493003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2262493003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:625: if (advancement
!= SkipFramesToCatchUp && advancedAnimation)
For ease of review:
This line is the primary change in this patch. Instead of synchronously calling
animationAdvanced here, we defer it.

I found WebKit actually made the same change over a year ago:
https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/Bitmap...,
https://trac.webkit.org/changeset/183732

This change isn't a regression from
https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46, we just put a
bugtrap in that patch and ended up catching something real.

[chrishtr, 2016-08-22 22:55:51]

https://codereview.chromium.org/2262493003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2262493003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:627: 
Put lines 528-529 down here?

[pdr., 2016-08-22 23:32:56]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:33:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 01:29:25]

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

[commit-bot: I haz the power, 2016-08-23 01:29:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pdr., 2016-08-23 03:11:34]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 03:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 06:36:51]

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

[commit-bot: I haz the power, 2016-08-23 06:36:52]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-08-23 17:48:32]

On 2016/08/22 at 22:55:51, chrishtr wrote:
>
https://codereview.chromium.org/2262493003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):
> 
>
https://codereview.chromium.org/2262493003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:627: 
> Put lines 528-529 down here?

Done, and refactored a bit to hopefully be a little more understandable. PTAL

[chrishtr, 2016-08-23 18:01:22]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-08-23 18:01:23]

lgtm

[commit-bot: I haz the power, 2016-08-23 18:02:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 18:08:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-23 18:09:44]

Description was changed from

==========
Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skipping
frames" and it should not cause a synchronous layout because it will
occur during paint. This patch fixes a bug when skipping frames hit
the last frame of a non-looping gif. Before this patch, we would
synchronously notify observers when the last frame was hit which would
end up causing a layout. With this patch, we now defer the
last-frame-needs-paint notification until paint is done.

An enum has been added for the skippingFrames mode which may make it
easier to understand.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798
==========

to

==========
Prevent synchronous layout while painting the last frame of a gif

This patch is a followup to [1] and removes another synchronous layout
in the animated gif painting code.

If an animated gif is behind, BitmapImage::startAnimation will advance
the animation until it catches up. This process is called "skipping
frames" and it should not cause a synchronous layout because it will
occur during paint. This patch fixes a bug when skipping frames hit
the last frame of a non-looping gif. Before this patch, we would
synchronously notify observers when the last frame was hit which would
end up causing a layout. With this patch, we now defer the
last-frame-needs-paint notification until paint is done.

An enum has been added for the skippingFrames mode which may make it
easier to understand.

[1] https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46

BUG=623798

Committed: https://crrev.com/80ec545afbc7a952f201d6146612dffd4014fa18
Cr-Commit-Position: refs/heads/master@{#413784}
==========

[commit-bot: I haz the power, 2016-08-23 18:09:46]

Patchset 5 (id:??) landed as
https://crrev.com/80ec545afbc7a952f201d6146612dffd4014fa18
Cr-Commit-Position: refs/heads/master@{#413784}