Timer gUserInteractionTimerCallback in EventStateManager.cpp

Comment on attachment 8849880 [details] [diff] [review] Label timer in dom/events Review of attachment 8849880 [details] [diff] [review]: ----------------------------------------------------------------- Per offline discussion, the gUserInteractionTimer is a singleton to serve multiple documents/windows. Hence, some design change is expected to call the timercallback from different event target. In addition, if we keep gUserInteractionTimer as a singleton, we should make sure that the timer was cancel before we init another run of timercallback with different event target to prevent that the previous callback was invoked with the event target we set later for next InitWithCallback. ::: dom/events/EventStateManager.cpp @@ +341,5 @@ > + return; > + if (!gUserInteractionTimer && mPresContext) { > + EnsureDocument(mPresContext); > + CallCreateInstance("@mozilla.org/timer;1", &gUserInteractionTimer); > + nsCOMPtr<nsPIDOMWindowOuter> win(mDocument->GetWindow()); You can get DocGroup EventTarget via mDocument->EventTargetFor() without getting TabGroup from its outter window. @@ +1432,5 @@ > + EnsureDocument(mPresContext); > + nsCOMPtr<nsPIDOMWindowOuter> win(mDocument->GetWindow()); > + NS_ENSURE_TRUE_VOID(win); > + mClickHoldTimer->SetTarget( > + win->TabGroup()->EventTargetFor(TaskCategory::UI)); ditto

Comment on attachment 8850260 [details] [diff] [review] Label timer in dom/events (V2) Maybe we should use SystemGroup for UserInteractionTimer since it's not bound to a specific document.

Comment on attachment 8850260 [details] [diff] [review] Label timer in dom/events (V2) Review of attachment 8850260 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. Thanks! ::: dom/events/EventStateManager.cpp @@ +343,4 @@ > CallCreateInstance("@mozilla.org/timer;1", &gUserInteractionTimer); > + if (gUserInteractionTimer) { > + gUserInteractionTimer->SetTarget( > + SystemGroup::EventTargetFor(TaskCategory::Other)); We have to use SystemGroup carefully to prevent an assertion similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1339006#c11 The notification of user-interaction-active/user-interaction-inactive will trigger a JS callback in SessionRecorder: http://searchfox.org/mozilla-central/source/toolkit/modules/SessionRecorder.jsm#390-396 Comprehensive test is required to ensure that SessionRecorder and its user are chrome-privileged to prevent hitting this assertion.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5) > Comment on attachment 8850260 [details] [diff] [review] > Label timer in dom/events (V2) > > Review of attachment 8850260 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please see my comment below. > > Thanks! > > ::: dom/events/EventStateManager.cpp > @@ +343,4 @@ > > CallCreateInstance("@mozilla.org/timer;1", &gUserInteractionTimer); > > + if (gUserInteractionTimer) { > > + gUserInteractionTimer->SetTarget( > > + SystemGroup::EventTargetFor(TaskCategory::Other)); > > We have to use SystemGroup carefully to prevent an assertion similar to > https://bugzilla.mozilla.org/show_bug.cgi?id=1339006#c11 > > The notification of user-interaction-active/user-interaction-inactive will > trigger a JS callback in SessionRecorder: > http://searchfox.org/mozilla-central/source/toolkit/modules/SessionRecorder. > jsm#390-396 > > Comprehensive test is required to ensure that SessionRecorder and its user > are chrome-privileged to prevent hitting this assertion. Tested with my local build and it works fine.

Comment on attachment 8850260 [details] [diff] [review] Label timer in dom/events (V2) >+ EnsureDocument(mPresContext); Technically, what guarantees mPresContext is non-null here? >+ mClickHoldTimer->SetTarget(mDocument->EventTargetFor(TaskCategory::UI)); > mClickHoldTimer->InitWithFuncCallback(sClickHoldCallback, this, > clickHoldDelay, > nsITimer::TYPE_ONE_SHOT); Given that this is really about a system level thing, not so much about some particular document, I would just use SystemGroup::EventTargetFor(TaskCategory::Other) (we don't want this kind of timer to be delayed if the docgroup is preempted temporarily) That changed, r+ (also, I think CreateClickHoldTimer code is effectively dead. There is even a bug to remove this method)

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c01b9f972f5a Label timer in dom/events. f=bevistseng. r=smaug.