Closed Bug 1225588 Opened 4 years ago Closed 4 years ago

Expose DominatorTree to JavaScript

Categories

(DevTools :: Memory, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

This commit adds the DominatorTree.webidl interface, which is only exposed to
chrome JS. The interface is implemented by mozilla::devtools::DominatorTree,
which is a thin wrapper around JS::ubi::DominatorTree. This does not expose any
methods on the DominatorTree interface, those will come as follow up changesets.
r?bholley for dom/webidl related changes

r?sfink for the rest
Attachment #8688597 - Flags: review?(sphink)
Attachment #8688597 - Flags: review?(bobbyholley)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
(Woops, forgot to add the new tests!)
Attachment #8688602 - Flags: review?(sphink)
Attachment #8688602 - Flags: review?(bobbyholley)
Attachment #8688597 - Attachment is obsolete: true
Attachment #8688597 - Flags: review?(sphink)
Attachment #8688597 - Flags: review?(bobbyholley)
Comment on attachment 8688602 [details] [diff] [review]
Expose DominatorTree to JavaScript

Review of attachment 8688602 [details] [diff] [review]:
-----------------------------------------------------------------

Looks relatively generic - please ask someone else.
Attachment #8688602 - Flags: review?(bobbyholley)
Attachment #8688602 - Flags: review?(bzbarsky)
Comment on attachment 8688602 [details] [diff] [review]
Expose DominatorTree to JavaScript

You need NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER in your unlink.

But more to the point, do you plan to get other traceable members here?  Because if not, just do:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DominatorTree, mParent)

and it will do all the NS_IMPL_CYCLE_COLLECTION_CLASS/UNLINK/TRAVERSE/TRACE for you.  If you do need the manual bits, I think the missing unlink is the only part that's missing here.

The indentation in your interface map impl is weird: one entry is indented 4 spaces, one is not indented at all.  Please indent both by 2 spaces like your modeline says.

>+DominatorTree::WrapObject(JSContext* aCx, JS::HandleObject aGivenProto)

Please indent the body of this by two spaces, like the modeline says.

>+  JS::ubi::DominatorTree dominatorTree;

mDominatorTree, for a member.  Certainly if you also have an mParent.  Mixing conventions is worse than either one.

>+  explicit DominatorTree(JS::ubi::DominatorTree&& dominatorTree, nsCOMPtr<nsISupports>& aParent)

Second arg should just be nsISupports*, right?

>+  MOZ_DECLARE_REFCOUNTED_TYPENAME(DominatorTree);

Why is this needed?  I don't think it should be here.

Looks like HeapSnapshot has the same (but preexisting) lack-of-unlinking bug.  Again, it should just use NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE.

I didn't review the devtools tests.

r=me with the above fixed.
Attachment #8688602 - Flags: review?(bzbarsky) → review+
Comment on attachment 8688602 [details] [diff] [review]
Expose DominatorTree to JavaScript

Review of attachment 8688602 [details] [diff] [review]:
-----------------------------------------------------------------

Nothing looks out of place to me in this scaffolding.
Attachment #8688602 - Flags: review?(sphink) → review+
(In reply to Boris Zbarsky [:bz] from comment #5)

Thanks for the cycle collection macro tips, I've been having a hard time with those and I suspect that the leaks we have been seeing there are related.

> >+  explicit DominatorTree(JS::ubi::DominatorTree&& dominatorTree, nsCOMPtr<nsISupports>& aParent)
> 
> Second arg should just be nsISupports*, right?

How do I know it is initially held by someone else and not a dangling pointer in that case? Isn't nsCOMPtr<T>& / RefPtr<T>& equivalent to JS::Handle<T>?
> How do I know it is initially held by someone else and not a dangling pointer

Because that's the contract for refcounted pointer arguments, typically.

> Isn't nsCOMPtr<T>& / RefPtr<T>& equivalent to JS::Handle<T>?

Not really.  Without const it's closer to a MutableHandle or something like that.  You could take a const ref, and then it would be kinda like Handle, but just very very ugly.  So the convention is to use raw ptrs as in params, and callers need to hold the ref.
This commit adds the DominatorTree.webidl interface, which is only exposed to
chrome JS. The interface is implemented by mozilla::devtools::DominatorTree,
which is a thin wrapper around JS::ubi::DominatorTree. This does not expose any
methods on the DominatorTree interface, those will come as follow up changesets.
Attachment #8689165 - Flags: review+
Attachment #8688602 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b6c8ce8730d7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.