OwnedRegisterTest now dervives from HeliumTest.

OwnedRegisterTest now dervives from HeliumTest.

Updates test class to dervive from HeliumTest, which contributes
ShadowingAtExitManager.

BUG=658798
Committed: https://crrev.com/c69887e2547ece792256051d5fcce3f6d575b25d
Cr-Commit-Position: refs/heads/master@{#433046}

[CJ, 2016-11-01 18:51:49]

lethalantidote@chromium.org changed reviewers:
+ scf@chromium.org, xyzzyz@chromium.org

[xyzzyz, 2016-11-01 18:53:07]

lgtm

[CJ, 2016-11-01 18:53:26]

The CQ bit was checked by lethalantidote@chromium.org

[commit-bot: I haz the power, 2016-11-01 18:53: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, 2016-11-01 18:53:49]

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

[commit-bot: I haz the power, 2016-11-01 18:53:50]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[scf, 2016-11-01 18:56:04]

lgtm

[scf, 2016-11-01 18:56:05]

lgtm

[CJ, 2016-11-01 20:36:24]

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

[CJ, 2016-11-01 20:36:24]

+khushalsagar Owners.

[Khushal, 2016-11-02 08:40:36]

On 2016/11/01 20:36:24, CJ wrote:
> +khushalsagar Owners.

It looks like we want to fix it the right way. So a few questions:

1) It looks like the RevisionGeneratorTest is working with hard-coded values
just to verify that you get monotonically increasing revisions. Why not take the
current value and compare against that to make sure that the next call gives an
incremented revision?

2) That's a lot of complexity just to get a monotonically increasing number. Did
you consider using base::StaticAtomicSequenceNumber?

3) Lastly, I'm not sure I understand the point of having a global revision
generator, when all it is being used for is generating revision numbers for
Syncables. Aren't revisions supposed to be scoped to a particular Syncable? What
would go wrong if revision numbers for 2 Syncables collide?

[scf, 2016-11-03 18:09:47]

On 2016/11/02 08:40:36, Khushal wrote:
> On 2016/11/01 20:36:24, CJ wrote:
> > +khushalsagar Owners.
> 
> It looks like we want to fix it the right way. So a few questions:
> 
> 1) It looks like the RevisionGeneratorTest is working with hard-coded values
> just to verify that you get monotonically increasing revisions. Why not take
the
> current value and compare against that to make sure that the next call gives
an
> incremented revision?
> 
We want to catch non-initialized bug.

> 2) That's a lot of complexity just to get a monotonically increasing number.
Did
> you consider using base::StaticAtomicSequenceNumber?

We don't need multi threaded support. RevisionGenerator::GetNext is expected to
be accessed from the same thread sequence.
> 
> 3) Lastly, I'm not sure I understand the point of having a global revision
> generator, when all it is being used for is generating revision numbers for
> Syncables. Aren't revisions supposed to be scoped to a particular Syncable?
What
> would go wrong if revision numbers for 2 Syncables collide?

Revisions are scoped for within a single syncable you are right, but we (Kevin,
Wez) decided a global per host revision generator would make debugging easier as
we could infer order between Revision of different syncables in the same host.

[Khushal, 2016-11-03 19:34:00]

On 2016/11/03 18:09:47, scf wrote:
> On 2016/11/02 08:40:36, Khushal wrote:
> > On 2016/11/01 20:36:24, CJ wrote:
> > > +khushalsagar Owners.
> > 
> > It looks like we want to fix it the right way. So a few questions:
> > 
> > 1) It looks like the RevisionGeneratorTest is working with hard-coded values
> > just to verify that you get monotonically increasing revisions. Why not take
> the
> > current value and compare against that to make sure that the next call gives
> an
> > incremented revision?
> > 
> We want to catch non-initialized bug.

Hmmm, could we add a DCHECK in the RevisionGenerator's ctor for doing that?
DCHECK_EQ(revision_, 0);

> 
> > 2) That's a lot of complexity just to get a monotonically increasing number.
> Did
> > you consider using base::StaticAtomicSequenceNumber?
> 
> We don't need multi threaded support. RevisionGenerator::GetNext is expected
to
> be accessed from the same thread sequence.
> > 
> > 3) Lastly, I'm not sure I understand the point of having a global revision
> > generator, when all it is being used for is generating revision numbers for
> > Syncables. Aren't revisions supposed to be scoped to a particular Syncable?
> What
> > would go wrong if revision numbers for 2 Syncables collide?
> 
> Revisions are scoped for within a single syncable you are right, but we
(Kevin,
> Wez) decided a global per host revision generator would make debugging easier
as
> we could infer order between Revision of different syncables in the same host.

