Make UserActivityDetector a singleton

Make UserActivityDetector a singleton

BUG=426561, 408752
TEST=None

Committed: https://crrev.com/c44cb64db7d79078401a3fe776a38eba94faf9e5
Cr-Commit-Position: refs/heads/master@{#302734}

[pkotwicz, 2014-10-31 00:51:35]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2014-10-31 01:19:50]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2014-10-31 01:24:42]

Patchset #1 (id:40001) has been deleted

[pkotwicz, 2014-10-31 01:27:45]

pkotwicz@chromium.org changed reviewers:
+ oshima@chromium.org

[pkotwicz, 2014-10-31 01:27:46]

Oshima, PTAL

I moved the creation of UserActivityDetector to ChromeBrowserMainPartsChromeos
because we do not seem to create singletons in shell.cc (with the exception of
aura::Env). I am unsure whether I should just leave the creation of
UserActivityDetector in shell.cc.

[pkotwicz, 2014-10-31 01:42:00]

Patchset #1 (id:60001) has been deleted

[Daniel Erat, 2014-10-31 15:51:15]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2014-10-31 15:51:16]

i strongly prefer explicit ownership of objects and don't like seeing more
singletons get created. were any other approaches considered?

please also make sure that you're not breaking app_shell.

[pkotwicz, 2014-10-31 16:52:58]

I suspect that it would be preferable to have a singleton which contains all of
the things which should be shared between Ash and Athena. I imagine that there
are other objects that we will want to share.

I can't think of other things from ash::Shell (other than
ash::Shell::GetPrimaryRootWindow()) that we would share with Athena. Oshima,
what do you think?

[oshima, 2014-10-31 16:53:31]

ctor/dtor and creation should probably be left as is as derat@ said. I don't
have strong feeling about ownership though (it sometimes have negative impact.
ash::Shell, g_browser_process are good examples).

Let us know if you have better idea to remove ash dependency.

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
File ui/wm/core/user_activity_detector.cc (right):

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
ui/wm/core/user_activity_detector.cc:17: UserActivityDetector* g_instance =
NULL;
My preference is no g_ for file scoped variable as it's for global scoepd
variable, but i'll leave this to owners.

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
ui/wm/core/user_activity_detector.cc:51: g_instance = new
UserActivityDetector();
Initialize in ctor/dtor.

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
File ui/wm/core/user_activity_detector.h (right):

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
ui/wm/core/user_activity_detector.h:33: static void Shutdown();
Can you keep ctor/dtor, and creation in shell. Just add static accessor only.

[pkotwicz, 2014-10-31 16:58:30]

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
File ui/wm/core/user_activity_detector.h (right):

https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
ui/wm/core/user_activity_detector.h:33: static void Shutdown();
For the sake of interest, which other classes have a static getter but not a
static constructor and destructor?

[oshima, 2014-10-31 17:22:57]

On 2014/10/31 16:58:30, pkotwicz wrote:
>
https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
> File ui/wm/core/user_activity_detector.h (right):
> 
>
https://codereview.chromium.org/693643004/diff/80001/ui/wm/core/user_activity...
> ui/wm/core/user_activity_detector.h:33: static void Shutdown();
> For the sake of interest, which other classes have a static getter but not a
> static constructor and destructor?

