Suppress spurious foreground/background notifications

Suppress spurious foreground/background notifications

WebContentsObserver::WasShown/WasHidden can be invoked for WebContents that are already in that state.

This can cause us to track first foreground/background time incorrectly.

This change suppresses shown/hidden notifications when we're already in that state, so our first fg/bg time tracking works correctly.

BUG=590476
Committed: https://crrev.com/8de43b1b18151fd3428cb9812a95254e36770f84
Cr-Commit-Position: refs/heads/master@{#378212}

[Bryan McQuade, 2016-02-27 22:05:38]

Description was changed from

==========
Audit that foreground/background state is consistent.

BUG=
==========

to

==========
Suppress spurious foreground/background notifications

WebContentsObserver::WasShown/WasHidden can be invoked for WebContents that are
already in that state.

This can cause us to track first foreground/background time incorrectly.

This change suppresses shown/hidden notifications when we're already in that
state, so our first fg/bg tracking time works correctly.

BUG=590476
==========

[Bryan McQuade, 2016-02-27 22:37:19]

Description was changed from

==========
Suppress spurious foreground/background notifications

WebContentsObserver::WasShown/WasHidden can be invoked for WebContents that are
already in that state.

This can cause us to track first foreground/background time incorrectly.

This change suppresses shown/hidden notifications when we're already in that
state, so our first fg/bg tracking time works correctly.

BUG=590476
==========

to

==========
Suppress spurious foreground/background notifications

WebContentsObserver::WasShown/WasHidden can be invoked for WebContents that are
already in that state.

This can cause us to track first foreground/background time incorrectly.

This change suppresses shown/hidden notifications when we're already in that
state, so our first fg/bg time tracking works correctly.

BUG=590476
==========

[Bryan McQuade, 2016-02-28 22:59:07]

bmcquade@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2016-02-29 14:33:12]

Looks good. Why bandaid this at our end and not at the WebContents layer? Does
any code rely on this behavior?

[Bryan McQuade, 2016-02-29 14:36:27]

On 2016/02/29 at 14:33:12, csharrison wrote:
> Looks good. Why bandaid this at our end and not at the WebContents layer? Does
any code rely on this behavior?

Good question. There seem to be numerous places where hidden/shown gets invoked
for a WC that's already hidden or shown. It's not clearly documented whether
this is a bug or not.

We should figure out if this is a bug, and if so, open an issue, but that's
going to be a much larger change, and I'd like to make sure our metrics are
correct in the meantime.

I'll start a thread to see if this is the expected behavior or not.

[Charlie Harrison, 2016-02-29 14:39:54]

On 2016/02/29 14:33:12, csharrison wrote:
> Looks good. Why bandaid this at our end and not at the WebContents layer? Does
> any code rely on this behavior?

Sorry I just now noticed your other CL :P

I think I would prefer not modifying this code at all (maybe keep DCHECKs).
Instead, move DCHECKs / logic to the WebContents layer, so we consume an API w/
guarantees.

[Charlie Harrison, 2016-02-29 14:42:00]

On 2016/02/29 14:39:54, csharrison wrote:
> On 2016/02/29 14:33:12, csharrison wrote:
> > Looks good. Why bandaid this at our end and not at the WebContents layer?
Does
> > any code rely on this behavior?
> 
> Sorry I just now noticed your other CL :P
> 
> I think I would prefer not modifying this code at all (maybe keep DCHECKs).
> Instead, move DCHECKs / logic to the WebContents layer, so we consume an API
w/
> guarantees.

Oops didn't see your previous comment. This change lgtm as a temporary stopgap,
but I don't think it should be in the codebase long term if possible.

[Bryan McQuade, 2016-02-29 14:47:31]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-02-29 14:47:41]

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

[commit-bot: I haz the power, 2016-02-29 15:38:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-29 15:39:16]

Description was changed from

==========
Suppress spurious foreground/background notifications

WebContentsObserver::WasShown/WasHidden can be invoked for WebContents that are
already in that state.

This can cause us to track first foreground/background time incorrectly.

This change suppresses shown/hidden notifications when we're already in that
state, so our first fg/bg time tracking works correctly.

BUG=590476
==========

to

==========
Suppress spurious foreground/background notifications

WebContentsObserver::WasShown/WasHidden can be invoked for WebContents that are
already in that state.

This can cause us to track first foreground/background time incorrectly.

This change suppresses shown/hidden notifications when we're already in that
state, so our first fg/bg time tracking works correctly.

BUG=590476

Committed: https://crrev.com/8de43b1b18151fd3428cb9812a95254e36770f84
Cr-Commit-Position: refs/heads/master@{#378212}
==========

[commit-bot: I haz the power, 2016-02-29 15:39:17]

Patchset 2 (id:??) landed as
https://crrev.com/8de43b1b18151fd3428cb9812a95254e36770f84
Cr-Commit-Position: refs/heads/master@{#378212}