Support IPH NotifyEvent calls before Initialize

Support IPH NotifyEvent calls before Initialize

Previously any attempt to call FeatureEngagementTracker::NotifyEvent()
would crash if the Model and Store had not finished initializing yet.
This adds support for queuing up calls to NotifyEvent() so that once the
underlying Model and Store are initialized they can properly receive the
calls.

This is done by building a Model implementation that wraps another Model
implementation and queues up calls if the underlying Model is not ready.

BUG=706309
Review-Url: https://codereview.chromium.org/2877243002
Cr-Commit-Position: refs/heads/master@{#471528}
Committed: https://chromium.googlesource.com/chromium/src/+/f5bb53683cec36b37334b069b35b7c42b76f59a0

[David Trainor- moved to gerrit, 2017-05-12 23:04:21]

dtrainor@chromium.org changed reviewers:
+ nyquist@chromium.org

[David Trainor- moved to gerrit, 2017-05-12 23:04:21]

ptal

[nyquist, 2017-05-12 23:12:24]

Overall lgtm!

It's probably good to remove the extra line from //components/BUILD.gn though
:-p

And if you disagree with wrapping the Model in an InitAwareModel I'm happy to
discuss of course!

https://codereview.chromium.org/2877243002/diff/1/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/2877243002/diff/1/components/BUILD.gn#newcode244
components/BUILD.gn:244: deps += [
"//components/feature_engagement_tracker:unit_tests" ]
I don't think you want this one to be part of your CL ;-)

https://codereview.chromium.org/2877243002/diff/1/components/feature_engageme...
File
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc
(right):

https://codereview.chromium.org/2877243002/diff/1/components/feature_engageme...
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc:45:
base::MakeUnique<ModelImpl>(base::MakeUnique<InMemoryStore>(),
It looks like the InMemoryStore posts the result of the Load(). Even if it's
always successful, doesn't that have the same issue as below where you might get
a call to NotifyEvent(...) before that callback happens, i.e. DCHECK()?

We can discuss, but it feels like this should also be wrapped in an
InitAwareModel. I might have misread the code though.

[David Trainor- moved to gerrit, 2017-05-13 00:03:52]

https://codereview.chromium.org/2877243002/diff/1/components/feature_engageme...
File
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc
(right):

https://codereview.chromium.org/2877243002/diff/1/components/feature_engageme...
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc:45:
base::MakeUnique<ModelImpl>(base::MakeUnique<InMemoryStore>(),
On 2017/05/12 23:12:24, nyquist (nychthemeron ping) wrote:
> It looks like the InMemoryStore posts the result of the Load(). Even if it's
> always successful, doesn't that have the same issue as below where you might
get
> a call to NotifyEvent(...) before that callback happens, i.e. DCHECK()?
> 
> We can discuss, but it feels like this should also be wrapped in an
> InitAwareModel. I might have misread the code though.

Line 49 has you covered don't worry.

[David Trainor- moved to gerrit, 2017-05-13 00:03:58]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-05-13 00:03:58]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2877243002/#ps20001
(title: "Fixed build.gn")

[commit-bot: I haz the power, 2017-05-13 00:04: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-05-13 01:20:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494633838308120,
"parent_rev": "8b17479ec85a247bfc0956ac04a27d06a19d62db", "commit_rev":
"f5bb53683cec36b37334b069b35b7c42b76f59a0"}

[commit-bot: I haz the power, 2017-05-13 01:20:55]

Description was changed from

==========
Support IPH NotifyEvent calls before Initialize

Previously any attempt to call FeatureEngagementTracker::NotifyEvent()
would crash if the Model and Store had not finished initializing yet.
This adds support for queuing up calls to NotifyEvent() so that once the
underlying Model and Store are initialized they can properly receive the
calls.

This is done by building a Model implementation that wraps another Model
implementation and queues up calls if the underlying Model is not ready.

BUG=706309
==========

to

==========
Support IPH NotifyEvent calls before Initialize

Previously any attempt to call FeatureEngagementTracker::NotifyEvent()
would crash if the Model and Store had not finished initializing yet.
This adds support for queuing up calls to NotifyEvent() so that once the
underlying Model and Store are initialized they can properly receive the
calls.

This is done by building a Model implementation that wraps another Model
implementation and queues up calls if the underlying Model is not ready.

BUG=706309

Review-Url: https://codereview.chromium.org/2877243002
Cr-Commit-Position: refs/heads/master@{#471528}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5bb53683cec36b37334b069b35b...
==========

[commit-bot: I haz the power, 2017-05-13 01:20:56]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f5bb53683cec36b37334b069b35b...