athena does something similar, although they do have Create as impl class is
hidden.
(I'm thinking of removing Shutdown and automate it at some point in the future)

[pkotwicz, 2014-10-31 17:40:16]

derat@, waiting for your comments before proceeding. I mostly want to make sure
that you are OK with oshima@'s suggestion in #10.

[Daniel Erat, 2014-10-31 17:55:21]

On 2014/10/31 17:40:16, pkotwicz wrote:
> derat@, waiting for your comments before proceeding. I mostly want to make
sure
> that you are OK with oshima@'s suggestion in #10.

so is the proposal that we'd still have clear ownership of the object, but it'd
also expose a static Get() method to return the singleton instance? is the main
issue that athena doesn't have a mega-class like ash::Shell that can own things
like this?

[pkotwicz, 2014-10-31 17:58:54]

Yes, that is the proposal.

The problem is that there is a need to get access to UserActivityDetector from
code which runs on both Athena and Ash. To rephrase, the problem is that there
is no mega-class which is shared by Athena and Ash which owns things like this.

[oshima, 2014-10-31 18:18:28]

On 2014/10/31 17:58:54, pkotwicz wrote:
> Yes, that is the proposal.
> 
> The problem is that there is a need to get access to UserActivityDetector from
> code which runs on both Athena and Ash. To rephrase, the problem is that there
> is no mega-class which is shared by Athena and Ash which owns things like
this.

And I'd like to avoid having such mega-class.

[Daniel Erat, 2014-10-31 18:24:37]

On 2014/10/31 18:18:28, oshima wrote:
> On 2014/10/31 17:58:54, pkotwicz wrote:
> > Yes, that is the proposal.
> > 
> > The problem is that there is a need to get access to UserActivityDetector
from
> > code which runs on both Athena and Ash. To rephrase, the problem is that
there
> > is no mega-class which is shared by Athena and Ash which owns things like
> this.
> 
> And I'd like to avoid having such mega-class.

i'm okay with this approach as long as the creation order is sane enough that
code that accesses the singleton doesn't end up looking like this:

  UserActivityDetector* detector = UserActivityDetector::Get();
  if (!detector)
    return;
  // use it...

when i see NULL checks like this, it implies that we don't know when the
singleton actually gets created, which seems like a bad sign.

[oshima, 2014-10-31 18:37:33]

On 2014/10/31 18:24:37, Daniel Erat wrote:
> On 2014/10/31 18:18:28, oshima wrote:
> > On 2014/10/31 17:58:54, pkotwicz wrote:
> > > Yes, that is the proposal.
> > > 
> > > The problem is that there is a need to get access to UserActivityDetector
> from
> > > code which runs on both Athena and Ash. To rephrase, the problem is that
> there
> > > is no mega-class which is shared by Athena and Ash which owns things like
> > this.
> > 
> > And I'd like to avoid having such mega-class.
> 
> i'm okay with this approach as long as the creation order is sane enough that
> code that accesses the singleton doesn't end up looking like this:
> 
>   UserActivityDetector* detector = UserActivityDetector::Get();
>   if (!detector)
>     return;
>   // use it...
> 
> when i see NULL checks like this, it implies that we don't know when the
> singleton actually gets created, which seems like a bad sign.

I agree 100%. Unfortunately we already have the code like this

if (ash::Shell::GetInstance())

in which case, we need the if like above. It'd be really nice if we can
eliminate them,
but it's probably outside of the scope of this CL.

[pkotwicz, 2014-10-31 21:36:54]

oshima@ and derat@ can you please take another look?

[pkotwicz, 2014-10-31 21:38:15]

Patchset #3 (id:120001) has been deleted

[pkotwicz, 2014-10-31 21:38:50]

Patchset #2 (id:100001) has been deleted

[Daniel Erat, 2014-10-31 21:55:49]

lgtm

[oshima, 2014-10-31 22:10:44]

lgtm

[pkotwicz, 2014-10-31 22:16:38]

pkotwicz@chromium.org changed reviewers:
+ sky@chromium.org

[pkotwicz, 2014-10-31 22:16:39]

sky@ for ui/ and chrome/browser/idle_chromeos.cc OWNERS

[sky, 2014-10-31 23:02:23]

Why do you want to convert to a singleton? Assuming there is a good reason you
should make the constructor/destructor private and use a singleton.

https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
File ui/wm/core/user_activity_detector.cc (right):

https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
ui/wm/core/user_activity_detector.cc:17: UserActivityDetector* g_instance =
NULL;
nullptr

[pkotwicz, 2014-10-31 23:22:16]

Oshima, can you please explain to Scott?

[stevenjb, 2014-11-03 16:51:23]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2014-11-03 16:51:23]

FWIW, I think this would be more robust if:

1) UserActivityDetector has an explicit (static) Initialize() and Shutdown()
method so that it is obvious where/when it is created and destroyed.
2) An explicit HasInstance() or IsInitialized() method is used in destructors /
shutdown methods (only) to test the existence of the singleton during shutdown
where ordering is not certain.
3) Get() has a CHECK(s_instance) so that it fails immediately if called outside
of Initialize / Shutdown.