I see. We should use tracing for debugging that needs to track ordering of
events. For instance,
https://cs.chromium.org/chromium/src/blimp/engine/session/tab.cc?l=67
Any reason that wouldn't suffice here?

Also, I'm not sure whether this strategy will work. We'll have to enforce that
every Syncable implementation use this revision generator, and I don't know how
we do that. For instance, the rendering syncable is going to be in the renderer
process. So now should we not generate revisions there and always pass them from
the browser?

[Khushal, 2016-11-03 19:36:27]

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

[Khushal, 2016-11-03 19:36:27]

Woops, forgot to send the mail.
+Kevin on this too.

[scf, 2016-11-04 18:21:14]

Sorry didn't realize there was questions still here. Thanks @lethalantidote for
letting me know.

Inline...

On 2016/11/03 19:34:00, Khushal wrote:
> On 2016/11/03 18:09:47, scf wrote:
> > On 2016/11/02 08:40:36, Khushal wrote:
> > > On 2016/11/01 20:36:24, CJ wrote:
> > > > +khushalsagar Owners.
> > > 
> > > It looks like we want to fix it the right way. So a few questions:
> > > 
> > > 1) It looks like the RevisionGeneratorTest is working with hard-coded
values
> > > just to verify that you get monotonically increasing revisions. Why not
take
> > the
> > > current value and compare against that to make sure that the next call
gives
> > an
> > > incremented revision?
> > > 
> > We want to catch non-initialized bug.
> 
> Hmmm, could we add a DCHECK in the RevisionGenerator's ctor for doing that?
> DCHECK_EQ(revision_, 0);
> 
True, I'm fine with that.

> > 
> > > 2) That's a lot of complexity just to get a monotonically increasing
number.
> > Did
> > > you consider using base::StaticAtomicSequenceNumber?
> > 
> > We don't need multi threaded support. RevisionGenerator::GetNext is expected
> to
> > be accessed from the same thread sequence.
> > > 
> > > 3) Lastly, I'm not sure I understand the point of having a global revision
> > > generator, when all it is being used for is generating revision numbers
for
> > > Syncables. Aren't revisions supposed to be scoped to a particular
Syncable?
> > What
> > > would go wrong if revision numbers for 2 Syncables collide?
> > 
> > Revisions are scoped for within a single syncable you are right, but we
> (Kevin,
> > Wez) decided a global per host revision generator would make debugging
easier
> as
> > we could infer order between Revision of different syncables in the same
host.
> 
> I see. We should use tracing for debugging that needs to track ordering of
> events. For instance,
> https://cs.chromium.org/chromium/src/blimp/engine/session/tab.cc?l=67
> Any reason that wouldn't suffice here?
> 
I think the idea was inferring things automatically without worrying about
looking at the logs at all.

> Also, I'm not sure whether this strategy will work. We'll have to enforce that
> every Syncable implementation use this revision generator, and I don't know
how
> we do that. For instance, the rendering syncable is going to be in the
renderer
> process. So now should we not generate revisions there and always pass them
from
> the browser?

I remember @wez saying that Syncables should always be accessed withing the same
sequence. If that is not the case you are right this might not work.

[Khushal, 2016-11-04 18:37:05]

On 2016/11/04 18:21:14, scf wrote:
> Sorry didn't realize there was questions still here. Thanks @lethalantidote
for
> letting me know.
> 
> Inline...
> 
> On 2016/11/03 19:34:00, Khushal wrote:
> > On 2016/11/03 18:09:47, scf wrote:
> > > On 2016/11/02 08:40:36, Khushal wrote:
> > > > On 2016/11/01 20:36:24, CJ wrote:
> > > > > +khushalsagar Owners.
> > > > 
> > > > It looks like we want to fix it the right way. So a few questions:
> > > > 
> > > > 1) It looks like the RevisionGeneratorTest is working with hard-coded
> values
> > > > just to verify that you get monotonically increasing revisions. Why not
> take
> > > the
> > > > current value and compare against that to make sure that the next call
> gives
> > > an
> > > > incremented revision?
> > > > 
> > > We want to catch non-initialized bug.
> > 
> > Hmmm, could we add a DCHECK in the RevisionGenerator's ctor for doing that?
> > DCHECK_EQ(revision_, 0);
> > 
> True, I'm fine with that.
> 
> > > 
> > > > 2) That's a lot of complexity just to get a monotonically increasing
> number.
> > > Did
> > > > you consider using base::StaticAtomicSequenceNumber?
> > > 
> > > We don't need multi threaded support. RevisionGenerator::GetNext is
expected
> > to
> > > be accessed from the same thread sequence.
> > > > 
> > > > 3) Lastly, I'm not sure I understand the point of having a global
revision
> > > > generator, when all it is being used for is generating revision numbers
> for
> > > > Syncables. Aren't revisions supposed to be scoped to a particular
> Syncable?
> > > What
> > > > would go wrong if revision numbers for 2 Syncables collide?
> > > 
> > > Revisions are scoped for within a single syncable you are right, but we
> > (Kevin,
> > > Wez) decided a global per host revision generator would make debugging
> easier
> > as
> > > we could infer order between Revision of different syncables in the same
> host.
> > 
> > I see. We should use tracing for debugging that needs to track ordering of
> > events. For instance,
> > https://cs.chromium.org/chromium/src/blimp/engine/session/tab.cc?l=67
> > Any reason that wouldn't suffice here?
> > 
> I think the idea was inferring things automatically without worrying about
> looking at the logs at all.

