1004487 - Add Cu.isModuleLoaded method to know if a resource has been loaded via Cu.import

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Girish Sharma [:Optimizer]]

Attachment: mem-is-loaded

See https://bugzilla.mozilla.org/show_bug.cgi?id=1001131#c20 for more information

Not sure about whom exactly to ask for review, or if it is the right component for the bug.

Comment 1

[Girish Sharma [:Optimizer]]

try [0] with everything green except X and dt which is fixed in the next try [1]

[0] https://tbpl.mozilla.org/?tree=Try&rev=8026aaf0812d
[1] https://tbpl.mozilla.org/?tree=Try&rev=6c464e889699

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8415882 [details] [diff] [review]
mem-is-loaded

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

Why does this need to be on Cu? It seems like a pretty niche use case, and so I think we should just leave it on xpcIJSModuleLoader.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1232,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsAutoCString key;
> +    rv = resolvedURI->GetSpec(key);
> +    NS_ENSURE_SUCCESS(rv, rv);

We should factor this out into a helper method to avoid the massive copy-paste.

Comment 3

[Girish Sharma [:Optimizer]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 8415882 [details] [diff] [review]
> mem-is-loaded
> 
> Review of attachment 8415882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why does this need to be on Cu? It seems like a pretty niche use case, and
> so I think we should just leave it on xpcIJSModuleLoader.

No such reason for it to be on Cu . I just think that if it is on Cu, its usage will increase. I personally like the kind of pattern isLoaded will introduce (also see bug 1004885)

Also, How to use method of xpcIJSModuleLoader from JS code ?

> ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> @@ +1232,5 @@
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    nsAutoCString key;
> > +    rv = resolvedURI->GetSpec(key);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> We should factor this out into a helper method to avoid the massive
> copy-paste.

Sure. Do you want that to be a private method ?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Girish Sharma [:Optimizer] from comment #3)
> > Why does this need to be on Cu? It seems like a pretty niche use case, and
> > so I think we should just leave it on xpcIJSModuleLoader.
> 
> No such reason for it to be on Cu . I just think that if it is on Cu, its
> usage will increase. I personally like the kind of pattern isLoaded will
> introduce (also see bug 1004885)

I'd imagine that we'd only use this in a couple of places. I'd rather not pollute Cu.


> 
> Also, How to use method of xpcIJSModuleLoader from JS code ?

Get the service, the same way the XPCComponents_Utils::Import does.
 
> > ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> > @@ +1232,5 @@
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > > +
> > > +    nsAutoCString key;
> > > +    rv = resolvedURI->GetSpec(key);
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > 
> > We should factor this out into a helper method to avoid the massive
> > copy-paste.
> 
> Sure. Do you want that to be a private method ?

Yes.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #4)
> I'd imagine that we'd only use this in a couple of places. I'd rather not
> pollute Cu.

We can always add it later, so I don't have any objection to starting with the conservative thing, but this seems likely to be useful in more than just a "couple of places".

Comment 6

[Bobby Holley (:bholley)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > I'd imagine that we'd only use this in a couple of places. I'd rather not
> > pollute Cu.
> 
> We can always add it later, so I don't have any objection to starting with
> the conservative thing, but this seems likely to be useful in more than just
> a "couple of places".

I'm totally willing to believe you, but can you explain why? In general, I'd think we'd only use it in glue code for memory tricks and whatnot. Most consumers basically want to Cu.import something and use it.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #6)
> I'm totally willing to believe you, but can you explain why? In general, I'd
> think we'd only use it in glue code for memory tricks and whatnot. Most
> consumers basically want to Cu.import something and use it.

I know of at least two existing cases where code wants to refer to a module, but only if it's already been loaded. And we've moving towards lazy-loading modules more and more. So I don't think it's isolated to "glue code". But like I said, I don't feel too strongly here :)

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #7)
> (In reply to Bobby Holley (:bholley) from comment #6)
> > I'm totally willing to believe you, but can you explain why? In general, I'd
> > think we'd only use it in glue code for memory tricks and whatnot. Most
> > consumers basically want to Cu.import something and use it.
> 
> I know of at least two existing cases where code wants to refer to a module,
> but only if it's already been loaded. And we've moving towards lazy-loading
> modules more and more. So I don't think it's isolated to "glue code". But
> like I said, I don't feel too strongly here :)

I also don't have any strong opinion towards both the things.

About the number of use cases.. I have not looked into any code (even in devtools) beyond the need for minimizing devtools modules at stratup. Thus, only 2 use cases.

I am sure there will be many other use cases, once people start thinking wrt this approach.

Comment 9

[Bobby Holley (:bholley)]

Ok, Cu is fine then, but let's call it something more specific, like Cu.isModuleLoaded?

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: mem-is-loaded v2

So, I renamed the method to isModuleLoaded. I also tried to split out the copied over code into a new method but..

This is the copied part which isModuleLoaded uses from ImportInto method:

    nsresult rv;
    if (!mInitialized) {
        rv = ReallyInit();
        NS_ENSURE_SUCCESS(rv, rv);
    }

    nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);
    NS_ENSURE_SUCCESS(rv, rv);

    // Get the URI.
    nsCOMPtr<nsIURI> resURI;
    rv = ioService->NewURI(aLocation, nullptr, nullptr, getter_AddRefs(resURI));
    NS_ENSURE_SUCCESS(rv, rv);

    // figure out the resolved URI
    nsCOMPtr<nsIChannel> scriptChannel;
    rv = ioService->NewChannelFromURI(resURI, getter_AddRefs(scriptChannel));
    NS_ENSURE_SUCCESS(rv, NS_ERROR_INVALID_ARG);

    nsCOMPtr<nsIURI> resolvedURI;
    rv = scriptChannel->GetURI(getter_AddRefs(resolvedURI));
    NS_ENSURE_SUCCESS(rv, rv);

    nsAutoCString key;
    rv = resolvedURI->GetSpec(key);
    NS_ENSURE_SUCCESS(rv, rv);

    ModuleEntry* mod;

But the sad thing is that, ImportInto has a lot more dependency from this copied code. For that reason resURI, resolvedURI and key would have to be passed as reference to the private method, which would not be required by the isModuleLoaded method at all. Thus I did not move the common code into a private method.

Please suggest if this is correct. Thanks.

try build: https://tbpl.mozilla.org/?tree=Try&rev=0add57630bad

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8419760 [details] [diff] [review]
mem-is-loaded v2

I fixed the code duplication problems in bug 1008006. Please rebase your patch onto that.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: mem-is-loaded v3

Rebased.

https://tbpl.mozilla.org/?tree=Try&rev=3ac95c581d1e

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8420603 [details] [diff] [review]
mem-is-loaded v3

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

r=bholley, though this will need to wait for the dependent bug to land and go green (which will happen as soon as inbound reopens).

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1212,5 @@
>  
> +/* boolean isModuleLoaded (in AUTF8String registryLocation); */
> +NS_IMETHODIMP
> +mozJSComponentLoader::IsModuleLoaded(const nsACString& aLocation,
> +                                     bool* retval)

* on the right in XPConnect.

@@ +1228,5 @@
> +    rv = info.EnsureKey();
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ModuleEntry* mod;
> +    if (mImports.Get(info.Key(), &mod)) {

Can't you just do:

*retval = !!mImports.Get(info.Key());

?

::: js/xpconnect/src/XPCComponents.cpp
@@ +2726,5 @@
>  
> +/* boolean isModuleLoaded (in AUTF8String registryLocation);
> + */
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::IsModuleLoaded(const nsACString& registryLocation, bool* retval)

* on the right.

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: mem-is-loaded final

Addressed review comments.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a54ace627db8

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a54ace627db8