884268 - Move nsContentUtils::PreserveWrapper into nsWrapperCache

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Move CheckCCWrapperTraversal

Feel free to throw this at someone else if you're busy.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Move PreserveWrapper

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Don't include nsWrapperCache.h in nsContentUtils.h

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 4

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

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.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/c4618e8cb8ff
https://hg.mozilla.org/mozilla-central/rev/d4c35209af10
https://hg.mozilla.org/mozilla-central/rev/6fe13dc22c56