Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to
not update the current instance variables and just save the updated
values to prefs. This is needed because HandleViewed can be called
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return
false during the same layout. By not saving the updated values to the
current instance variables, the CanShow calls will remain consistent
during the same layout. To facilitate this change, this CL also changes
the return type of HandleViewed to void since the return value is never
used.

BUG=625192
Committed: https://crrev.com/ab57275f5b35d23c5d8504782f229cebdb7de8eb
Cr-Commit-Position: refs/heads/master@{#407218}

[gchatz, 2016-07-15 23:57:14]

Description was changed from

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods
to only save the updated views/first_view_time/closed variables to
prefs instead of also updating the current instance variables. This is
needed because HandleViewed can be called in the middle of the chain
of CanShow calls needed to layout the NTP. So, during the
initialization of the NTP, some CanShow calls can return true while
others can return false, thus causing layout bugs. To facilitate this
change, this CL also changes the return type of HandleViewed to void
since the return value is never used.

BUG=625192
==========

to

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to not
update the current instance variables and just save
the updated values to prefs. This is needed because HandleViewed
can be called in the middle of the chain of CanShow calls made 
during layout of the NTP, causing some CanShow calls to 
return true while others to return false during the same layout. By
not saving the updated values to the current instance variables, the
CanShow calls will remain consistent during the same layout. To 
facilitate this change, this CL also changes the return type of HandleViewed to
void
since the return value is never used.

BUG=625192
==========

[gchatz, 2016-07-15 23:59:06]

Description was changed from

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to not
update the current instance variables and just save
the updated values to prefs. This is needed because HandleViewed
can be called in the middle of the chain of CanShow calls made 
during layout of the NTP, causing some CanShow calls to 
return true while others to return false during the same layout. By
not saving the updated values to the current instance variables, the
CanShow calls will remain consistent during the same layout. To 
facilitate this change, this CL also changes the return type of HandleViewed to
void
since the return value is never used.

BUG=625192
==========

to

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to 
not update the current instance variables and just save
the updated values to prefs. This is needed because HandleViewed
can be called in the middle of the chain of CanShow calls made 
during layout of the NTP, causing some CanShow calls to 
return true while others to return false during the same layout. 
By not saving the updated values to the current instance variables,
the CanShow calls will remain consistent during the same layout. To 
facilitate this change, this CL also changes the return type of 
HandleViewed to void since the return value is never used.

BUG=625192
==========

[gchatz, 2016-07-16 00:33:36]

Description was changed from

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to 
not update the current instance variables and just save
the updated values to prefs. This is needed because HandleViewed
can be called in the middle of the chain of CanShow calls made 
during layout of the NTP, causing some CanShow calls to 
return true while others to return false during the same layout. 
By not saving the updated values to the current instance variables,
the CanShow calls will remain consistent during the same layout. To 
facilitate this change, this CL also changes the return type of 
HandleViewed to void since the return value is never used.

BUG=625192
==========

to

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to 
not update the current instance variables and just save the updated 
values to prefs. This is needed because HandleViewed can be called 
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return 
false during the same layout. By not saving the updated values to the 
current instance variables, the CanShow calls will remain consistent 
during the same layout. To facilitate this change, this CL also changes 
the return type of HandleViewed to void since the return value is never 
used.

BUG=625192
==========

[gchatz, 2016-07-16 00:34:05]

gchatz@chromium.org changed reviewers:
+ rohitrao@chromium.org

[gchatz, 2016-07-16 00:34:22]

gchatz@chromium.org changed reviewers:
+ justincohen@chromium.org

[justincohen, 2016-07-18 00:00:39]

lgtm

[rohitrao (ping after 24h), 2016-07-19 11:40:13]

What is the lifetime of a NotificationPromo object?  Who owns it, and when is it
created/destroyed?

(I'm wondering how long one of these can stay alive, and whether that could
cause us to show promos when we shouldn't.)

[gchatz, 2016-07-19 18:38:47]

On 2016/07/19 11:40:13, rohitrao wrote:
> What is the lifetime of a NotificationPromo object?  Who owns it, and when is
it
> created/destroyed?
> 
> (I'm wondering how long one of these can stay alive, and whether that could
> cause us to show promos when we shouldn't.)

NotificationPromo is owned by NotificationPromoWhatsNew downstream, and is
created/destroyed when it is created/destroyed. NotificationPromoWhatsNew is
owned by GLC, and is created when GLC is, and is destroyed at the following
times:
1. when GLC is destroyed
2. when the promo is tapped
3. when GLC's reload method is called, and the promo should be hidden but isn't

When these methods were static, HandleViewed/Closed did not take affect until
prefs were loaded again. This change makes the app follow those steps again.

[gchatz, 2016-07-21 17:52:50]

On 2016/07/19 18:38:47, gchatz wrote:
> On 2016/07/19 11:40:13, rohitrao wrote:
> > What is the lifetime of a NotificationPromo object?  Who owns it, and when
is
> it
> > created/destroyed?
> > 
> > (I'm wondering how long one of these can stay alive, and whether that could
> > cause us to show promos when we shouldn't.)
> 
> NotificationPromo is owned by NotificationPromoWhatsNew downstream, and is
> created/destroyed when it is created/destroyed. NotificationPromoWhatsNew is
> owned by GLC, and is created when GLC is, and is destroyed at the following
> times:
> 1. when GLC is destroyed
> 2. when the promo is tapped
> 3. when GLC's reload method is called, and the promo should be hidden but
isn't
> 
> When these methods were static, HandleViewed/Closed did not take affect until
> prefs were loaded again. This change makes the app follow those steps again.

Friendly ping

[rohitrao (ping after 24h), 2016-07-22 16:40:57]

lgtm

[gchatz, 2016-07-22 18:13:37]

The CQ bit was checked by gchatz@chromium.org

[commit-bot: I haz the power, 2016-07-22 18:14:13]

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-07-22 18:51:50]

Description was changed from

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to 
not update the current instance variables and just save the updated 
values to prefs. This is needed because HandleViewed can be called 
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return 
false during the same layout. By not saving the updated values to the 
current instance variables, the CanShow calls will remain consistent 
during the same layout. To facilitate this change, this CL also changes 
the return type of HandleViewed to void since the return value is never 
used.

BUG=625192
==========

to

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to 
not update the current instance variables and just save the updated 
values to prefs. This is needed because HandleViewed can be called 
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return 
false during the same layout. By not saving the updated values to the 
current instance variables, the CanShow calls will remain consistent 
during the same layout. To facilitate this change, this CL also changes 
the return type of HandleViewed to void since the return value is never 
used.

BUG=625192
==========

[commit-bot: I haz the power, 2016-07-22 18:51:51]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-22 18:53:11]

Description was changed from

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to 
not update the current instance variables and just save the updated 
values to prefs. This is needed because HandleViewed can be called 
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return 
false during the same layout. By not saving the updated values to the 
current instance variables, the CanShow calls will remain consistent 
during the same layout. To facilitate this change, this CL also changes 
the return type of HandleViewed to void since the return value is never 
used.

BUG=625192
==========

to

==========
Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to
not update the current instance variables and just save the updated
values to prefs. This is needed because HandleViewed can be called
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return
false during the same layout. By not saving the updated values to the
current instance variables, the CanShow calls will remain consistent
during the same layout. To facilitate this change, this CL also changes
the return type of HandleViewed to void since the return value is never
used.

BUG=625192

Committed: https://crrev.com/ab57275f5b35d23c5d8504782f229cebdb7de8eb
Cr-Commit-Position: refs/heads/master@{#407218}
==========

[commit-bot: I haz the power, 2016-07-22 18:53:12]

Patchset 1 (id:??) landed as
https://crrev.com/ab57275f5b35d23c5d8504782f229cebdb7de8eb
Cr-Commit-Position: refs/heads/master@{#407218}