1339707 - Label runnables in docshell/

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This bug is to track and review how to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to docshell/.

Comment 1

[Samael Wang [:freesamael] (away for now)]

Attachment: v0

Comment 2

[Samael Wang [:freesamael] (away for now)]

Attachment: v1

Comment 3

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8841441 [details] [diff] [review]
v1

Hi Olli,

I'm trying to replace runnable dispatching under docshell/ dir with TabGroup / DocGroup implmentation. Could you take a look?

Comment 4

[Samael Wang [:freesamael] (away for now)]

Attachment: v2

Comment 5

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8843909 [details] [diff] [review]
v2

Bevis reminded me that I should also update the nsITimer usages. The patch is updated accordingly.

I associated the tooltip & ping timer to the document, while refresh timer to tabgroup. Could you take another review for that?

BTW I'm noticing that we almost always use a native pointer to hold the return value of nsINode::OwnerDoc(). Is there a reason to prefer native pointer over nsCOMPtr in this case? I wonder when I should use a native pointer for similar cases.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Native pointer? Do you have link to some code?
My guess is that you mean raw pointer? If there isn't need for nsCOMPtr, using raw pointer is faster, no extra Addref/Release. But if one isn't sure whether some JS could run while using the variable, then use nsCOMPtr for safety.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843909 [details] [diff] [review]
v2

>+nsDocShell::DispatchToTabGroup(const char* aName,
>+                               TaskCategory aCategory,
>+                               already_AddRefed<nsIRunnable>&& aRunnable)
>+{
>+  // Hold the ref so we won't forget to release it.
>+  nsCOMPtr<nsIRunnable> runnable(aRunnable);
>+  nsCOMPtr<nsPIDOMWindowOuter> win = GetWindow();
>+  if (!win) {
>+    // Window should only be unavailable after destroyed.
>+    MOZ_ASSERT(mIsBeingDestroyed);
>+    return NS_ERROR_FAILURE;
>+  }
This is tiny bit scary to not run some runnables, but I guess it in practice should be fine.

> nsSHEntryShared::RemoveFromBFCacheAsync()
> {
>   NS_ASSERTION(mContentViewer && mDocument, "we're not in the bfcache!");
> 
>   // Release the reference to the contentviewer asynchronously so that the
>   // document doesn't get nuked mid-mutation.
> 
>+  if (!mDocument) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
ok, this is ok, but could you change the NS_ASSERTION to MOZ_ASSERT

Comment 8

[Samael Wang [:freesamael] (away for now)]

Attachment: Replace NS_DispatchToCurrentThread to DocGroup / TabGroup dispatch method and set nsITimer target

Comment 9

[Samael Wang [:freesamael] (away for now)]

Waiting for try result...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca09b5473bcbfc6e3e19377e2e48316c9af0d319&group_state=expanded

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Just realized that there is one more timer use implicitly in docshell:
http://searchfox.org/mozilla-central/source/docshell/shistory/nsSHEntryShared.cpp#36
which depends on the support of bug 1345464 that I just filed. :-|

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> Just realized that there is one more timer use implicitly in docshell:
> http://searchfox.org/mozilla-central/source/docshell/shistory/
> nsSHEntryShared.cpp#36
> which depends on the support of bug 1345464 that I just filed. :-|

we could have a followup bug depends on bug 1345464 to address this if the labeling of HistoryTracker is really required after further review.

Comment 12

[Samael Wang [:freesamael] (away for now)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> Just realized that there is one more timer use implicitly in docshell:
> http://searchfox.org/mozilla-central/source/docshell/shistory/
> nsSHEntryShared.cpp#36
> which depends on the support of bug 1345464 that I just filed. :-|

Cool I didn't notice that. Thx.

Comment 13

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b111ed7442f0e54f5c2e34d6180bf090c71aa06&group_state=expanded

Comment 14

[Samael Wang [:freesamael] (away for now)]

I think the try result looks generally ok.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1622f3e9dd54
Replace NS_DispatchToCurrentThread to DocGroup / TabGroup dispatch method and set nsITimer target. r=smaug

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1622f3e9dd54