Label runnables in layout/tables/

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: TYLin, Assigned: jeremychen)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

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

User Story

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

Attachments

(1 attachment)

Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

Try with this patch looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a11341b4992a9459db9dec7129dac67efcaf5155

I'm not sure if this code path would sometimes go through off-main thread, so I sent a try to check this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be0279806a4c2fdfc4ef4365f5f1e87ce957b09

Looks like there's no crash on try. However, I'm not familiar with these codes, I can't be sure if we could remove the "NS_IsMainThread" branch condition in this patch.


Hi Cameron, could you take a look at this and give me some feedback? Thanks.
Attachment #8845224 - Flags: feedback?(cam)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

https://reviewboard.mozilla.org/r/118420/#review120402

f+, the actual Dispatch call looks right, though the rest of the code around it could be simplified a bit.

::: layout/tables/nsTableFrame.cpp:5008
(Diff revision 3)
>      // XXX In principle this should only be necessary for border style changes
>      // However the bc painting code tries to maximize the drawn border segments
>      // so it stores in the cellmap where a new border segment starts and this
>      // introduces a unwanted cellmap data dependence on color
>      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> +    if (NS_IsMainThread()) {

We don't need to check if we're on the main thread here.  We should always be on the main thread here (looks like BCRecalcNeeded is just called from DidSetStyleContext, which is done on the main thread).  So maybe the use of NS_DispatchToCurrentThread is a little misleading, since it sounds like it needs to handle us running on any thread.  (I'm not sure if there is some advantage to using NS_DispatchToCurrentThread when we know we're on the main thread, but I did notice that there are lots of uses of it that could be replaced with NS_DispatchToMainThread with no change in behaviour.)

As far as I know, it is safe to call Dispatch even if we are dispatching to a non-main thread; it's just not necessary.  So for simplicity, if we did need to handle different threads here, we should still just use a single Dispatch call, rather than switching to decide whether to call NS_DispatchToCurrentThread instead.

::: layout/tables/nsTableFrame.cpp:5009
(Diff revision 3)
>      // However the bc painting code tries to maximize the drawn border segments
>      // so it stores in the cellmap where a new border segment starts and this
>      // introduces a unwanted cellmap data dependence on color
>      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> +    if (NS_IsMainThread()) {
> +      nsCOMPtr<nsIDocument> doc = this->GetContent()->OwnerDoc();

No need for "this->".

Also, OwnerDoc() never returns nulls, so no need to null check it.
(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8845224 [details]
> Bug 1342873 - wip.
> 
> https://reviewboard.mozilla.org/r/118420/#review120402
> 
> f+, the actual Dispatch call looks right, though the rest of the code around
> it could be simplified a bit.
> 
> ::: layout/tables/nsTableFrame.cpp:5008
> (Diff revision 3)
> >      // XXX In principle this should only be necessary for border style changes
> >      // However the bc painting code tries to maximize the drawn border segments
> >      // so it stores in the cellmap where a new border segment starts and this
> >      // introduces a unwanted cellmap data dependence on color
> >      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> > +    if (NS_IsMainThread()) {
> 
> We don't need to check if we're on the main thread here.  We should always
> be on the main thread here (looks like BCRecalcNeeded is just called from
> DidSetStyleContext, which is done on the main thread).  So maybe the use of
> NS_DispatchToCurrentThread is a little misleading, since it sounds like it
> needs to handle us running on any thread.  (I'm not sure if there is some
> advantage to using NS_DispatchToCurrentThread when we know we're on the main
> thread, but I did notice that there are lots of uses of it that could be
> replaced with NS_DispatchToMainThread with no change in behaviour.)

Yes, for main thread cases, DispatchToMainThread and DispatchToCurrentThread should behave the same. However, I'm not sure if replacing DispatchToCurrentThread with DispatchToMainThread on main thread cases would be more clear or not, since DispatchToMainThread seems implicitly represents a potential cross-threads event looping.

> As far as I know, it is safe to call Dispatch even if we are dispatching to
> a non-main thread; it's just not necessary.  So for simplicity, if we did
> need to handle different threads here, we should still just use a single
> Dispatch call, rather than switching to decide whether to call
> NS_DispatchToCurrentThread instead.

Okay, sounds fair enough.

> ::: layout/tables/nsTableFrame.cpp:5009
> (Diff revision 3)
> >      // However the bc painting code tries to maximize the drawn border segments
> >      // so it stores in the cellmap where a new border segment starts and this
> >      // introduces a unwanted cellmap data dependence on color
> >      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> > +    if (NS_IsMainThread()) {
> > +      nsCOMPtr<nsIDocument> doc = this->GetContent()->OwnerDoc();
> 
> No need for "this->".
> 
> Also, OwnerDoc() never returns nulls, so no need to null check it.

Lesson learned!!!! Thank you.
Comment hidden (mozreview-request)
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED

Comment 8

2 years ago
mozreview-review
Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

https://reviewboard.mozilla.org/r/118420/#review120446

Looks good, thanks.
Attachment #8845224 - Flags: review?(cam) → review+

Comment 9

2 years ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6932ea4168a2
label runnables of nsDelayedCalcBCBorders for nsTableFrame. r=heycam

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6932ea4168a2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1347815
No longer blocks: 1347815

Updated

2 years ago
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.