Closed Bug 1348738 Opened 7 years ago Closed 7 years ago

Labeling timer callback using nsRepeatService

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

Details

(Whiteboard: [QDL][TDC-MVP][LAYOUT])

Attachments

(5 files)

Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1348738 - (Part 1) Refactor nsRepeatService's InitWithCallback calls into a common helper function.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1348738 - (Part 2) Make nsRepeatService use InitWithFuncCallback instead of InitWithCallback.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1348738 - (Part 3) Add a string-label arg to nsRepeatService::Start and upgrade to InitWithNamedFuncCallback from InitWithFuncCallback.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1348738 - (Part 4) Add a nsIDocument arg to nsRepeatService::Start to get the event target from it.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Depends on: 1348221
Sorry for the delay here -- I've been working my way through these & and am only just getting to this one. :)  I plan to review this tomorrow.
That's okay! Take you time!
No longer depends on: 1348221
Comment on attachment 8849098 [details]
Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.

https://reviewboard.mozilla.org/r/121928/#review126900

::: commit-message-6d38a:1
(Diff revision 2)
> +Bug 1348738 - Labeling timer callback using nsRepeatService. r?dholbert

This patch feels like it's a lot of things at once. Could you split it into several patches instead of one?

Suggested split (though you maybe can come up with another split strategy) -- and note that the first two patches I'm listing here are for *new things* (not currently part of your patch) that I'm suggesting you should do as prerequisites here:
========
 - Part 0: Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr, so that we have stronger guarantees what nsRepeatService::GetInstance() returns to us. (See my note about that below)

 - Part 1: Refactor out the two "InitWithCallback" invocations into a (very simple for now) nsRepeatService helper-function, which just takes the requested delay time as an arg.  (This isn't part of your changes here yet, but I think it's something we should do to reduce duplication, now that the InitWithXYZ invocations are getting more complex.)

 - Part 2: Refactor nsRepeatService to use a callback function instead of being an nsITimerCallback subclass.  This would use "InitWithFuncCallback"  (notably not yet using the "Named" version), which would we swapped into the new nsRepeatService function from part 1.

 - Part 3: add the string-label args to "start", and upgrade to now use the "Named" version of the init function (InitWithNamedFuncCallback)

 - Part 4: Add the nsIDocument member to "start", so that you can use it to call SetTarget.

(Parts 3 and 4 could maybe happen in the same patch, if you prefer.)

::: layout/xul/nsRepeatService.h:13
(Diff revision 2)
>  //
>  #ifndef nsRepeatService_h__
>  #define nsRepeatService_h__
>  
>  #include "nsCOMPtr.h"
> +#include "nsIDocument.h"

Please move this #include from the .h file to the .cpp file, and just use a forward-declaration in this .cpp file.   (i.e. add "class nsIDocument;", just above "class nsITimer" a few lines down from this)

::: layout/xul/nsRepeatService.h:33
(Diff revision 2)
>      
> -  NS_DECL_NSITIMERCALLBACK
> -

While you're getting rid of lines here, please also get rid of this whitespace-on-a-blank-line (on the line right after "typedef").  It's highlighted in red in MozReview.

::: layout/xul/nsRepeatService.h:34
(Diff revision 2)
>    // Start dispatching timer events to the callback. There is no memory
>    // management of aData here; it is the caller's responsibility to call
>    // Stop() before aData's memory is released.
> -  void Start(Callback aCallback, void* aData,
> -             uint32_t aInitialDelay = INITAL_REPEAT_DELAY);
> +  void Start(Callback aCallback, void* aData, nsIDocument* aDoc,
> +             const char* aName, uint32_t aInitialDelay = INITAL_REPEAT_DELAY);

Please add a sentence or two at the end of this function's documentation, to hint at what |aDoc| and |aName| are used for.  (And also to make it clear that we don't hold onto aDoc as part of the data needed for the repeating callback.)

e.g. "aDoc is only used when starting the timer, to determine [...]"

