Closed
Bug 1350787
Opened 8 years ago
Closed 8 years ago
Label runnables in dom/xhr
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hsinyi, Assigned: shawnjohnjr)
References
Details
(Whiteboard: [QDL][TDC-MVP][DOM])
Attachments
(1 file, 4 obsolete files)
This bug is to track and review how to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to dom/xhr.
Reporter | ||
Updated•8 years ago
|
Summary: Label runnables in xhr/ → Label runnables in dom/xhr
Reporter | ||
Updated•8 years ago
|
Whiteboard: [QDL][TDC-MVP][DOM]
Assignee | ||
Comment 1•8 years ago
|
||
Sorry for a late response. I'm now switching to this bug.
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8864439 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8864443 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr
Review of attachment 8864443 [details] [diff] [review]:
-----------------------------------------------------------------
Hi Bevis,
Do you mind reviewing this labeling patch?
I search based on 'dispatch' keyword, since bug 1318506 already fixed Timer callback part.
Attachment #8864443 -
Flags: review?(btseng)
Comment 5•8 years ago
|
||
Comment on attachment 8864443 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr
Review of attachment 8864443 [details] [diff] [review]:
-----------------------------------------------------------------
I'd like to revisit again after the comments are addressed.
::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2834,5 @@
> mSuspendedDoc = nullptr;
> }
>
> if (mResumeTimeoutRunnable) {
> + DispatchRunnable(GetOwnerGlobal(), mResumeTimeoutRunnable.forget());
Add MOZ_ASSERT(NS_IsMainThread()); at the beginning of XMLHttpRequestMainThread::UnsuppressEventHandlingAndResume().
@@ +2841,5 @@
> }
>
> nsresult
> XMLHttpRequestMainThread::SendInternal(const BodyExtractorBase* aBody)
> {
MOZ_ASSERT(NS_IsMainThread());
@@ +3130,5 @@
> +nsresult
> +XMLHttpRequestMainThread::DispatchRunnable(nsIGlobalObject* aGlobal,
> + already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> + nsCOMPtr<nsIRunnable> runnable = aRunnable;
You don't need this but just call target->Dispatch(Move(aRunnable), ...) isntead.
@@ +3132,5 @@
> + already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> + nsCOMPtr<nsIRunnable> runnable = aRunnable;
> +
> + nsCOMPtr<nsIEventTarget> target = aGlobal->EventTargetFor(TaskCategory::Other);
Add MOZ_ASSERT(aGlobal); before accessing aGlobal if you think that aGlobal will never be nullptr when accessing.
::: dom/xhr/XMLHttpRequestMainThread.h
@@ +579,5 @@
> nsresult OnRedirectVerifyCallback(nsresult result);
>
> void SetTimerEventTarget(nsITimer* aTimer);
>
> + nsresult DispatchRunnable(nsIGlobalObject* aGlobal,
DispatchToMainThread sounds better.
Why don't we have GetOwnerGlobal() inside its implementation instead of having aGlobal as a parameter?
Will aGlobal be different from different call site?
Attachment #8864443 -
Flags: review?(btseng)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5)
> Comment on attachment 8864443 [details] [diff] [review]
> Bug 1350787 - DocGroup labeling runnables in dom/xhr
>
> Review of attachment 8864443 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I'd like to revisit again after the comments are addressed.
>
> ::: dom/xhr/XMLHttpRequestMainThread.cpp
> @@ +2834,5 @@
> > mSuspendedDoc = nullptr;
> > }
> >
> > if (mResumeTimeoutRunnable) {
> > + DispatchRunnable(GetOwnerGlobal(), mResumeTimeoutRunnable.forget());
>
> Add MOZ_ASSERT(NS_IsMainThread()); at the beginning of
> XMLHttpRequestMainThread::UnsuppressEventHandlingAndResume().
In general, in class XMLHttpRequestMainThread, all functions are missing MOZ_ASSERT(NS_IsMainThread()).
I thought it's on purpose not to add it. Anyway, I will add it.
>
> @@ +3130,5 @@
> > +nsresult
> > +XMLHttpRequestMainThread::DispatchRunnable(nsIGlobalObject* aGlobal,
> > + already_AddRefed<nsIRunnable>&& aRunnable)
> > +{
> > + nsCOMPtr<nsIRunnable> runnable = aRunnable;
>
> You don't need this but just call target->Dispatch(Move(aRunnable), ...)
> isntead.
>
Good catch! I now learn new stuff this time.
> @@ +3132,5 @@
> > + already_AddRefed<nsIRunnable>&& aRunnable)
> > +{
> > + nsCOMPtr<nsIRunnable> runnable = aRunnable;
> > +
> > + nsCOMPtr<nsIEventTarget> target = aGlobal->EventTargetFor(TaskCategory::Other);
>
> Add MOZ_ASSERT(aGlobal); before accessing aGlobal if you think that aGlobal
> will never be nullptr when accessing.
okay.
> ::: dom/xhr/XMLHttpRequestMainThread.h
> @@ +579,5 @@
> > nsresult OnRedirectVerifyCallback(nsresult result);
> >
> > void SetTimerEventTarget(nsITimer* aTimer);
> >
> > + nsresult DispatchRunnable(nsIGlobalObject* aGlobal,
>
> DispatchToMainThread sounds better.
> Why don't we have GetOwnerGlobal() inside its implementation instead of
> having aGlobal as a parameter?
Okay.
>
> Will aGlobal be different from different call site?
It should not.
Assignee | ||
Updated•8 years ago
|
Attachment #8864443 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8864762 -
Flags: feedback?(btseng)
Comment 8•8 years ago
|
||
Comment on attachment 8864762 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr
Review of attachment 8864762 [details] [diff] [review]:
-----------------------------------------------------------------
Per offline discussion, it will be great if we can cache EventTarget at ::Construct() time, then we don't have to make assertion on GetOwnerGlobal() when dispatching runnables.
If there is still any limitation on caching it, then we can replace with this patch instead.
Thank!
Attachment #8864762 -
Flags: feedback?(btseng)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> Comment on attachment 8864762 [details] [diff] [review]
> Bug 1350787 - DocGroup labeling runnables in dom/xhr
>
> Review of attachment 8864762 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Per offline discussion, it will be great if we can cache EventTarget at
> ::Construct() time, then we don't have to make assertion on GetOwnerGlobal()
> when dispatching runnables.
> If there is still any limitation on caching it, then we can replace with
> this patch instead.
>
> Thank!
I will figure out that problem for sure.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> > Comment on attachment 8864762 [details] [diff] [review]
> > Bug 1350787 - DocGroup labeling runnables in dom/xhr
> >
> > Review of attachment 8864762 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Per offline discussion, it will be great if we can cache EventTarget at
> > ::Construct() time, then we don't have to make assertion on GetOwnerGlobal()
> > when dispatching runnables.
> > If there is still any limitation on caching it, then we can replace with
> > this patch instead.
> >
> > Thank!
>
> I will figure out that problem for sure.
As discussed, we don't need to have mEventTarget, but just handle in |XMLHttpRequestMainThread::DispatchToMainThread|. In ::Construct(), aGlobalObject can be nullptr.
Assignee | ||
Updated•8 years ago
|
Attachment #8864762 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8864796 -
Flags: feedback?(btseng)
Comment 12•8 years ago
|
||
Comment on attachment 8864796 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr
Review of attachment 8864796 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8864796 -
Flags: feedback?(btseng) → feedback+
Assignee | ||
Updated•8 years ago
|
Attachment #8864796 -
Flags: review?(amarchesini)
Comment 13•8 years ago
|
||
Comment on attachment 8864796 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr
Review of attachment 8864796 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3028,5 @@
> return NS_ERROR_DOM_NETWORK_ERR;
> } else {
> // Defer the actual sending of async events just in case listeners
> // are attached after the send() method is called.
> + DispatchToMainThread(NewRunnableMethod<ProgressEventType>(this,
return ispatchToMainThread(...
Attachment #8864796 -
Flags: review?(amarchesini) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8864796 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
Comment 16•8 years ago
|
||
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4607916d474a
DocGroup labeling runnables in dom/xhr, r=baku, f=bevistseng
Comment 17•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•