The default bug view has changed. See this FAQ.

Move window.performance to the new DOM bindings

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Core & HTML
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Ehsan)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
This is a followup from bug 539095, where bz noted that window.performance.timing and window.performance.navigation (and maybe window.performance?) should use the new DOM bindings.
Blocks: 580070
Could use typedef support here so we don't have to do the manual expansion thing.
Depends on: 742144
(Assignee)

Updated

5 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 2

5 years ago
Created attachment 640506 [details] [diff] [review]
Patch (v1)

This seems to work correctly!
Attachment #640506 - Flags: review?(bzbarsky)
Comment on attachment 640506 [details] [diff] [review]
Patch (v1)

>+++ b/dom/base/nsPerformance.cpp
>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsPerformanceTiming)
...
>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsPerformanceTiming)
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
>+NS_IMPL_CYCLE_COLLECTION_TRACE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsPerformanceTiming)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mPerformance)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsPerformanceTiming)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPerformance)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

I believe you can replace all that with:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsPerformanceTiming, 
                                          mPerformance);


>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsPerformanceNavigation)

Similar here.

>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsPerformance)

And here, though you'll need to add NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_3 to nsWrapperCache.h

>+++ b/dom/bindings/Bindings.conf
>+'Performance': {
>+    'nativeType': 'nsPerformance',
>+    'infallible': [ 'now', 'timing', 'navigation' ]

You probably want to add:

  'resultNotAddRefed': [ 'timing', 'navigation' ]

to this so the binding won't do an extra addref/release on them.

I really need to get Peter to review my patch to move the infallibility annotations into the webidl.... ;)

>+++ b/dom/webidl/PerformanceTiming.webidl
>+  // readonly attribute unsigned long long secureConnectionStart;

Hmm.  Do we have a bug filed on that?  If not, would you mind filing?

I wonder whether the XPCOM interfaces for these are used in C++... If not, we can probably nix those too.  Probably in a followup.

r=me with the above nits.  Were there any particular pain points you ran into?
Attachment #640506 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 640605 [details] [diff] [review]
Patch (v2)

This patch was causing leaks.  In order to fix that, I broke the cycle of nsDOMNavigationTiming objects by making nsPerformanceTiming and nsPerformanceNavigation not hold strong refs to it and just ask nsPerformance about it (and thus making those two objects one word smaller), and also participating nsPerformance in the cycle collection of nsGlobalWindow.
Attachment #640506 - Attachment is obsolete: true
Attachment #640605 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #3)
> I really need to get Peter to review my patch to move the infallibility
> annotations into the webidl.... ;)

That would be nice!  What's the bug #?

> >+++ b/dom/webidl/PerformanceTiming.webidl
> >+  // readonly attribute unsigned long long secureConnectionStart;
> 
> Hmm.  Do we have a bug filed on that?  If not, would you mind filing?

-> bug 772589.

> I wonder whether the XPCOM interfaces for these are used in C++... If not,
> we can probably nix those too.  Probably in a followup.

I'll do that when I land this patch in another bug.  I don't know what happens if I remove the classinfo stuff for these interfaces and I didn't have enough time to figure it out last night.  :-)

> r=me with the above nits.  Were there any particular pain points you ran
> into?

No particular pain points besides setting up the cycle collection participation (and that was just as painful as it usually is!).  One thing that I would like to see in the documentation is sort of a higher overview on how these tie the js/C++ code, and what happens if your interface has the new bindings in addition to quickstubs for example (I think SetIsDOMBinding() tells the runtime code to ignore the quickstubs -- but are they safe to be removed?).  Also, it would be nice for the docs to point to a simple conversion patch.  The WebGL and 2d canvas patches were huge.  I was hoping for this one to be smaller but that didn't really happen either!
> That would be nice!  What's the bug #?

Bug 757164.

> I think SetIsDOMBinding() tells the runtime code to ignore the quickstubs -- but are they
> safe to be removed?

Yes, and yes.  I'll add that to the docs.

I think pointing to this patch for now makes sense, until something smaller comes along.  ;)
(Assignee)

Comment 7

5 years ago
(In reply to comment #6)
> > I think SetIsDOMBinding() tells the runtime code to ignore the quickstubs -- but are they
> > safe to be removed?
> 
> Yes, and yes.  I'll add that to the docs.

So, should we remove the quickstubs entries for the interfaces here and also for things like XHRs?
(Assignee)

Comment 8

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #6)
> I think pointing to this patch for now makes sense, until something smaller
> comes along.  ;)

Done.
> So, should we remove the quickstubs entries for the interfaces here and also for things
> like XHRs?

Here, yes.

For XHR, we should do that, and also remove the "prefable: True" bit we have for it right now.  And look into whether there are any C++ consumers, for that matter.  I think khuey had some patches along those lines, in fact.
(Assignee)

Comment 10

5 years ago
Created attachment 640899 [details] [diff] [review]
Patch (v3)

Removed the quickstub entries.
Attachment #640605 - Attachment is obsolete: true
Attachment #640605 - Flags: review?(bzbarsky)
Attachment #640899 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Depends on: 757164
Comment on attachment 640899 [details] [diff] [review]
Patch (v3)

r=me
Attachment #640899 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

5 years ago
Landed with the infallibility annotations moved to the WebIDL files.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd08c10193c6
Target Milestone: --- → mozilla17
(Assignee)

Comment 13

5 years ago
Filed bug 774556 for removing nsIDOMWindowPerformance* interfaces.
(Assignee)

Comment 14

5 years ago
Added a comment mentioning bug 772589 (which I forgot to do when landing):

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb282ed64bf7
https://hg.mozilla.org/mozilla-central/rev/dd08c10193c6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 774757
https://hg.mozilla.org/mozilla-central/rev/fb282ed64bf7
You need to log in before you can comment on or make changes to this bug.