Closed Bug 884268 Opened 6 years ago Closed 6 years ago

Move nsContentUtils::PreserveWrapper into nsWrapperCache

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

Attachments

(3 files)

Feel free to throw this at someone else if you're busy.
Attachment #764090 - Flags: review?(bugs)
Attachment #764092 - Flags: review?(bugs)
Comment on attachment 764090 [details] [diff] [review]
Part a: Move CheckCCWrapperTraversal

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

::: dom/base/nsWrapperCache.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsWrapperCache.h"

This should be nsWrapperCacheInlines.h, actually, to link on debug builds on try. Fixed locally.
Attachment #764090 - Flags: review?(bugs) → review+
Comment on attachment 764092 [details] [diff] [review]
Part b: Move PreserveWrapper

foo->PreserveWrapper(foo); looks a bit odd, but is no worse than what we have now.

>@@ -8297,21 +8297,17 @@ class CGBindingRoot(CGThing):
>         descriptors = config.getDescriptors(webIDLFile=webIDLFile,
>                                             hasInterfaceOrInterfacePrototypeObject=True,
>                                             skipGen=False)
>         def descriptorRequiresPreferences(desc):
>             iface = desc.interface
>             return any(m.getExtendedAttribute("Pref") for m in iface.members + [iface]);
>         requiresPreferences = any(descriptorRequiresPreferences(d) for d in descriptors)
>         hasOwnedDescriptors = any(d.nativeOwnership == 'owned' for d in descriptors)
>-        def descriptorRequiresContentUtils(desc):
>-            return ((desc.concrete and not desc.proxy and
>-                     not desc.workers and desc.wrapperCache) or
>-                    desc.interface.hasInterfaceObject())
>-        requiresContentUtils = any(descriptorRequiresContentUtils(d) for d in descriptors)
>+        requiresContentUtils = any(d.interface.hasInterfaceObject() for d in descriptors)
Could you explain this change. Though the build would fail if this was wrong.
Attachment #764092 - Flags: review?(bugs) → review+
Attachment #764093 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 764092 [details] [diff] [review]
> Part b: Move PreserveWrapper
> 
> foo->PreserveWrapper(foo); looks a bit odd, but is no worse than what we
> have now.
> 
> >@@ -8297,21 +8297,17 @@ class CGBindingRoot(CGThing):
> >         descriptors = config.getDescriptors(webIDLFile=webIDLFile,
> >                                             hasInterfaceOrInterfacePrototypeObject=True,
> >                                             skipGen=False)
> >         def descriptorRequiresPreferences(desc):
> >             iface = desc.interface
> >             return any(m.getExtendedAttribute("Pref") for m in iface.members + [iface]);
> >         requiresPreferences = any(descriptorRequiresPreferences(d) for d in descriptors)
> >         hasOwnedDescriptors = any(d.nativeOwnership == 'owned' for d in descriptors)
> >-        def descriptorRequiresContentUtils(desc):
> >-            return ((desc.concrete and not desc.proxy and
> >-                     not desc.workers and desc.wrapperCache) or
> >-                    desc.interface.hasInterfaceObject())
> >-        requiresContentUtils = any(descriptorRequiresContentUtils(d) for d in descriptors)
> >+        requiresContentUtils = any(d.interface.hasInterfaceObject() for d in descriptors)
> Could you explain this change. Though the build would fail if this was wrong.

There's currently two places in the generated code where we use contentutils: for PreserveWrapper (if desc.concrete and not desc.proxy and not desc.workers and desc.wrapperCache), and to get xpconnect in CGClassHasInstanceHook (if desc.interface.hasInterfaceObject()). Since I changed the PreserveWrapper call to not go through contentutils, we don't need to include it anymore.
https://hg.mozilla.org/mozilla-central/rev/c4618e8cb8ff
https://hg.mozilla.org/mozilla-central/rev/d4c35209af10
https://hg.mozilla.org/mozilla-central/rev/6fe13dc22c56
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.