Huh, I'm confused. Inferring things how and where? Either this will be used in
code, or when you debug where you will either print logs, collect a trace, etc.
May be I'm not understanding the use case clearly?

> 
> > Also, I'm not sure whether this strategy will work. We'll have to enforce
that
> > every Syncable implementation use this revision generator, and I don't know
> how
> > we do that. For instance, the rendering syncable is going to be in the
> renderer
> > process. So now should we not generate revisions there and always pass them
> from
> > the browser?
> 
> I remember @wez saying that Syncables should always be accessed withing the
same
> sequence. If that is not the case you are right this might not work.

Yes, but there is no policy on how they should generate revisions. I can always
have the real Syncable implementation (for LazySyncables) be on a different
thread, and a proxy Syncable to it on the sync layer's thread. The revisions are
generated by the implementation, and updates are posted back to the proxy
syncable with its revision, and the call to GetRevision on the sync layer's
thread will return this revision. So the thread where the revision is being
generated does not need to be the same thread where the Syncable exists.

[scf, 2016-11-04 19:21:18]

On 2016/11/04 18:37:05, Khushal wrote:
> On 2016/11/04 18:21:14, scf wrote:
> > Sorry didn't realize there was questions still here. Thanks @lethalantidote
> for
> > letting me know.
> > 
> > Inline...
> > 
> > On 2016/11/03 19:34:00, Khushal wrote:
> > > On 2016/11/03 18:09:47, scf wrote:
> > > > On 2016/11/02 08:40:36, Khushal wrote:
> > > > > On 2016/11/01 20:36:24, CJ wrote:
> > > > > > +khushalsagar Owners.
> > > > > 
> > > > > It looks like we want to fix it the right way. So a few questions:
> > > > > 
> > > > > 1) It looks like the RevisionGeneratorTest is working with hard-coded
> > values
> > > > > just to verify that you get monotonically increasing revisions. Why
not
> > take
> > > > the
> > > > > current value and compare against that to make sure that the next call
> > gives
> > > > an
> > > > > incremented revision?
> > > > > 
> > > > We want to catch non-initialized bug.
> > > 
> > > Hmmm, could we add a DCHECK in the RevisionGenerator's ctor for doing
that?
> > > DCHECK_EQ(revision_, 0);
> > > 
> > True, I'm fine with that.
> > 
> > > > 
> > > > > 2) That's a lot of complexity just to get a monotonically increasing
> > number.
> > > > Did
> > > > > you consider using base::StaticAtomicSequenceNumber?
> > > > 
> > > > We don't need multi threaded support. RevisionGenerator::GetNext is
> expected
> > > to
> > > > be accessed from the same thread sequence.
> > > > > 
> > > > > 3) Lastly, I'm not sure I understand the point of having a global
> revision
> > > > > generator, when all it is being used for is generating revision
numbers
> > for
> > > > > Syncables. Aren't revisions supposed to be scoped to a particular
> > Syncable?
> > > > What
> > > > > would go wrong if revision numbers for 2 Syncables collide?
> > > > 
> > > > Revisions are scoped for within a single syncable you are right, but we
> > > (Kevin,
> > > > Wez) decided a global per host revision generator would make debugging
> > easier
> > > as
> > > > we could infer order between Revision of different syncables in the same
> > host.
> > > 
> > > I see. We should use tracing for debugging that needs to track ordering of
> > > events. For instance,
> > > https://cs.chromium.org/chromium/src/blimp/engine/session/tab.cc?l=67
> > > Any reason that wouldn't suffice here?
> > > 
> > I think the idea was inferring things automatically without worrying about
> > looking at the logs at all.
> 
> Huh, I'm confused. Inferring things how and where? Either this will be used in
> code, or when you debug where you will either print logs, collect a trace,
etc.
> May be I'm not understanding the use case clearly?
> 
> > 
> > > Also, I'm not sure whether this strategy will work. We'll have to enforce
> that
> > > every Syncable implementation use this revision generator, and I don't
know
> > how
> > > we do that. For instance, the rendering syncable is going to be in the
> > renderer
> > > process. So now should we not generate revisions there and always pass
them
> > from
> > > the browser?
> > 
> > I remember @wez saying that Syncables should always be accessed withing the
> same
> > sequence. If that is not the case you are right this might not work.
> 
> Yes, but there is no policy on how they should generate revisions. I can
always
> have the real Syncable implementation (for LazySyncables) be on a different
> thread, and a proxy Syncable to it on the sync layer's thread. The revisions
are
> generated by the implementation, and updates are posted back to the proxy
> syncable with its revision, and the call to GetRevision on the sync layer's
> thread will return this revision. So the thread where the revision is being
> generated does not need to be the same thread where the Syncable exists.