(This is the pattern we use for ash::Shell, DBusThreadManager, and other
necessary singletons).

P.S. Looking through this CL, I missed where UserActivityDetector is constructed
/ destroyed?

[oshima, 2014-11-03 18:05:46]

On 2014/10/31 23:02:23, sky wrote:
> Why do you want to convert to a singleton?

Because we need this on both ash and athena, and we want this to be accessible
w/o mega class like ash::Shell
(or yet another mega class)

> Assuming there is a good reason you
> should make the constructor/destructor private and use a singleton.

We want to avoid singleton because it obfuscate construction/destruction order.
(it gets created when accessed,
but you can't delete until the end of the world).

> 
>
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
> File ui/wm/core/user_activity_detector.cc (right):
> 
>
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
> ui/wm/core/user_activity_detector.cc:17: UserActivityDetector* g_instance =
> NULL;
> nullptr

[oshima, 2014-11-03 18:16:29]

On 2014/11/03 16:51:23, stevenjb wrote:
> FWIW, I think this would be more robust if:
> 
> 1) UserActivityDetector has an explicit (static) Initialize() and Shutdown()
> method so that it is obvious where/when it is created and destroyed.
> 2) An explicit HasInstance() or IsInitialized() method is used in destructors
/
> shutdown methods (only) to test the existence of the singleton during shutdown
> where ordering is not certain.

I don't have strong feeling about these.

For 1), using ctr/dtor has more enforcement in the ctor/dtor order with
scoped_ptr.
athena uses this approach because we're trying to hide implementation from
public
interface, but it does not apply to this class. (doing so is an option though)

For 2), I personal want to avoid it because the such code that checks the
existence
implies some design issue, and should be discouraged. (Unfortunately, we already
have
such case though)

> 3) Get() has a CHECK(s_instance) so that it fails immediately if called
outside
> of Initialize / Shutdown.

Thanks, I missed this. peter can you add this?

> 
> (This is the pattern we use for ash::Shell, DBusThreadManager, and other
> necessary singletons).
> 
> P.S. Looking through this CL, I missed where UserActivityDetector is
constructed
> / destroyed?

dtr/dtor hasn't changed and that's why you don't see them.

[stevenjb, 2014-11-03 18:20:31]

On 2014/11/03 18:16:29, oshima wrote:
> On 2014/11/03 16:51:23, stevenjb wrote:
> > FWIW, I think this would be more robust if:
> > 
> > 1) UserActivityDetector has an explicit (static) Initialize() and Shutdown()
> > method so that it is obvious where/when it is created and destroyed.
> > 2) An explicit HasInstance() or IsInitialized() method is used in
destructors
> /
> > shutdown methods (only) to test the existence of the singleton during
shutdown
> > where ordering is not certain.
> 
> I don't have strong feeling about these.
> 
> For 1), using ctr/dtor has more enforcement in the ctor/dtor order with
> scoped_ptr.
> athena uses this approach because we're trying to hide implementation from
> public
> interface, but it does not apply to this class. (doing so is an option though)
> 
> For 2), I personal want to avoid it because the such code that checks the
> existence
> implies some design issue, and should be discouraged. (Unfortunately, we
already
> have
> such case though)
> 
> > 3) Get() has a CHECK(s_instance) so that it fails immediately if called
> outside
> > of Initialize / Shutdown.
> 
> Thanks, I missed this. peter can you add this?
> 
> > 
> > (This is the pattern we use for ash::Shell, DBusThreadManager, and other
> > necessary singletons).
> > 
> > P.S. Looking through this CL, I missed where UserActivityDetector is
> constructed
> > / destroyed?
> 
> dtr/dtor hasn't changed and that's why you don't see them.

I see. so Shell still owns it for Ash, but does not provide access to it. FWIW,
I find that pattern confusing.

WRT 2/3, because shutdown code may need to test for the existence of the
singleton, you can't do 3 without 2. (I prefer IsInitialized() over
HasInstance() for the reason you gave - we should only use that test in shutdown
or startup code where the initialization order ins not gauranteed).

[oshima, 2014-11-03 18:39:55]

