Last Comment Bug 749101 - Move window.performance to the new DOM bindings
: Move window.performance to the new DOM bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on: 742144 757164 774757
Blocks: domperf ParisBindings
  Show dependency treegraph
 
Reported: 2012-04-26 01:24 PDT by :Gijs Kruitbosch (away 26-29 incl.)
Modified: 2012-07-18 05:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (34.00 KB, patch)
2012-07-09 22:26 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Patch (v2) (34.81 KB, patch)
2012-07-10 08:01 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v3) (35.29 KB, patch)
2012-07-10 20:18 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review

Description :Gijs Kruitbosch (away 26-29 incl.) 2012-04-26 01:24:50 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2012-06-12 23:20:51 PDT
Could use typedef support here so we don't have to do the manual expansion thing.
Comment 2 :Ehsan Akhgari 2012-07-09 22:26:33 PDT
Created attachment 640506 [details] [diff] [review]
Patch (v1)

This seems to work correctly!
Comment 3 Boris Zbarsky [:bz] 2012-07-09 22:51:19 PDT
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?
Comment 4 :Ehsan Akhgari 2012-07-10 08:01:29 PDT
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.
Comment 5 :Ehsan Akhgari 2012-07-10 12:04:12 PDT
(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!
Comment 6 Boris Zbarsky [:bz] 2012-07-10 18:44:13 PDT
> 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.  ;)
Comment 7 :Ehsan Akhgari 2012-07-10 18:57:39 PDT
(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?
Comment 8 :Ehsan Akhgari 2012-07-10 19:03:55 PDT
(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.
Comment 9 Boris Zbarsky [:bz] 2012-07-10 19:14:58 PDT
> 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.
Comment 10 :Ehsan Akhgari 2012-07-10 20:18:54 PDT
Created attachment 640899 [details] [diff] [review]
Patch (v3)

Removed the quickstub entries.
Comment 11 Boris Zbarsky [:bz] 2012-07-13 17:25:12 PDT
Comment on attachment 640899 [details] [diff] [review]
Patch (v3)

r=me
Comment 12 :Ehsan Akhgari 2012-07-16 18:45:52 PDT
Landed with the infallibility annotations moved to the WebIDL files.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd08c10193c6
Comment 13 :Ehsan Akhgari 2012-07-16 19:42:52 PDT
Filed bug 774556 for removing nsIDOMWindowPerformance* interfaces.
Comment 14 :Ehsan Akhgari 2012-07-16 22:42:00 PDT
Added a comment mentioning bug 772589 (which I forgot to do when landing):

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb282ed64bf7
Comment 15 Ed Morley [:emorley] 2012-07-17 02:10:19 PDT
https://hg.mozilla.org/mozilla-central/rev/dd08c10193c6
Comment 16 Ed Morley [:emorley] 2012-07-18 05:58:07 PDT
https://hg.mozilla.org/mozilla-central/rev/fb282ed64bf7

Note You need to log in before you can comment on or make changes to this bug.