::: layout/xul/nsRepeatService.h:44
(Diff revision 2)
> +  static void Notify(nsITimer *timer, void* aClosure);
> +

Two things about the timer param here:
 (1) shift "*" to the left
 (2) give it an "a" prefix, "aTimer", to match gecko coding style

::: layout/xul/nsRepeatService.cpp:15
(Diff revision 2)
>  // See documentation in associated header file
>  //
>  
>  #include "nsRepeatService.h"
>  #include "nsIServiceManager.h"
> +using namespace mozilla;

Insert a blank line between these #includes and the new "using" line, please

::: layout/xul/nsRepeatService.cpp:60
(Diff revision 2)
> +    mRepeatTimer->InitWithNamedFuncCallback(Notify,
> +                                            nullptr,
> +                                            aInitialDelay,
> +                                            nsITimer::TYPE_ONE_SHOT,
> +                                            mCallbackName.Data());

As noted in my header-comment for this patch, we should refactor this InitWithXYZ invocation into a helper function, since it happens in two separate places with basically the same args (aside from the delay, which can be an arg to the helper function).

::: layout/xul/nsRepeatService.cpp:85
(Diff revision 2)
> +  auto rs = nsRepeatService::GetInstance();
> +
>    // do callback
> -  if (mCallback)
> +  if (rs->mCallback) {

(This is an elaboration on what I'm suggesting as "part 0" in my header comment for this patch):

As with the other layout/xul labeling bug, there's an important question here: so this nsRepeatService instance was alive *when the timer was set up*, but how do we know it's still alive now? (How do we know nsRepeatService::GetInstance isn't giving us a pointer to something that's been deleted?)

With the patch right now, it's hard to be sure.  If nsRepeatService::Shutdown has been called and then this Notify() function gets invoked afterwards, then "rs = nsRepeatService::GetInstance()" will produce a pointer to deleted memory. And that's potentially exploitable.

(Before your changes here, the nsRepeatService object would be kept alive via the refcounted nsITimerCallback pointer that the nsITimer holds onto. But now that you're using a function callback, the timer doesn't keep an owning reference.)

Anyway -- to clear up these fears, please add a patch (called "part 0" in my suggested ordering) to ensure that nsRepeatService::Shutdown() *nulls out its copy of the pointer* after it potentially-deletes gRepeatService by decrementing its refcount. The best way to do this is by ripping out the old-crufty-raw-pointer-with-manual-ADDREF/RELEASE and using a StaticRefPtr instead -- then, the Shutdown() function should just assign that StaticRefPtr to null (to decrement the refcount & clear our pointer all in one step).  And while you're doing this, you could make gRepeatService a file-scoped static variable, too -- there's no reason it needs to be a static class variable, as far as I can tell.  (Might as well make the .h file more lightweight.)

See https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/dom/base/TabGroup.cpp#24 for sample usage of a StaticRefPtr. That file happens to use a fancy "ClearOnShutdown" mechanism, too, but I don't think you need to bother with that -- that would be an unrelated & perhaps-unnecessary separate refactoring.  For now, the existing Shutdown() function seems fine as long as we make it actually null out the pointer.

And then, once you've done all this, your new code that I'm commenting on here (with "auto rs = nsRepeatService::GetInstance()") should *null-check* rs before using it, and return early if "rs" is null. (That should happen as part of whatever patch adds this GetInstance() call -- which I think is "part 2" in my suggested patch splitting strategy.)
Attachment #8849098 - Flags: review?(dholbert) → review-
Comment on attachment 8849098 [details]
Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.

https://reviewboard.mozilla.org/r/121928/#review126942

Two more nits:

::: layout/xul/nsRepeatService.cpp:45
(Diff revision 2)
>  void nsRepeatService::Start(Callback aCallback, void* aCallbackData,
> +                            nsIDocument* aDoc, const char* aName,
>                              uint32_t aInitialDelay)
>  {
>    NS_PRECONDITION(aCallback != nullptr, "null ptr");
>  
>    mCallback = aCallback;
>    mCallbackData = aCallbackData;
> +  mCallbackName.Assign(aName);

The char*-buffer-handling here is a little too haphazard. In particular:
We don't have any guarantees on the lifetime of the character buffer that's passed in. (It could be a string literal which lasts forever, OR it could be a stack-allocated local char[] array in the caller which is going away right after we return).  I don't recall offhand what this Assign(char*) method actually does under the hood with the passed-in char*, but it's got to be one of two things and either one is bad:
   (a) it might just save a pointer to the given char* array (which is bad because who knows if those characters will still be alive when we actually use them later on inside of Notify! Right now they will because they happen to always be string literals, but there's no enforcement/documentation of that requirement.)
...or:
   (b) it might make its own private copy of the character array, since it recognizes it can't assume anything about lifetimes. That's bad because it's doing string-copying that we know is actually unnecessary. (because in practice these are always string literals, so we'd prefer pointer-sharing if we can prove it's safe)

SO: really, you should instead make this function (nsRepeatService::Start()) take "const nsACString&" (abstract C-string) as its name argument, and the callsites should pass in NS_LITERAL_CSTRING("whatever") for that arg.  And then you can just do "mCallbackName = aName", and I believe string sharing should work out OK. (since I'm pretty sure the nACString that NS_LITERAL_CSTRING() produces can tell that its memory will stick around forever, so operator= will just do pointer-sharing without needing to do any needless string-copying.)

This change should all should happen up-front in whichever patch adds the "aName" arg, which in my suggested patch-splitting-strategy is "part 3".

BTW, https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings is a really good guide on our string types, if you haven't run across it yet.  Good thing to have bookmarked -- I refer to it every time I have to think about strings.

::: layout/xul/nsRepeatService.cpp:83
(Diff revision 2)
> +nsRepeatService::Notify(nsITimer *timer, void* aClosure)
>  {

Two things about this function's naming/signature:
 (1) it feels wrong that it's a public function that anyone who #includes nsRepeatService could just directly call into.  We're only expecting it to be called from the timer (via our InitWithFuncCallback invocation).  So we should probably make it a file-scoped static function, not a class function.

 (2) "Notify" might be a confusing name, since it makes it sound like we're still implementing nsITimerCallback (and its Notify API).  Maybe rename this to "TriggerCallback", since that's what it does? (it invokes mCallback)

These changes would probably want to happen as part of my suggested Part 2 (where we convert from nsITimerCallback to using InitWithFuncCallback)
About the `Notify` function, actually, I want to remove it. I want to use `NewNamedTimerCallback` I implemented in Bug 1348221 instead of Notify function. 

The advantages of `NewNamedTimerCallback`:

- Store the args `aName` in the lambda function instead of using the member variable `mCallbackName`.
- No need the Notify function.

What do you think about this?

But, `NewNamedTimerCallback` is put in `APZThreadUtils.h`. I am trying to move it to XPCOM (see Bug 1205480). Or, do you think I can include `APZThreadUtils.h` directly to do this?
Flags: needinfo?(dholbert)
I'd suggest filing a followup for that (perhaps depending on this bug & bug 1205480).

For now, I'm a bit uncomfortable adding non-APZ-related clients of APZThreadUtils.h.
Flags: needinfo?(dholbert)
About comment 8, I found the copy constructor of nsACString is protected, and I can't capture it in lambda function. So, I'll still use `InitWithNamedCallbackFunc`.
Comment on attachment 8849098 [details]
Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.

https://reviewboard.mozilla.org/r/121928/#review126900

> (This is an elaboration on what I'm suggesting as "part 0" in my header comment for this patch):
> 
> As with the other layout/xul labeling bug, there's an important question here: so this nsRepeatService instance was alive *when the timer was set up*, but how do we know it's still alive now? (How do we know nsRepeatService::GetInstance isn't giving us a pointer to something that's been deleted?)
> 
> With the patch right now, it's hard to be sure.  If nsRepeatService::Shutdown has been called and then this Notify() function gets invoked afterwards, then "rs = nsRepeatService::GetInstance()" will produce a pointer to deleted memory. And that's potentially exploitable.
> 
> (Before your changes here, the nsRepeatService object would be kept alive via the refcounted nsITimerCallback pointer that the nsITimer holds onto. But now that you're using a function callback, the timer doesn't keep an owning reference.)
> 
> Anyway -- to clear up these fears, please add a patch (called "part 0" in my suggested ordering) to ensure that nsRepeatService::Shutdown() *nulls out its copy of the pointer* after it potentially-deletes gRepeatService by decrementing its refcount. The best way to do this is by ripping out the old-crufty-raw-pointer-with-manual-ADDREF/RELEASE and using a StaticRefPtr instead -- then, the Shutdown() function should just assign that StaticRefPtr to null (to decrement the refcount & clear our pointer all in one step).  And while you're doing this, you could make gRepeatService a file-scoped static variable, too -- there's no reason it needs to be a static class variable, as far as I can tell.  (Might as well make the .h file more lightweight.)
> 
> See https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/dom/base/TabGroup.cpp#24 for sample usage of a StaticRefPtr. That file happens to use a fancy "ClearOnShutdown" mechanism, too, but I don't think you need to bother with that -- that would be an unrelated & perhaps-unnecessary separate refactoring.  For now, the existing Shutdown() function seems fine as long as we make it actually null out the pointer.
> 
> And then, once you've done all this, your new code that I'm commenting on here (with "auto rs = nsRepeatService::GetInstance()") should *null-check* rs before using it, and return early if "rs" is null. (That should happen as part of whatever patch adds this GetInstance() call -- which I think is "part 2" in my suggested patch splitting strategy.)

Please see the part 0 in new patches. Did you mean that?

And I have a question about this. In original implementation, `nsRepeatService::gInstance` would be release only in `nsRepeatService::Shutdown` and it would be set to 0 in the macro `NS_IF_RESEASE`. If it is 0, I think the next time `nsRepeatService::GetInstance` called, the new `nsRepeatService` will be created. So, I didn't understand what the scenario of dangling pointer happend. But I agree that using StaticRefPtr (smart pointer) would be safer.
Comment on attachment 8849098 [details]
Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.

https://reviewboard.mozilla.org/r/121928/#review126942

> Two things about this function's naming/signature:
>  (1) it feels wrong that it's a public function that anyone who #includes nsRepeatService could just directly call into.  We're only expecting it to be called from the timer (via our InitWithFuncCallback invocation).  So we should probably make it a file-scoped static function, not a class function.
> 
>  (2) "Notify" might be a confusing name, since it makes it sound like we're still implementing nsITimerCallback (and its Notify API).  Maybe rename this to "TriggerCallback", since that's what it does? (it invokes mCallback)
> 
> These changes would probably want to happen as part of my suggested Part 2 (where we convert from nsITimerCallback to using InitWithFuncCallback)

After created the helper function to call `InitWithNamedCallbackFunc`, I use lambda function in that function. I think we no longer need the `Notify` (`TriggerCallback`) function.
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #10)
> About comment 8, I found the copy constructor of cnsACString is protected,
> and I can't capture it in lambda function. So, I'll still use
> `InitWithNamedCallbackFunc`.

If you want to create a copy of a nsACString, you'd want to declare the copy with type "nsCString" (or "nsCAutoString" if it's a stack variable).

You can't declare a variable with concrete type nsACString.  That's an abstract class, and it only exists as a superclass, to be used as a reference or a pointer to an actual concrete instance.  (So you should only ever see nsACString as "nsACString&" or "nsACString*", or via its concrete subclasses like nsCString & nsCAutoString)
> or "nsCAutoString" if it's a stack variable).

Sorry, I meant nsAutoCString. (I forgot the C & Auto prefix ordering)
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #16)
> Comment on attachment 8849098 [details]
> Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.
> 
> https://reviewboard.mozilla.org/r/121928/#review126900
[...]
> And I have a question about this. In original implementation,
> `nsRepeatService::gInstance` would be release only in
> `nsRepeatService::Shutdown` and it would be set to 0 in the macro
> `NS_IF_RESEASE`.

Oh, I'd forgotten that NS_IF_RELEASE sets the pointer to 0 in the deletion case!  That's nice.  That macro (& its friends) are old and crufty, and I'd forgotten that they had that feature. :)

So you're right that there wasn't actually a dangling pointer issue here.  But, good to upgrade from old crufty macros to modern RAII helper-structs, anyway. :)

> If it is 0, I think the next time
> `nsRepeatService::GetInstance` called, the new `nsRepeatService` will be
> created.

This is actually a good point, too, and it means we probably *don't* want our Notify function to use nsRepeatService::GetInstance after all! (We don't want Notify() to trigger the *creation of a new nsRepeatService instance* after Shutdown() has been called.  Maybe we can't even get callbacks that late, but I'm not sure.)

So: inside of your new Notify implementation (and its new incarnation), we should just directly use gRepeatService (now sRepeatService) there, and null-check it before using it... anyway, I'll leave this as a comment on that patch, too.
Comment on attachment 8849098 [details]
Bug 1348738 - (Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr.

https://reviewboard.mozilla.org/r/121928/#review127230

r=me

::: layout/xul/nsRepeatService.cpp:17
(Diff revision 4)
> +
> +static StaticRefPtr<nsRepeatService> sRepeatService;

(I'm realizing that this should eventually be StaticAutoPtr, since we won't need refcounting at all once we stop inheriting from nsITimerCallback in part 2.

But, no need to change anything here -- I just think we should tweak this pointer in Part 2. I'll make a further note there.)
Attachment #8849098 - Flags: review?(dholbert) → review+
Comment on attachment 8852391 [details]
Bug 1348738 - (Part 1) Refactor nsRepeatService's InitWithCallback calls into a common helper function.

https://reviewboard.mozilla.org/r/124636/#review127232

r=me with 2 nits:

::: commit-message-34f56:1
(Diff revision 2)
> +Bug 1348738 - (Part 1) Create a helper function with one arg delay time to call InitWithCallback. r?dholbert

Please reword this to something like:

Bug 1348738 - (Part 1) Refactor nsRepeatService's InitWithCallback calls into a common helper function.

This reformulation makes it clearer (to me at least) that this patch isn't changing behavior at all -- it's just moving code around.  (And the "one arg delay time" isn't really significant enough that we need to bother mentioning in the commit message, IMO -- the high-level point here is that we're just refactoring similar code into a helper function to better share code, so we should just say that & hint at which code is the code that's being refactored.)

::: layout/xul/nsRepeatService.h:52
(Diff revision 2)
>  private:
>    Callback           mCallback;
>    void*              mCallbackData;
>    nsCOMPtr<nsITimer> mRepeatTimer;
>  
> +  // helper function to initialize callback function to mRepeatTimer
> +  void InitTimerCallback(uint32_t aInitialDelay);
> +

Please insert this new decl a few lines higher -- between "private" and the member-variables.
Attachment #8852391 - Flags: review?(dholbert) → review+
Comment on attachment 8852392 [details]
Bug 1348738 - (Part 2) Make nsRepeatService use InitWithFuncCallback instead of InitWithCallback.

https://reviewboard.mozilla.org/r/124638/#review127236

::: commit-message-54f93:1
(Diff revision 2)
> +Bug 1348738 - (Part 2) Use InitWithFuncCallback instead of InitWithCallback. r?dholbert

s/Use/Make nsRepeatService use/

::: commit-message-54f93:3
(Diff revision 2)
> +Remove the inheritance from nsITimerCallback for nsRepeatService.
> +nsRepeatService is no need to inherit nsITimerCallback when using
> +InitWithFuncCallback.

The wording here is a little funky (and the connection to the first line of the commit message isn't immediately obvious).

How about replace this paragraph with the following (and see the other issue I'm filing for an explanation of the "singly-owned" bit):

"This patch also makes nsRepeatService stop inheriting from nsITimerCallback. We needed that inheritance for InitWithCallback, but we do not need it for InitWithFuncCallback.

This patch also makes nsRepeatService singly-owned instead of being refcounted, since we're left with only a single owning pointer."

::: layout/xul/nsRepeatService.h:29
(Diff revision 2)
> +  NS_INLINE_DECL_REFCOUNTING(nsRepeatService)
> +

I think this object doesn't need to be refcounted anymore, as of this patch.  It's now singly-owned by sRepeatService, so sRepeatService can just be a non-refcounted smart pointer.  So how about:

 (1) Don't introduce this refcounting macro here -- we'll just let the refcounting methods disappear with "public nsITimerCallback" going away.

 (2) Convert sRepeatService to have type StaticAutoPtr instead of StaticRefPtr (and add #include "mozilla/StaticPtr.h" to nsRepeatService.cpp to provide that type)

 (3) Move the ~nsRepeatService() decl from the "protected" section up to the "public" section.  (because now we'll be expecting to call "delete" from external code -- the StaticAutoPtr destructor -- instead of from inside our own Release method).
Attachment #8852392 - Flags: review?(dholbert) → review-
Comment on attachment 8852393 [details]
Bug 1348738 - (Part 3) Add a string-label arg to nsRepeatService::Start and upgrade to InitWithNamedFuncCallback from InitWithFuncCallback.

https://reviewboard.mozilla.org/r/124640/#review127248

r=me with commit message tweaked slightly:

::: commit-message-e451d:1
(Diff revision 2)
> +Bug 1348738 - (Part 3) Add the string-label arg to "Start" and upgrade to InitWithNamedFuncCallback from InitWithFuncCallback. r?dholbert

s/Add the/Add a/

s/Start/nsRepeatService::Start/
Attachment #8852393 - Flags: review?(dholbert) → review+
Comment on attachment 8852394 [details]
Bug 1348738 - (Part 4) Add a nsIDocument arg to nsRepeatService::Start to get the event target from it.

https://reviewboard.mozilla.org/r/124642/#review127314

This looks good (with some comment nits), though I'm not sure we're using the right TaskCategory here. See below.

::: commit-message-052a8:1
(Diff revision 2)
> +Bug 1348738 - (Part 4) Add the nsIDocument arg to "Start" to get the event target from it. r?dholbert

As for the previous part:
 s/Add the/Add a/
 s/Start/nsRepeatService::Start/

::: layout/xul/nsRepeatService.h:41
(Diff revision 2)
>    // Stop() before aData's memory is released.
>    //
>    // aCallbackName is the label of the callback, used to pass to
>    // InitWithNamedCallbackFunc.
> +  //
> +  // aDocument is used to get the event target in Start(). We need a event

s/a event/an event/

::: layout/xul/nsRepeatService.cpp:61
(Diff revision 2)
>  
>    nsresult rv;
>    mRepeatTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
>  
>    if (NS_SUCCEEDED(rv))  {
> +    mRepeatTimer->SetTarget(aDocument->EventTargetFor(TaskCategory::Other));

Do you know if "::Other" is actually the right category for these?

The clients of this code seem to all be things that are responding to user interaction (related to scrollbars/scrollbuttons, it seems).  Could you check what they're actually doing, and see whether that's the sort of thing that better fits TaskCategory::UI vs. TaskCategory::Other?

(per documentation at https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TaskCategory.h#14 )

billm is probably the authority on this, if it's unclear after you've investigated a bit.
(In reply to Daniel Holbert [:dholbert] from comment #30)
> ::: layout/xul/nsRepeatService.cpp:61
> (Diff revision 2)
> >  
> >    nsresult rv;
> >    mRepeatTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> >  
> >    if (NS_SUCCEEDED(rv))  {
> > +    mRepeatTimer->SetTarget(aDocument->EventTargetFor(TaskCategory::Other));
> 
> Do you know if "::Other" is actually the right category for these?
> 
> The clients of this code seem to all be things that are responding to user
> interaction (related to scrollbars/scrollbuttons, it seems).  Could you
> check what they're actually doing, and see whether that's the sort of thing
> that better fits TaskCategory::UI vs. TaskCategory::Other?
> 
> (per documentation at
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TaskCategory.
> h#14 )
> 
> billm is probably the authority on this, if it's unclear after you've
> investigated a bit.

Hmm... I think we should use TaskCategory::UI here. But I'm not sure this callback in [1].

Hi Bill, for the case in [1], which category should we use here?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#4110
Flags: needinfo?(wmccloskey)
TaskCategory::UI is really reserved for mouse and keyboard events that come in directly over IPC. I don't think we want to use it here. ::Other sounds right to me.
Flags: needinfo?(wmccloskey)
Comment on attachment 8852394 [details]
Bug 1348738 - (Part 4) Add a nsIDocument arg to nsRepeatService::Start to get the event target from it.

https://reviewboard.mozilla.org/r/124642/#review128774

Thanks Bill! This has r=me then, with the other nits in comment 30 addressed.
Attachment #8852394 - Flags: review?(dholbert) → review+
Comment on attachment 8852392 [details]
Bug 1348738 - (Part 2) Make nsRepeatService use InitWithFuncCallback instead of InitWithCallback.

https://reviewboard.mozilla.org/r/124638/#review129292

r=me with nits addressed:

::: commit-message-54f93:8
(Diff revision 3)
> +This patch makes nsRepeatService stop inheriting from nsITimerCallback.
> +We needed that inheritance for InitWithCallback, but we do not need it
> +for InitWithFuncCallback.
> +
> +This patch also makes nsRepeatService singly-owned instead of being
> +refcounted, since we're left with only a single owning pointer."

Drop the stray quote character here.

::: layout/xul/nsRepeatService.cpp:12
(Diff revision 3)
>  
> +#include "mozilla/StaticPtr.h"
>  #include "nsRepeatService.h"
>  #include "nsIServiceManager.h"
>  

I'd like nsRepeatService.h to remain the first #include here. We have a convention that Foo.cpp should always include Foo.h first.  This gives us some assurance that Foo.h itself has all of the #includes, namespaces, and forward-declarations that it needs.  (This is kind of a sham in light of unified builds, but it still sort of helps.)

So: please insert this new #include 1 line lower, *after* nsRepeatService.h.

::: layout/xul/nsRepeatService.cpp:82
(Diff revision 3)
> +  mRepeatTimer->InitWithFuncCallback([](nsITimer* aTimer, void* aClosure) {
> +    // Use gRepeatService instead of nsRepeatService::GetInstance(). We should
> +    // not re-create a new nsRepeatService after nsRepeatService::Shutdown() is
> +    // called.
> +    nsRepeatService* rs = gRepeatService;
> +    if (!rs) {

Nit: right now, it's not immediately clear that there's a connection between the two sentences in the comment.  (It sounds kinda like it's just stating two independent facts -- first about which thing we're using, and second that we don't want to recreate something.) 

Please rephrase to make the connection clearer. Maybe like so:
    // Use gRepeatService instead of nsRepeatService::GetInstance() (because
    // we don't want GetInstance() to re-create a new instance for us, if we
    // happen to get invoked after ::Shutdown has nulled out gRepeatService).
Attachment #8852392 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/45556e7c9cf6
(Part 0) Convert nsRepeatService::gInstance to be a file-scoped StaticRefPtr. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6d7693423a59
(Part 1) Refactor nsRepeatService's InitWithCallback calls into a common helper function. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/10c80ad4d4fa
(Part 2) Make nsRepeatService use InitWithFuncCallback instead of InitWithCallback. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2f0275864ecf
(Part 3) Add a string-label arg to nsRepeatService::Start and upgrade to InitWithNamedFuncCallback from InitWithFuncCallback. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c1ae2c3b3905
(Part 4) Add a nsIDocument arg to nsRepeatService::Start to get the event target from it. r=dholbert
Keywords: checkin-needed
Status: NEW → ASSIGNED
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: