Disable info bar animations during automated testing.

Disable info bar animations during automated testing.

BUG=chromedriver:1683
Review-Url: https://codereview.chromium.org/2705293008
Cr-Commit-Position: refs/heads/master@{#452738}
Committed: https://chromium.googlesource.com/chromium/src/+/20e8838145837ef381ebe3116c4af6bb1734b116

[samuong, 2017-02-22 23:58:49]

samuong@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2017-02-23 00:33:56]

LGTM

https://codereview.chromium.org/2705293008/diff/20001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:52: // Info bar animations cause
viewport resizes. Disable them for automated
Nit: Infobar

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar.cc:58: animate = false;
Nit: Shorter:

  animate &= owner_->animations_enabled();

(2 places)

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_manager.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.cc:104: : infobars_enabled_(true),
animations_enabled_(true) {
Nit: You're welcome to move these initializations to the member declaration
points in the header if you want.

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_manager.h (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.h:96: bool animations_enabled() {
return animations_enabled_; }
Nit: const

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.h:115: void set_animations_enabled(bool
animations_enabled) {
Nit: Looks like in this class non-virtuals are usually declared above virtuals,
so I'd do that here.

[samuong, 2017-02-23 00:45:26]

https://codereview.chromium.org/2705293008/diff/20001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:52: // Info bar animations cause
viewport resizes. Disable them for automated
On 2017/02/23 00:33:55, Peter Kasting (backlogged) wrote:
> Nit: Infobar

Done.

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar.cc:58: animate = false;
On 2017/02/23 00:33:55, Peter Kasting (backlogged) wrote:
> Nit: Shorter:
> 
>   animate &= owner_->animations_enabled();
> 
> (2 places)

Done.

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_manager.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.cc:104: : infobars_enabled_(true),
animations_enabled_(true) {
On 2017/02/23 00:33:56, Peter Kasting (backlogged) wrote:
> Nit: You're welcome to move these initializations to the member declaration
> points in the header if you want.

Done. Just to confirm, you meant using brace-initialization?

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_manager.h (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.h:96: bool animations_enabled() {
return animations_enabled_; }
On 2017/02/23 00:33:56, Peter Kasting (backlogged) wrote:
> Nit: const

Done.

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.h:115: void set_animations_enabled(bool
animations_enabled) {
On 2017/02/23 00:33:56, Peter Kasting (backlogged) wrote:
> Nit: Looks like in this class non-virtuals are usually declared above
virtuals,
> so I'd do that here.

Done.

[Peter Kasting, 2017-02-23 00:51:34]

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
File components/infobars/core/infobar_manager.cc (right):

https://codereview.chromium.org/2705293008/diff/20001/components/infobars/cor...
components/infobars/core/infobar_manager.cc:104: : infobars_enabled_(true),
animations_enabled_(true) {
On 2017/02/23 00:45:26, samuong wrote:
> On 2017/02/23 00:33:56, Peter Kasting (backlogged) wrote:
> > Nit: You're welcome to move these initializations to the member declaration
> > points in the header if you want.
> 
> Done. Just to confirm, you meant using brace-initialization?

I would use "= true" in preference to "{true}", since AFAIK both are legal, and
that's what Chromium prefers in such cases:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab...

[samuong, 2017-02-23 04:27:19]

Done

[samuong, 2017-02-23 04:27:24]

The CQ bit was checked by samuong@chromium.org

[samuong, 2017-02-23 04:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2705293008/#ps60001
(title: "use = for init rather than {}")

[commit-bot: I haz the power, 2017-02-23 04:27:40]

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, 2017-02-23 05:07:14]

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

[commit-bot: I haz the power, 2017-02-23 05:07:15]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[samuong, 2017-02-23 22:28:22]

Patchset #5 (id:80001) has been deleted

[samuong, 2017-02-23 22:45:49]

Peter, it looks like I was wrong to assume that |InfoBar::owner_| was valid from
InfoBar::Show/Hide. In the case of the Hide method, we could dereference an
invalid pointer if we are being called from
InfoBarContainer::ChangeInfoBarManager.

So I've modified this to have InfoBarContainer check whether animations are
enabled. This happens in two places:

- InfoBarContainer::OnInfoBarRemoved, which is called from InfoBarManager
- InfoBarContainer::AddInfoBar, which is called from ChangeInfoBarManager (after
setting the new manager) and from InfoBarManager

In these cases, infobar_manager_ should be OK to dereference. Do you mind taking
another look to make sure this is OK?

[Peter Kasting, 2017-02-24 01:28:03]

LGTM

[samuong, 2017-02-24 02:31:17]

The CQ bit was checked by samuong@chromium.org

[commit-bot: I haz the power, 2017-02-24 02:31:48]

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, 2017-02-24 03:21:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487903477278000,
"parent_rev": "230c3f1c820ccb738e9715cd45b2b5e049d782d7", "commit_rev":
"20e8838145837ef381ebe3116c4af6bb1734b116"}

[commit-bot: I haz the power, 2017-02-24 03:21:51]

Description was changed from

==========
Disable info bar animations during automated testing.

BUG=chromedriver:1683
==========

to

==========
Disable info bar animations during automated testing.

BUG=chromedriver:1683

Review-Url: https://codereview.chromium.org/2705293008
Cr-Commit-Position: refs/heads/master@{#452738}
Committed:
https://chromium.googlesource.com/chromium/src/+/20e8838145837ef381ebe3116c4a...
==========

[commit-bot: I haz the power, 2017-02-24 03:21:52]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/20e8838145837ef381ebe3116c4a...