Closed Bug 1342863 Opened 3 years ago Closed 3 years ago

Label runnables in layout/base/

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: TYLin, Assigned: kuoe0.tw)

References

Details

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

User Story

See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.

Attachments

(8 files)

59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
We do not need to label AccessibleCaretManager::mCaretTimeoutTimer because it is disabled on all platform and is going to be removed in Bug 1347047. 

http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/AccessibleCaretManager.cpp#1415-1416
Assignee: nobody → kuoe0
Status: NEW → ASSIGNED
vsync runnables in layout/base would be handled in bug 1333962.
Blocks: 1347815
No longer blocks: 1347815
Hi Bevis, could you take a look at the patches (Part 6 ~ Part 8)? Is that the right way to label a timer callback? And I have no idea about how to make the `getName()` function works in the patch (Part 8).
Flags: needinfo?(btseng)
1. Yes, calling nsITimer->SetTarget() after do_CreateInstance(NS_TIMER_CONTRACTID) replaces the EventTarget that was set with NS_GetCurrentThread() internally in nsTimerImpl Constructor with the one you provided.
2. You don't have to make PaintTimerCallBack inherit nsINamed but replace the call of InitWithFuncCallback to InitWithNamedFuncCallback(..., "PaintTimerCallBack").
3. I see no reason why you need to make PresShell/nsPresContext inherit nsINamed. InitWithNamedFuncCallback shall cover what you need to name the runnables of the TimerCallback.

nit:
you can setEventTarget just in 1 line by:
> mDelayedPaintTimer->SetTarget(mDocument->EventTargetFor(TaskCategory::Other));
It seems not possible that the eventTarget will be released during these operation.
Flags: needinfo?(btseng)
PaintTimerCallBack is not type `nsTimerCallbackFunc`. I think we can't use `InitWithNamedFuncCallback()` instead of `InitWithCallback()`. How to make the derived classes of `nsITimerCallback` own their name?

There is also the following code in nsPresContext.

```
nsresult rv = timer->InitWithCallback(NewTimerCallback([self, aTransactionId](){
  nsAutoScriptBlocker blockScripts;
  self->NotifyDidPaintForSubtree(aTransactionId);
}), nullptr, 100, nsITimer::TYPE_ONE_SHOT, "NotifyDidPaintForSubtree");
```

The object created by `NewTimerCallback()` is also type `nsITimerCallback`. So, I also can't use it with `InitWithNamedFuncCallback()`. Is there any way to convert type `nsITimerCallback` to `nsTimerCallbackFunc`?
Flags: needinfo?(btseng)
m... Let me check if I can overload InitWithCallback with a name parameter or define another one call InitWithNamedCallback tomorrow.
Keep NI on me.
After looking into nsTimerImpl::GetName(), for the timer initialized with InitWithCallback()/Init(), the only way to name the timer is to inherit nsINamed as you have done.