On 2014/11/03 18:20:31, stevenjb wrote:
> On 2014/11/03 18:16:29, oshima wrote:
> > On 2014/11/03 16:51:23, stevenjb wrote:
> > > FWIW, I think this would be more robust if:
> > > 
> > > 1) UserActivityDetector has an explicit (static) Initialize() and
Shutdown()
> > > method so that it is obvious where/when it is created and destroyed.
> > > 2) An explicit HasInstance() or IsInitialized() method is used in
> destructors
> > /
> > > shutdown methods (only) to test the existence of the singleton during
> shutdown
> > > where ordering is not certain.
> > 
> > I don't have strong feeling about these.
> > 
> > For 1), using ctr/dtor has more enforcement in the ctor/dtor order with
> > scoped_ptr.
> > athena uses this approach because we're trying to hide implementation from
> > public
> > interface, but it does not apply to this class. (doing so is an option
though)
> > 
> > For 2), I personal want to avoid it because the such code that checks the
> > existence
> > implies some design issue, and should be discouraged. (Unfortunately, we
> already
> > have
> > such case though)
> > 
> > > 3) Get() has a CHECK(s_instance) so that it fails immediately if called
> > outside
> > > of Initialize / Shutdown.
> > 
> > Thanks, I missed this. peter can you add this?
> > 
> > > 
> > > (This is the pattern we use for ash::Shell, DBusThreadManager, and other
> > > necessary singletons).
> > > 
> > > P.S. Looking through this CL, I missed where UserActivityDetector is
> > constructed
> > > / destroyed?
> > 
> > dtr/dtor hasn't changed and that's why you don't see them.
> 
> I see. so Shell still owns it for Ash, but does not provide access to it.
FWIW,
> I find that pattern confusing.

Both has pros and cons. My rule of thumb is that use static methods if you want
to
hide impl and/or the class is large. For smaller objects, it can be ctor/dtor
with
static accessor because such objects tend to be owned by other objects, and can
take
advantage of RAII pattern. There are always exceptions of course.

> 
> WRT 2/3, because shutdown code may need to test for the existence of the
> singleton, you can't do 3 without 2.

Why shutdown code needs this? With the current pattern, you don't need to worry
about
this at least.

> (I prefer IsInitialized() over
> HasInstance() for the reason you gave - we should only use that test in
shutdown
> or startup code where the initialization order ins not gauranteed).

[stevenjb, 2014-11-03 18:56:19]

Now that we are accessing this as a singleton, we can no longer safely make
assumptions about when the instance is destroyed. Shutdown code that can
safely assume that the singleton still exists in Ash may not be a valid
assumption in Athena. e.g. ~KioskModeIdleAppNameNotification() tests
whether or not the singleton exists before accessing it in the destructor,
and rightly so. The order in which KioskModeIdleAppNameNotification()
shouldn't be depended on the destruction order of UserActivityDetector.


On Mon, Nov 3, 2014 at 11:39 AM, <oshima@chromium.org> wrote:

