Label runnables in dom/media/VideoFrameContainer.cpp

RESOLVED FIXED in Firefox 54

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1341539
Priority: -- → P3
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8842709 [details]
Bug 1343458 - Label runnables in dom/media/VideoFrameContainer.cpp.

https://reviewboard.mozilla.org/r/116478/#review118584

As far as I can tell, what you did here is assign these runnables to a tabgroup, you didn't actually label the runnables, unless I missed something? So either r+ with the changes I suggest to ensure the runnables are labled, or an explanation and/or the commit message adjusted to ensure it reflects what your patch does.

::: dom/media/VideoFrameContainer.cpp:177
(Diff revision 1)
>                                            newImages.LastElement().mFrameID);
>    }
>  
>    SetCurrentFramesLocked(mLastPlayedVideoFrame.GetIntrinsicSize(), images);
>    nsCOMPtr<nsIRunnable> event =
>      new VideoFrameContainerInvalidateRunnable(this);

VideoFrameContainerInvalidateRunnable inherits from mozilla::Runnable, which implements nsINamed, so you should be able to call SetNamed() in its constructor to set a distinctive name.

::: dom/media/VideoFrameContainer.cpp:249
(Diff revision 1)
>      RefPtr<VideoFrameContainer> self = this;
>      PrincipalHandle principalHandle = mPendingPrincipalHandle;
>      mLastPrincipalHandle = mPendingPrincipalHandle;
>      mPendingPrincipalHandle = PRINCIPAL_HANDLE_NONE;
>      mFrameIDForPendingPrincipalHandle = 0;
> -    NS_DispatchToMainThread(NS_NewRunnableFunction([self, principalHandle]() {
> +    mMainThread->Dispatch(NS_NewRunnableFunction([self, principalHandle]() {

It seems we should be using the variant of NS_NewRunnableFunction that takes a name argument going forward?

https://dxr.mozilla.org/mozilla-central/rev/d29f84406483c721a13cf9a52936ecced0c5c98a/xpcom/threads/nsThreadUtils.h#437
Attachment #8842709 - Flags: review?(cpearce) → review+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8842709 [details]
Bug 1343458 - Label runnables in dom/media/VideoFrameContainer.cpp.

https://reviewboard.mozilla.org/r/116478/#review118584

By labeling a runnable, it means to associate a special attribue with the runnable so its execution can be prioritized and delayed. This is done by dispatching a runnable through some utility class. See https://wiki.mozilla.org/Quantum/DOM#Q1:_What_is_the_impact_of_leaving_a_runnable_unlabeled.3F for more details. Giving a runnable a name however is another kind of labeling which doesn't change the execution order of runnables and is for debugging purpose only.
Comment hidden (mozreview-request)

Comment 5

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6874dcb3e095
Label runnables in dom/media/VideoFrameContainer.cpp. r=cpearce
Thanks for explaining.

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6874dcb3e095
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

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