I think we are not in the same page. My understanding was that after
TwoPhaseSyncable::PreCreateChangesetToCurrent was called and fetched the data,
the Revision would be generated using the syncable sequence so we would be safe
here. @wez probably is more knowledge on the threading here and can probably add
more.

[Kevin M, 2016-11-04 19:52:54]

We can asynchronously grab data from different threads via PostTask, but
actually *committing* those changes to the State it object itself -- and thereby
getting new revisions from the generator -- will always happen on the same
thread.

[Khushal, 2016-11-04 20:21:25]

On 2016/11/04 19:52:54, Kevin M wrote:
> We can asynchronously grab data from different threads via PostTask, but
> actually *committing* those changes to the State it object itself -- and
thereby
> getting new revisions from the generator -- will always happen on the same
> thread.

There will be revision tracking necessary on the implementation side, to
generate diffs when you're asked to generate a revision from some arbitrary
point, so the implementation needs the revision when generating the changeset.
You can always generate the revision on the sync thread side, and pass that to
the implementation side when asking for a changeset. I'm not saying that this is
not possible, but I don't want to have to jump through hoops and make every
Syncable implementation have to take care of this just for use in "debugging".
Also, I'm weary of baking this requirement in the design, so that we end up
having some logic that actually starts relying and using this just because the
code supports it.

I'm still waiting for an answer to, "Why can't we use tracing for this?"

[Kevin M, 2016-11-15 19:38:10]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-11-15 19:38:11]

lgtm

[commit-bot: I haz the power, 2016-11-15 19:38:57]

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-11-15 19:52:04]

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

[commit-bot: I haz the power, 2016-11-15 19:52:06]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[CJ, 2016-11-15 21:39:09]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-11-15 21:39:09]

The patchset sent to the CQ was uploaded after l-g-t-m from xyzzyz@chromium.org,
scf@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2470883002/#ps20001
(title: "Merge branch 'master' into fixtest")

[commit-bot: I haz the power, 2016-11-15 21:39:32]

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-11-15 22:03:31]

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

[commit-bot: I haz the power, 2016-11-15 22:03:33]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[CJ, 2016-11-15 22:25:02]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-11-15 22:25:02]

The patchset sent to the CQ was uploaded after l-g-t-m from xyzzyz@chromium.org,
scf@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2470883002/#ps40001
(title: "Merge branch 'master' into fixtest")

[commit-bot: I haz the power, 2016-11-15 22:26: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-11-15 22:58:56]

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

[commit-bot: I haz the power, 2016-11-15 22:58:58]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[CJ, 2016-11-16 23:57:17]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-11-16 23:57:17]

The patchset sent to the CQ was uploaded after l-g-t-m from xyzzyz@chromium.org,
scf@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2470883002/#ps60001
(title: "Merge branch 'master' into fixtest")

[commit-bot: I haz the power, 2016-11-16 23:58:20]

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-11-17 00:47:23]

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

[commit-bot: I haz the power, 2016-11-17 00:47:24]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[CJ, 2016-11-18 00:19:53]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-11-18 00:19:53]

The patchset sent to the CQ was uploaded after l-g-t-m from xyzzyz@chromium.org,
scf@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2470883002/#ps80001
(title: "Merge branch 'master' into fixtest")

[commit-bot: I haz the power, 2016-11-18 00:21:49]

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-11-18 03:13:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-18 03:16:34]

Description was changed from

==========
OwnedRegisterTest now dervives from HeliumTest.

Updates test class to dervive from HeliumTest, which contributes
ShadowingAtExitManager.

BUG=658798
==========

to

==========
OwnedRegisterTest now dervives from HeliumTest.

Updates test class to dervive from HeliumTest, which contributes
ShadowingAtExitManager.

BUG=658798

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

[commit-bot: I haz the power, 2016-11-18 03:16:35]

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