blimp: Update animation policy for images on the blimp engine.

blimp: Update animation policy for images on the blimp engine.

The Blimp Engine currently uses the default animation policy in blink,
which means for an image like a GIF the engine would trigger regular
commits to drive the animation. Change this to NO_ANIMATION, so only
the first frame of the animation is shown.

BUG=605190
Committed: https://crrev.com/03a74c3c04ff24b43fbeaf5dc0c3e013c364e500
Cr-Commit-Position: refs/heads/master@{#388908}

[Khushal, 2016-04-20 21:55:17]

khushalsagar@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2016-04-20 23:09:07]

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
File blimp/engine/app/engine_settings.cc (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
blimp/engine/app/engine_settings.cc:14: // only the first frame will be shown.
Weird line-wrap?

This comment seems unnecessarily verbose; I'd suggest something like "Disable
animation in images (e.g. GIFs) by default.". Reader should be able to find the
enum declaration and read the comments on its precise meaning themselves.

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
File blimp/engine/app/engine_settings.h (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
blimp/engine/app/engine_settings.h:16: content::ImageAnimationPolicy
animation_policy;
Not specific to this CL, but...

1. This structure needs a comment to explain what "settings" it carries.
2. The members need comments to explain what they actually affect (though "These
parameters mirror those in blink::WebPreferences" or something would be fine.)
3. Order of definitions is odd; ctor/dtor etc normally precede members.
4. Not clear what the default values are?

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
blimp/engine/app/settings_manager.cc:25: DCHECK(prefs);
nit: Also not specific to this CL, but no point DCHECK()ing a pointer
immediately before you de-reference it!

[Wez, 2016-04-20 23:09:30]

LGTM once the comment is tidied up.

[Khushal, 2016-04-21 00:21:25]

Thanks, done. Could you take another look?

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
File blimp/engine/app/engine_settings.cc (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
blimp/engine/app/engine_settings.cc:14: // only the first frame will be shown.
On 2016/04/20 23:09:06, Wez wrote:
> Weird line-wrap?
> 
> This comment seems unnecessarily verbose; I'd suggest something like "Disable
> animation in images (e.g. GIFs) by default.". Reader should be able to find
the
> enum declaration and read the comments on its precise meaning themselves.

Done.

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
File blimp/engine/app/engine_settings.h (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/engine_set...
blimp/engine/app/engine_settings.h:16: content::ImageAnimationPolicy
animation_policy;
On 2016/04/20 23:09:06, Wez wrote:
> Not specific to this CL, but...
> 
> 1. This structure needs a comment to explain what "settings" it carries.
> 2. The members need comments to explain what they actually affect (though
"These
> parameters mirror those in blink::WebPreferences" or something would be fine.)
> 3. Order of definitions is odd; ctor/dtor etc normally precede members.

Done.

> 4. Not clear what the default values are?

Should the default values be marked in the header? I was being consistent with
the pattern in content::WebPreferences where the default values are just
specified in the implementation.

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
blimp/engine/app/settings_manager.cc:25: DCHECK(prefs);
On 2016/04/20 23:09:06, Wez wrote:
> nit: Also not specific to this CL, but no point DCHECK()ing a pointer
> immediately before you de-reference it!

The DCHECK helps since it gives a clear stack trace in case someone passes a
nullptr rather than a segmentation fault.

[Wez, 2016-04-21 00:40:29]

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
blimp/engine/app/settings_manager.cc:25: DCHECK(prefs);
On 2016/04/21 00:21:25, Khushal wrote:
> On 2016/04/20 23:09:06, Wez wrote:
> > nit: Also not specific to this CL, but no point DCHECK()ing a pointer
> > immediately before you de-reference it!
> 
> The DCHECK helps since it gives a clear stack trace in case someone passes a
> nullptr rather than a segmentation fault.

The stack trace will be the same, surely? The only difference will be that the
failing pointer is a small integer, rather than actually zero.

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
File blimp/engine/app/engine_settings.cc (right):

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.cc:14: // for details.
nit: I don't think you need the ImageAnimationPolicy.h mention here; reader can
look up the enum themselves to find the relevant comments.

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
File blimp/engine/app/engine_settings.h (right):

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.h:25: content::ImageAnimationPolicy
animation_policy;
How about using in-line initialization to make the default values more obvious
here?

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/settin...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/settin...
blimp/engine/app/settings_manager.cc:28: prefs->animation_policy =
settings_.animation_policy;
We should also add a SettingsManager test to verify that it copies all the
settings into |prefs| correctly.

[Khushal, 2016-04-21 01:20:18]

Done. Uploaded the same change twice by mistake. :P

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/1903873003/diff/1/blimp/engine/app/settings_m...
blimp/engine/app/settings_manager.cc:25: DCHECK(prefs);
On 2016/04/21 00:40:28, Wez wrote:
> On 2016/04/21 00:21:25, Khushal wrote:
> > On 2016/04/20 23:09:06, Wez wrote:
> > > nit: Also not specific to this CL, but no point DCHECK()ing a pointer
> > > immediately before you de-reference it!
> > 
> > The DCHECK helps since it gives a clear stack trace in case someone passes a
> > nullptr rather than a segmentation fault.
> 
> The stack trace will be the same, surely? The only difference will be that the
> failing pointer is a small integer, rather than actually zero.

Woops, I didn't notice that. :P
Done.

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
File blimp/engine/app/engine_settings.cc (right):

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.cc:14: // for details.
On 2016/04/21 00:40:28, Wez wrote:
> nit: I don't think you need the ImageAnimationPolicy.h mention here; reader
can
> look up the enum themselves to find the relevant comments.

Done.

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
File blimp/engine/app/engine_settings.h (right):

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.h:25: content::ImageAnimationPolicy
animation_policy;
On 2016/04/21 00:40:29, Wez wrote:
> How about using in-line initialization to make the default values more obvious
> here?

Makes sense. Done.

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/settin...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/1903873003/diff/20001/blimp/engine/app/settin...
blimp/engine/app/settings_manager.cc:28: prefs->animation_policy =
settings_.animation_policy;
On 2016/04/21 00:40:29, Wez wrote:
> We should also add a SettingsManager test to verify that it copies all the
> settings into |prefs| correctly.

Done.

[Wez, 2016-04-21 17:29:02]

LGTM, but (having review Chromium style guide ;) please consider the notes on
simplifying the EngineSettings & getting rid of the .cc

https://codereview.chromium.org/1903873003/diff/60001/blimp/engine/app/engine...
File blimp/engine/app/engine_settings.h (right):

https://codereview.chromium.org/1903873003/diff/60001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.h:17: EngineSettings();
Since this struct is pure POD I think you can declare this ctor inline and avoid
the cc file.

https://codereview.chromium.org/1903873003/diff/60001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.h:19: ~EngineSettings();
Do you actually need an explicit copy ctor and dtor, since this is a pure POD
struct?

[Khushal, 2016-04-21 18:45:48]

Done. Thanks!

https://codereview.chromium.org/1903873003/diff/60001/blimp/engine/app/engine...
File blimp/engine/app/engine_settings.h (right):

https://codereview.chromium.org/1903873003/diff/60001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.h:17: EngineSettings();
On 2016/04/21 17:29:02, Wez wrote:
> Since this struct is pure POD I think you can declare this ctor inline and
avoid
> the cc file.

Done.

https://codereview.chromium.org/1903873003/diff/60001/blimp/engine/app/engine...
blimp/engine/app/engine_settings.h:19: ~EngineSettings();
On 2016/04/21 17:29:02, Wez wrote:
> Do you actually need an explicit copy ctor and dtor, since this is a pure POD
> struct?

Nope. Removed.

[Khushal, 2016-04-21 18:45:58]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-04-21 18:45:58]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1903873003/#ps10004
(title: "Remove EngineSettings cc file.")

[commit-bot: I haz the power, 2016-04-21 18:47:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903873003/10004
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903873003/10004

[commit-bot: I haz the power, 2016-04-21 20:17:57]

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

[commit-bot: I haz the power, 2016-04-21 20:17:58]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Wez, 2016-04-21 20:47:03]

Failure looks unrelated to this CL; resubmitting.

[Wez, 2016-04-21 20:47:07]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-04-21 20:47:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903873003/10004
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903873003/10004

[commit-bot: I haz the power, 2016-04-21 21:38:41]

Committed patchset #5 (id:10004)

[commit-bot: I haz the power, 2016-04-22 19:39:51]

Description was changed from

==========
blimp: Update animation policy for images on the blimp engine.

The Blimp Engine currently uses the default animation policy in blink,
which means for an image like a GIF the engine would trigger regular
commits to drive the animation. Change this to NO_ANIMATION, so only
the first frame of the animation is shown.

BUG=605190
==========

to

==========
blimp: Update animation policy for images on the blimp engine.

The Blimp Engine currently uses the default animation policy in blink,
which means for an image like a GIF the engine would trigger regular
commits to drive the animation. Change this to NO_ANIMATION, so only
the first frame of the animation is shown.

BUG=605190

Committed: https://crrev.com/03a74c3c04ff24b43fbeaf5dc0c3e013c364e500
Cr-Commit-Position: refs/heads/master@{#388908}
==========

[commit-bot: I haz the power, 2016-04-22 19:39:52]

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