> On 2014/11/03 18:20:31, stevenjb wrote:
>
>> On 2014/11/03 18:16:29, oshima wrote:
>> > On 2014/11/03 16:51:23, stevenjb wrote:
>> > > FWIW, I think this would be more robust if:
>> > >
>> > > 1) UserActivityDetector has an explicit (static) Initialize() and
>>
> Shutdown()
>
>> > > method so that it is obvious where/when it is created and destroyed.
>> > > 2) An explicit HasInstance() or IsInitialized() method is used in
>> destructors
>> > /
>> > > shutdown methods (only) to test the existence of the singleton during
>> shutdown
>> > > where ordering is not certain.
>> >
>> > I don't have strong feeling about these.
>> >
>> > For 1), using ctr/dtor has more enforcement in the ctor/dtor order with
>> > scoped_ptr.
>> > athena uses this approach because we're trying to hide implementation
>> from
>> > public
>> > interface, but it does not apply to this class. (doing so is an option
>>
> though)
>
>> >
>> > For 2), I personal want to avoid it because the such code that checks
>> the
>> > existence
>> > implies some design issue, and should be discouraged. (Unfortunately, we
>> already
>> > have
>> > such case though)
>> >
>> > > 3) Get() has a CHECK(s_instance) so that it fails immediately if
>> called
>> > outside
>> > > of Initialize / Shutdown.
>> >
>> > Thanks, I missed this. peter can you add this?
>> >
>> > >
>> > > (This is the pattern we use for ash::Shell, DBusThreadManager, and
>> other
>> > > necessary singletons).
>> > >
>> > > P.S. Looking through this CL, I missed where UserActivityDetector is
>> > constructed
>> > > / destroyed?
>> >
>> > dtr/dtor hasn't changed and that's why you don't see them.
>>
>
>  I see. so Shell still owns it for Ash, but does not provide access to it.
>>
> FWIW,
>
>> I find that pattern confusing.
>>
>
> Both has pros and cons. My rule of thumb is that use static methods if you
> want
> to
> hide impl and/or the class is large. For smaller objects, it can be
> ctor/dtor
> with
> static accessor because such objects tend to be owned by other objects,
> and can
> take
> advantage of RAII pattern. There are always exceptions of course.
>
>
>  WRT 2/3, because shutdown code may need to test for the existence of the
>> singleton, you can't do 3 without 2.
>>
>
> Why shutdown code needs this? With the current pattern, you don't need to
> worry
> about
> this at least.
>
>
>  (I prefer IsInitialized() over
>> HasInstance() for the reason you gave - we should only use that test in
>>
> shutdown
>
>> or startup code where the initialization order ins not gauranteed).
>>
>
>
>
>
>
> https://codereview.chromium.org/693643004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-11-03 21:17:45]

On Mon, Nov 3, 2014 at 10:56 AM, Steven Bennetts <stevenjb@chromium.org>
wrote:

> Now that we are accessing this as a singleton, we can no longer safely
> make assumptions about when the instance is destroyed.
>
Shutdown code that can safely assume that the singleton still exists in Ash
> may not be a valid assumption in Athena.
>
e.g. ~KioskModeIdleAppNameNotification() tests whether or not the singleton
> exists before accessing it in the destructor, and rightly so. The order in
> which KioskModeIdleAppNameNotification() shouldn't be depended on the
> destruction order of UserActivityDetector.
>

It shouldn't depend on ash destruction either IMHO. I looked at the code
and the Shutdown gets called before CloseAsh, so  I think this check should
be removed. I'll look into others and fix (where possible) in separate CL.


>
> On Mon, Nov 3, 2014 at 11:39 AM, <oshima@chromium.org> wrote:
>
>> On 2014/11/03 18:20:31, stevenjb wrote:
>>
>>> On 2014/11/03 18:16:29, oshima wrote:
>>> > On 2014/11/03 16:51:23, stevenjb wrote:
>>> > > FWIW, I think this would be more robust if:
>>> > >
>>> > > 1) UserActivityDetector has an explicit (static) Initialize() and
>>>
>> Shutdown()
>>
>>> > > method so that it is obvious where/when it is created and destroyed.
>>> > > 2) An explicit HasInstance() or IsInitialized() method is used in
>>> destructors
>>> > /
>>> > > shutdown methods (only) to test the existence of the singleton during
>>> shutdown
>>> > > where ordering is not certain.
>>> >
>>> > I don't have strong feeling about these.
>>> >
>>> > For 1), using ctr/dtor has more enforcement in the ctor/dtor order with
>>> > scoped_ptr.
>>> > athena uses this approach because we're trying to hide implementation
>>> from
>>> > public
>>> > interface, but it does not apply to this class. (doing so is an option
>>>
>> though)
>>
>>> >
>>> > For 2), I personal want to avoid it because the such code that checks
>>> the
>>> > existence
>>> > implies some design issue, and should be discouraged. (Unfortunately,
>>> we
>>> already
>>> > have
>>> > such case though)
>>> >
>>> > > 3) Get() has a CHECK(s_instance) so that it fails immediately if
>>> called
>>> > outside
>>> > > of Initialize / Shutdown.
>>> >
>>> > Thanks, I missed this. peter can you add this?
>>> >
>>> > >
>>> > > (This is the pattern we use for ash::Shell, DBusThreadManager, and
>>> other
>>> > > necessary singletons).
>>> > >
>>> > > P.S. Looking through this CL, I missed where UserActivityDetector is
>>> > constructed
>>> > > / destroyed?
>>> >
>>> > dtr/dtor hasn't changed and that's why you don't see them.
>>>
>>
>>  I see. so Shell still owns it for Ash, but does not provide access to it.
>>>
>> FWIW,
>>
>>> I find that pattern confusing.
>>>
>>
>> Both has pros and cons. My rule of thumb is that use static methods if
>> you want
>> to
>> hide impl and/or the class is large. For smaller objects, it can be
>> ctor/dtor
>> with
>> static accessor because such objects tend to be owned by other objects,
>> and can
>> take
>> advantage of RAII pattern. There are always exceptions of course.
>>
>>
>>  WRT 2/3, because shutdown code may need to test for the existence of the
>>> singleton, you can't do 3 without 2.
>>>
>>
>> Why shutdown code needs this? With the current pattern, you don't need to
>> worry
>> about
>> this at least.
>>
>>
>>  (I prefer IsInitialized() over
>>> HasInstance() for the reason you gave - we should only use that test in
>>>
>> shutdown
>>
>>> or startup code where the initialization order ins not gauranteed).
>>>
>>
>>
>>
>>
>>
>> https://codereview.chromium.org/693643004/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-11-03 21:57:50]