For NewTimerCallback(), you need to change GenericTimerCallbackBase by inheriting nsINamed and implementing it similar to what has been done in the Runnable class:
http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/threads/nsThreadUtils.h#254
Flags: needinfo?(btseng)
(In reply to Tommy Kuo [:KuoE0] from comment #30)
> Is there any way to convert type `nsITimerCallback` to `nsTimerCallbackFunc`?

Is is possible to rewrite PaintTimerCallBack and NewTimerCallback to use nsTimerCallbackFunc so that we could use InitWithNamedFuncCallback() with the timer?
Depends on: 1348221
The function `NewNamedTimerCallback()` is implemented in Bug 1348221.
Comment on attachment 8847463 [details]
Bug 1342863 - (Part 1) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in PresShell.

https://reviewboard.mozilla.org/r/120440/#review124708

Hi Tommy! Sorry for the delay here. This can be simplified a little using "do_AddRef", as noted below in the code.

Also, there seems to be a build error in a patch of yours that you layered this patch stack on top of right now. If I use MozReview's suggested command to pull this patch into my local tree & then try to build...
hg pull -r b385fa368948ac39c0c56f9a016a28422ef12874 https://reviewboard-hg.mozilla.org/gecko
...I get this build error:
> layout/base/nsPresContext.cpp:3275:43: error: use of undeclared identifier 'NewTimerCallback'
>     nsresult rv = timer->InitWithCallback(NewTimerCallback([self, aTransactionId](){
>                                           ^

It looks like that's because these commits are layered on top of an incomplete patch for bug 1348221 (which deletes NewTimerCallback but doesn't delete this particular instance in nsPresContext.cpp).

Would you mind rebasing this patch stack on top of a cset from current trunk?  That'll help me sanity-check my review feedback locally (before I post it) by letting me pull & try to build with some of my potential suggested tweaks.

::: layout/base/PresShell.cpp:2013
(Diff revision 5)
> -      RefPtr<nsRunnableMethod<PresShell> > resizeEvent =
> -        NewRunnableMethod("PresShell::FireResizeEvent",
> -                          this, &PresShell::FireResizeEvent);
> -      if (NS_SUCCEEDED(NS_DispatchToCurrentThread(resizeEvent))) {
> -        mResizeEvent = resizeEvent;
> +      RefPtr<nsRunnableMethod<PresShell>> event =
> +        NewRunnableMethod(this, &PresShell::FireResizeEvent);
> +      RefPtr<nsRunnableMethod<PresShell>> eventKeeper(event);
> +      nsresult rv = GetDocument()->Dispatch("PresShell::FireResizeEvent",
> +                                            TaskCategory::Other,
> +                                            event.forget());
> +      if (NS_SUCCEEDED(rv)) {
> +        mResizeEvent = eventKeeper;

Instead of introducing a second RefPtr local variable here ("eventKeeper"), you should just pass in "do_AddRef(event)".

That'll increment the refcount into a temporary already_AddRefed (so you don't need to call "forget"), and it'll let "event" stick around as an owning pointer.

::: layout/base/PresShell.cpp:6258
(Diff revision 5)
> -  RefPtr<nsRunnableMethod<PresShell> > ev =
> -    NewRunnableMethod("PresShell::UpdateApproximateFrameVisibility",
> -                      this, &PresShell::UpdateApproximateFrameVisibility);
> -  if (NS_SUCCEEDED(NS_DispatchToCurrentThread(ev))) {
> -    mUpdateApproximateFrameVisibilityEvent = ev;
> +  RefPtr<nsRunnableMethod<PresShell>> event =
> +    NewRunnableMethod(this, &PresShell::UpdateApproximateFrameVisibility);
> +  RefPtr<nsRunnableMethod<PresShell>> eventKeeper(event);
> +  nsresult rv =
> +    GetDocument()->Dispatch("PresShell::UpdateApproximateFrameVisibility",

Same here.
Attachment #8847463 - Flags: review?(dholbert) → review-
Comment on attachment 8847464 [details]
Bug 1342863 - (Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext.

https://reviewboard.mozilla.org/r/120442/#review124710

::: layout/base/nsPresContext.cpp:3335
(Diff revision 5)
> -    mWillPaintFallbackEvent = new RunWillPaintObservers(this);
> -    NS_DispatchToMainThread(mWillPaintFallbackEvent.get());
> +    RefPtr<RunWillPaintObservers> event = new RunWillPaintObservers(this);
> +    mWillPaintFallbackEvent = event;
> +    Document()->Dispatch("RunWillPaintObservers",
> +                         TaskCategory::Other,
> +                         event.forget());

As with part 1, it'd be a bit simpler to use do_AddRef here (rather than creating an extra local variable here, beyond mWillPaintFallbackEvent which already stores the same thing).

I think you can just restore the original "mWillPaintFallbackEvent = new (...)" line, and pass do_AddRef(mWillPaintFallbackEvent) into the Document()->Dispatch() method.
Comment on attachment 8847945 [details]
Bug 1342863 - (Part 7) DocGroup labeling for timer callback in nsPresContext.

https://reviewboard.mozilla.org/r/120872/#review124712

::: commit-message-1610e:1
(Diff revision 5)
> +Bug 1342863 - (Part 7) DocGroup labeling for timer callabck in nsPresContext. r?dholbert

Typo: s/callabck/callback/
(That ^^ may be all the review nits that I have -- I haven't quite marked these r+ yet, because I want to read up on TaskCategory::Other etc. & verify that the usage seems appropriate here before I officially grant review.)
Comment on attachment 8847464 [details]
Bug 1342863 - (Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext.

https://reviewboard.mozilla.org/r/120442/#review124710

> As with part 1, it'd be a bit simpler to use do_AddRef here (rather than creating an extra local variable here, beyond mWillPaintFallbackEvent which already stores the same thing).
> 
> I think you can just restore the original "mWillPaintFallbackEvent = new (...)" line, and pass do_AddRef(mWillPaintFallbackEvent) into the Document()->Dispatch() method.

`do_AddRef()` can't apply on `mWillPaintFallbackEvent`. The type of `mWillPaintFallbackEvent` is `nsRevocableEventPtr`, and `nsRevocableEventPtr` doesn't inherit `RefPtr`.
Comment on attachment 8847463 [details]
Bug 1342863 - (Part 1) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in PresShell.

https://reviewboard.mozilla.org/r/120440/#review125044

r=me with nits:

::: layout/base/PresShell.cpp:2015
(Diff revision 6)
> -                          this, &PresShell::FireResizeEvent);
> -      if (NS_SUCCEEDED(NS_DispatchToCurrentThread(resizeEvent))) {
> -        mResizeEvent = resizeEvent;
> +      nsresult rv = GetDocument()->Dispatch("PresShell::FireResizeEvent",
> +                                            TaskCategory::Other,
> +                                            do_AddRef(event));

Let's use mDocument instead of GetDocument() here. 

(That seems to be the prevailing usage inside of PresShell.cpp -- 73 hits for "mDocument->", 1 hit for "GetDocument()->")

Also: after that changes, the subsequent lines (the Dispatch(...) args) will need to be deindented by 4 spaces.

::: layout/base/PresShell.cpp:6264
(Diff revision 6)
>    }
>  
> -  RefPtr<nsRunnableMethod<PresShell> > ev =
> -    NewRunnableMethod("PresShell::UpdateApproximateFrameVisibility",
> -                      this, &PresShell::UpdateApproximateFrameVisibility);
> -  if (NS_SUCCEEDED(NS_DispatchToCurrentThread(ev))) {
> +  RefPtr<nsRunnableMethod<PresShell>> event =
> +    NewRunnableMethod(this, &PresShell::UpdateApproximateFrameVisibility);
> +  nsresult rv =
> +    GetDocument()->Dispatch("PresShell::UpdateApproximateFrameVisibility",

Same here -- please s/GetDocument()/mDocument/ and deindent subsequent lines.
Attachment #8847463 - Flags: review?(dholbert) → review+
Comment on attachment 8847464 [details]
Bug 1342863 - (Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext.

https://reviewboard.mozilla.org/r/120442/#review124710

> `do_AddRef()` can't apply on `mWillPaintFallbackEvent`. The type of `mWillPaintFallbackEvent` is `nsRevocableEventPtr`, and `nsRevocableEventPtr` doesn't inherit `RefPtr`.

I see -- thank you for clarifying.
Comment on attachment 8847464 [details]
Bug 1342863 - (Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext.

https://reviewboard.mozilla.org/r/120442/#review124710

> I see -- thank you for clarifying.

Actually, looks like do_AddRef can take a raw pointer, which nsRevocableEventPtr will return via "get()".

So I think you *could* still restore "mWillPaintFallbackEvent = new RunWillPaintObservers(this);" and then pass in do_AddRef(mWillPaintFallbackEvent.get()) to Dispatch() call.  Slightly shorter, but perhaps a little less elegant. *shrug*

r=me on this part either way.
Comment on attachment 8847464 [details]
Bug 1342863 - (Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext.

https://reviewboard.mozilla.org/r/120442/#review125062
Attachment #8847464 - Flags: review?(dholbert) → review+
Comment on attachment 8847465 [details]
Bug 1342863 - (Part 3) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in ZoomConstraintsClient.

https://reviewboard.mozilla.org/r/120444/#review125064

r=me
Attachment #8847465 - Flags: review?(dholbert) → review+
Comment on attachment 8847466 [details]
Bug 1342863 - (Part 4) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsDocumentViewer.

https://reviewboard.mozilla.org/r/120446/#review125096

r=me with one nit:

::: layout/base/nsDocumentViewer.cpp:2136
(Diff revision 6)
>    // Notify observers that a new page has been shown. This will get run
>    // from the event loop after we actually draw the page.
> -  NS_DispatchToMainThread(new nsDocumentShownDispatcher(mDocument));
>  
> +  RefPtr<nsDocumentShownDispatcher> event =
> +    new nsDocumentShownDispatcher(mDocument);
> +  mDocument->Dispatch("nsDocumentShownDispatcher",

Delete the blank line between the comment and the code here, please (since the comment is describing this code).
Attachment #8847466 - Flags: review?(dholbert) → review+
Comment on attachment 8847467 [details]
Bug 1342863 - (Part 5) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsRefreshDriver.

https://reviewboard.mozilla.org/r/120448/#review125098

r=me with one null check added

::: layout/base/nsRefreshDriver.cpp:2116
(Diff revision 6)
>        // updates our mMostRecentRefresh, but the DoRefresh call won't run
>        // and notify our observers until we get back to the event loop.
>        // Thus MostRecentRefresh() will lie between now and the DoRefresh.
> -      NS_DispatchToCurrentThread(NewRunnableMethod(this, &nsRefreshDriver::DoRefresh));
> +      RefPtr<nsRunnableMethod<nsRefreshDriver>> event =
> +        NewRunnableMethod(this, &nsRefreshDriver::DoRefresh);
> +      GetPresContext()->Document()->Dispatch("nsRefreshDriver::DoRefresh",

You need to null-check the result of GetPresContext(), because in general it might return null.

(The "Get" prefix on pointer-returning methods generally means "this might need a null-check". And And indeed, most calls to GetPresContext() in this file *do* include a null-check.)

So, please do:
 nsPresContext* pc = GetPresContext();
 if (pc) {
   pc->Document()->Dispatch(...)
   EnsureTimerStarted();
 } else {
   NS_ERROR("Thawing while document is being destroyed");
 }

(I suspect that in practice GetPresContext is always non-null here, but the NS_ERROR will help us find out if that's actually true or not. And I think we're fine to skip the DoRefresh/EnsureTimerStarted calls when GetPresContext is null.)
Attachment #8847467 - Flags: review?(dholbert) → review+
Comment on attachment 8847944 [details]
Bug 1342863 - (Part 6) DocGroup labeling for timer callback in PresShell.

https://reviewboard.mozilla.org/r/120870/#review125106

r=me with nits addressed (though feel free to tag me for followup review if the RefPtr thing isn't as trivial of a fix as I'm expecting)

::: layout/base/PresShell.cpp:1851
(Diff revision 6)
> +      mPaintSuppressionTimer->SetTarget(
> +          GetDocument()->EventTargetFor(TaskCategory::Other));

As noted for part 1, PresShell.cpp seems to prefer mDocument over GetDocument() (and it's less uneasy-making about potential nullness from the "Get" prefix).

So, s/GetDocument()/mDocument/ here, please.

::: layout/base/PresShell.cpp:2007
(Diff revision 6)
> +        mAsyncResizeEventTimer->SetTarget(
> +            GetDocument()->EventTargetFor(TaskCategory::Other));

s/GetDocument()/mDocument/

::: layout/base/PresShell.cpp:3706
(Diff revision 6)
>      // Delay paint for 1 second.
>      static const uint32_t kPaintDelayPeriod = 1000;
>      if (!mDelayedPaintTimer) {
> +      nsTimerCallbackFunc
> +        PaintTimerCallBack = [](nsITimer* aTimer, void* aClosure) {
> +          RefPtr<PresShell> self = static_cast<PresShell*>(aClosure);

Is there a reason you're using a local RefPtr inside of this callback, rather than just a raw pointer? The old code doesn't seem to use a RefPtr in the corresponding callback, and I don't think we need to add one. (Though I may be missing something)

If I understand correctly, two requests on this topic:
 (1) Please use a raw pointer, not a refptr, like so:
  auto self = static_cast<PresShell*>(aClosure);
(static_cast is one of the instances where we definitely allow the "auto" keyword, to avoid repeating the type twice on the same line.)

(2) Please add a comment inside/alongside this method to explain how we know that the PresShell is still alive despite not capturing a RefPtr. Something like:
      // Even though we don't retain a reference, we can be sure that the
      // passed-in PresShell is still alive, because if it died, it would've
      // called mDelayedPaintTimer->Cancel() during destruction and we'd
      // never have been invoked.

::: layout/base/PresShell.cpp:3712
(Diff revision 6)
> -      RefPtr<PaintTimerCallBack> cb = new PaintTimerCallBack(this);
> -      mDelayedPaintTimer->InitWithCallback(cb, kPaintDelayPeriod, nsITimer::TYPE_ONE_SHOT);
> +      mDelayedPaintTimer->SetTarget(
> +          GetDocument()->EventTargetFor(TaskCategory::Other));

s/GetDocument()/mDocument/

::: layout/base/PresShell.cpp:9151
(Diff revision 6)
> +    mReflowContinueTimer->SetTarget(
> +        GetDocument()->EventTargetFor(TaskCategory::Other));

s/GetDocument()/mDocument/
Attachment #8847944 - Flags: review?(dholbert) → review+
Comment on attachment 8847945 [details]
Bug 1342863 - (Part 7) DocGroup labeling for timer callback in nsPresContext.

https://reviewboard.mozilla.org/r/120872/#review125120

r=me
Attachment #8847945 - Flags: review?(dholbert) → review+
Comment on attachment 8847946 [details]
Bug 1342863 - (Part 8) DocGroup labeling for timer callback in nsRefreshDriver.

https://reviewboard.mozilla.org/r/120874/#review125128

r=me with a nullcheck added.

::: layout/base/nsRefreshDriver.cpp:1436
(Diff revision 6)
>        NS_ASSERTION(!sDisableHighPrecisionTimersTimer, "We shouldn't have an outstanding disable-high-precision timer !");
>  
>        nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
>        if (timer) {
> +        timer->SetTarget(
> +            GetPresContext()->Document()->EventTargetFor(TaskCategory::Other));
>          timer.forget(&sDisableHighPrecisionTimersTimer);

As with the other nsRefreshDriver patch, you technically need to be null-checking the result of GetPresContext() before you use it.

It looks like you only need the prescontext for the "SetTarget" call, and I think the SetTarget call is optional -- so you're probably fine to *just* wrap that in a null-check, and leave the rest the way you've already got it. (Right?)

So I think this should be OK:
  nsPresContext* pc = GetPresContext();
  if (pc) {
    timer->SetTarget(
      pc->->Document()->EventTargetFor(TaskCategory::Other));
  }
  timer.forget(...)
  etc.
Attachment #8847946 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17d753a5dd9a
(Part 1) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in PresShell. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b6422d885f48
(Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ec75fa65fb37
(Part 3) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in ZoomConstraintsClient. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1b57aaf3cb59
(Part 4) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsDocumentViewer. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1c8548bdac7f
(Part 5) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsRefreshDriver. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/07e257b46b6f
(Part 6) DocGroup labeling for timer callback in PresShell. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/581cfae86457
(Part 7) DocGroup labeling for timer callback in nsPresContext. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ca409f78c8a3
(Part 8) DocGroup labeling for timer callback in nsRefreshDriver. r=dholbert
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e8ee1c17236
(Part 1) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in PresShell. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/39ae4d2d94e6
(Part 2) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsPresContext. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2f76caec4138
(Part 3) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in ZoomConstraintsClient. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fdb7eefd7199
(Part 4) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsDocumentViewer. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e37a0705737c
(Part 5) DocGroup labeling for runnables dispatched by NS_DispatchTo(Main|Current)Thread in nsRefreshDriver. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/03c58ad8e44f
(Part 6) DocGroup labeling for timer callback in PresShell. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/54638993c8b2
(Part 7) DocGroup labeling for timer callback in nsPresContext. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2d27eb9b83cb
(Part 8) DocGroup labeling for timer callback in nsRefreshDriver. r=dholbert
Keywords: checkin-needed
Depends on: 1350566
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.