On Mon, Nov 3, 2014 at 10:05 AM,  <oshima@chromium.org> wrote:
> On 2014/10/31 23:02:23, sky wrote:
>>
>> Why do you want to convert to a singleton?
>
>
> Because we need this on both ash and athena, and we want this to be
> accessible
> w/o mega class like ash::Shell
> (or yet another mega class)
>
>> Assuming there is a good reason you
>> should make the constructor/destructor private and use a singleton.
>
>
> We want to avoid singleton because it obfuscate construction/destruction
> order.
> (it gets created when accessed,
> but you can't delete until the end of the world).

If it's not a singleton and you attempt to use some instance that
someone else created how do you know it won't be destroyed?

I think a better model for what you want is to let everyone create one
as needed, but if the implementation needs some shared state let that
be buried in the implementation.

  -Scott

>
>
>
>
>
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
>>
>> File ui/wm/core/user_activity_detector.cc (right):
>
>
>
>
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
>>
>> ui/wm/core/user_activity_detector.cc:17: UserActivityDetector* g_instance
>> =
>> NULL;
>> nullptr
>
>
>
> https://codereview.chromium.org/693643004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-11-03 22:35:13]

On Mon, Nov 3, 2014 at 1:57 PM, Scott Violet <sky@chromium.org> wrote:

> On Mon, Nov 3, 2014 at 10:05 AM,  <oshima@chromium.org> wrote:
> > On 2014/10/31 23:02:23, sky wrote:
> >>
> >> Why do you want to convert to a singleton?
> >
> >
> > Because we need this on both ash and athena, and we want this to be
> > accessible
> > w/o mega class like ash::Shell
> > (or yet another mega class)
> >
> >> Assuming there is a good reason you
> >> should make the constructor/destructor private and use a singleton.
> >
> >
> > We want to avoid singleton because it obfuscate construction/destruction
> > order.
> > (it gets created when accessed,
> > but you can't delete until the end of the world).
>
> If it's not a singleton and you attempt to use some instance that
> someone else created how do you know it won't be destroyed?
>

Sorry I wasn't clear. By singleton, I meant base::Singleton. This is still
technically a singleton as there needs to be only one instance.

The reason why we don't want to use base::Singleton is that it has several
problematic properties that we don't want, like:
* It gets created when someone accessed it even though it shouldn't.
Instead of letting it create, such code should fail.
* It get destroyed by at exit manager, which happens at lot later than it
should.
* Destruction order is undeterministic and can be changed by unrelated
change.

This object has clear life cycle, so I think it's better to have clear
construction/destruction point and catch the error when someone accessed it
when it should not.


> I think a better model for what you want is to let everyone create one
> as needed, but if the implementation needs some shared state let that
> be buried in the implementation.
>
>   -Scott
>
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
> >>
> >> File ui/wm/core/user_activity_detector.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
> >>
> >> ui/wm/core/user_activity_detector.cc:17: UserActivityDetector*
> g_instance
> >> =
> >> NULL;
> >> nullptr
> >
> >
> >
> > https://codereview.chromium.org/693643004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Daniel Erat, 2014-11-03 22:51:45]

there doesn't necessarily need to be just a single instance of
UserActivityDetector, assuming that listening for events is cheap. (we'll run
into this same problem with VideoActivityDetector, which is more heavyweight,
though.)

[oshima, 2014-11-03 23:10:53]

On 2014/11/03 22:51:45, Daniel Erat wrote:
> there doesn't necessarily need to be just a single instance of
> UserActivityDetector, assuming that listening for events is cheap. (we'll run
> into this same problem with VideoActivityDetector, which is more heavyweight,
> though.)

VideoDetector/VideoActivityNotifier (I assume that's what you're referring)
seems to be pretty much self contained and only ash dependency is ShellObserver.
I think it should be easy to remove dependency w/o singleton. I may be missing
something though.

I'm fine with alternative way as long as it doesn't introduce yet another mega
class, nor base::Singleton.
What would you recommend instead?

[Daniel Erat, 2014-11-03 23:13:58]

On 2014/11/03 23:10:53, oshima wrote:
> On 2014/11/03 22:51:45, Daniel Erat wrote:
> > there doesn't necessarily need to be just a single instance of
> > UserActivityDetector, assuming that listening for events is cheap. (we'll
run
> > into this same problem with VideoActivityDetector, which is more
heavyweight,
> > though.)
> 
> VideoDetector/VideoActivityNotifier (I assume that's what you're referring)
> seems to be pretty much self contained and only ash dependency is
ShellObserver.
> I think it should be easy to remove dependency w/o singleton. I may be missing
> something though.

yes, VideoDetector was what i meant.
 
> I'm fine with alternative way as long as it doesn't introduce yet another mega
> class, nor base::Singleton.
> What would you recommend instead?

i thought that scott was suggesting having each observer of UserActivityDetector
instead instantiate its own instance of UserActivityDetector. it'd probably make
sense to get rid of the observer methods if that were done and just support
setting a single activity callback. this would probably be a bit cleaner in some
regards, since different users of the detector have different requirements for
how frequently they should be notified about activity.

[oshima, 2014-11-03 23:26:59]

On 2014/11/03 23:13:58, Daniel Erat wrote:
> On 2014/11/03 23:10:53, oshima wrote:
> > On 2014/11/03 22:51:45, Daniel Erat wrote:
> > > there doesn't necessarily need to be just a single instance of
> > > UserActivityDetector, assuming that listening for events is cheap. (we'll
> run
> > > into this same problem with VideoActivityDetector, which is more
> heavyweight,
> > > though.)
> > 
> > VideoDetector/VideoActivityNotifier (I assume that's what you're referring)
> > seems to be pretty much self contained and only ash dependency is
> ShellObserver.
> > I think it should be easy to remove dependency w/o singleton. I may be
missing
> > something though.
> 
> yes, VideoDetector was what i meant.
>  
> > I'm fine with alternative way as long as it doesn't introduce yet another
mega
> > class, nor base::Singleton.
> > What would you recommend instead?
> 
> i thought that scott was suggesting having each observer of
UserActivityDetector
> instead instantiate its own instance of UserActivityDetector.

If we can do that w/o adding ash dependency, that'd be great.
Peter, can you look into it?

> it'd probably make
> sense to get rid of the observer methods if that were done and just support
> setting a single activity callback. this would probably be a bit cleaner in
some
> regards, since different users of the detector have different requirements for
> how frequently they should be notified about activity.

[pkotwicz, 2014-11-03 23:50:39]

Daniel, I do not see how it would be possible to have multiple
UserActivityDetectors. If we had multiple UserActivityDetectors, each instance
of the UserActivityDetector would need to somehow add itself as a pre target
handler for ash::Shell. (How can it do so without knowing about ash::Shell?)

The order that the pre target handlers of ash::Shell are added matters. In the
past we have had problems due to incorrect ordering.

[Daniel Erat, 2014-11-03 23:55:18]

On 2014/11/03 23:50:39, pkotwicz wrote:
> Daniel, I do not see how it would be possible to have multiple
> UserActivityDetectors. If we had multiple UserActivityDetectors, each instance
> of the UserActivityDetector would need to somehow add itself as a pre target
> handler for ash::Shell. (How can it do so without knowing about ash::Shell?)
> 
> The order that the pre target handlers of ash::Shell are added matters. In the
> past we have had problems due to incorrect ordering.

(if it isn't possible, never mind...)

[sky, 2014-11-04 00:17:01]

On Mon, Nov 3, 2014 at 2:34 PM, Mitsuru Oshima <oshima@google.com> wrote:
>
>
> On Mon, Nov 3, 2014 at 1:57 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> On Mon, Nov 3, 2014 at 10:05 AM,  <oshima@chromium.org> wrote:
>> > On 2014/10/31 23:02:23, sky wrote:
>> >>
>> >> Why do you want to convert to a singleton?
>> >
>> >
>> > Because we need this on both ash and athena, and we want this to be
>> > accessible
>> > w/o mega class like ash::Shell
>> > (or yet another mega class)
>> >
>> >> Assuming there is a good reason you
>> >> should make the constructor/destructor private and use a singleton.
>> >
>> >
>> > We want to avoid singleton because it obfuscate construction/destruction
>> > order.
>> > (it gets created when accessed,
>> > but you can't delete until the end of the world).
>>
>> If it's not a singleton and you attempt to use some instance that
>> someone else created how do you know it won't be destroyed?
>
>
> Sorry I wasn't clear. By singleton, I meant base::Singleton. This is still
> technically a singleton as there needs to be only one instance.
>
> The reason why we don't want to use base::Singleton is that it has several
> problematic properties that we don't want, like:
> * It gets created when someone accessed it even though it shouldn't. Instead
> of letting it create, such code should fail.
> * It get destroyed by at exit manager, which happens at lot later than it
> should.
> * Destruction order is undeterministic and can be changed by unrelated
> change.
>
> This object has clear life cycle, so I think it's better to have clear
> construction/destruction point and catch the error when someone accessed it
> when it should not.

How can you know it has clear life cycle? The constructor/destructor
are public. This is why I don't like what you've ended up with. There
is no guarantee of the lifetime of what ::Get() returns.

Why do we need a singleton here? Why can't each of the places that
needs UAD create one?

  -Scott

>
>>
>> I think a better model for what you want is to let everyone create one
>> as needed, but if the implementation needs some shared state let that
>> be buried in the implementation.
>>
>>   -Scott
>>
>> >
>> >
>> >
>> >
>> >
>> >
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
>> >>
>> >> File ui/wm/core/user_activity_detector.cc (right):
>> >
>> >
>> >
>> >
>> >
https://codereview.chromium.org/693643004/diff/140001/ui/wm/core/user_activit...
>> >>
>> >> ui/wm/core/user_activity_detector.cc:17: UserActivityDetector*
>> >> g_instance
>> >> =
>> >> NULL;
>> >> nullptr
>> >
>> >
>> >
>> > https://codereview.chromium.org/693643004/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2014-11-04 01:42:47]

Scott, I do not think I understand your suggestion. Can you ping me when you are
free to discuss?

[sky, 2014-11-04 03:35:01]

Oshima wanted to discuss tomorrow in person. Will update you then.

  -Scott

On Mon, Nov 3, 2014 at 5:42 PM,  <pkotwicz@chromium.org> wrote:
> Scott, I do not think I understand your suggestion. Can you ping me when you
> are
> free to discuss?
>
>
> https://codereview.chromium.org/693643004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-11-04 22:16:29]

Oshima convinced me: LGTM

[pkotwicz, 2014-11-04 23:01:00]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-11-04 23:02:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693643004/160001

[commit-bot: I haz the power, 2014-11-05 00:19:10]

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

[commit-bot: I haz the power, 2014-11-05 00:19:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2014-11-05 00:51:59]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:53:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693643004/160001

[commit-bot: I haz the power, 2014-11-05 01:30:32]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2014-11-05 01:31:56]

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