Closed Bug 1366402 Opened 3 years ago Closed 3 years ago

Use AutoMemMap in the component loader

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(1 file)

The plain file script loading code in mozJSComponentLoader duplicates a lot of the same file opening/mapping/cleanup logic that's implemented in the auto AutoMemMap RAII class used by other parts of the loader. Reusing it there would allow us to delete a lot of redundant code.
Comment on attachment 8869759 [details]
Bug 1366402: Use AutoMemMap helper in mozJSComponentLoader.

https://reviewboard.mozilla.org/r/141320/#review145618

Nice. I always just ignored that big chunk of code.

::: js/xpconnect/loader/ScriptPreloader-inl.h:24
(Diff revision 1)
>  
>  #include <prio.h>
>  
>  namespace mozilla {
> +
> +// A specialization of GenericErrorResult which auto-converts to a nsresult.

Maybe start the comment with XXX to indicate an issue that should be eventually fixed?

::: js/xpconnect/loader/mozJSComponentLoader.cpp:42
(Diff revision 1)
>  #include "xpcpublic.h"
>  #include "nsContentUtils.h"
>  #include "nsXULAppAPI.h"
>  #include "WrapperFactory.h"
>  
> +#include "AutoMemMap.h"

I think this should be mozilla/devtools/AutoMemMap.h based on the EXPORTS in moz.build.
Attachment #8869759 - Flags: review?(continuation) → review+
Reviewboard isn't showing this review, so replying in Bugzilla.

(In reply to Andrew McCreight [:mccr8] from comment #2)
> ::: js/xpconnect/loader/ScriptPreloader-inl.h:24
> (Diff revision 1)
> >  
> >  #include <prio.h>
> >  
> >  namespace mozilla {
> > +
> > +// A specialization of GenericErrorResult which auto-converts to a nsresult.
> 
> Maybe start the comment with XXX to indicate an issue that should be
> eventually fixed?

I should hopefully be able to get bug 1366511 landed in the next week or two,
so was planning to remove it in whichever patch landed second. I'll update the
comment with an XXX and bug number.

> ::: js/xpconnect/loader/mozJSComponentLoader.cpp:42
> (Diff revision 1)
> >  #include "xpcpublic.h"
> >  #include "nsContentUtils.h"
> >  #include "nsXULAppAPI.h"
> >  #include "WrapperFactory.h"
> >  
> > +#include "AutoMemMap.h"
> 
> I think this should be mozilla/devtools/AutoMemMap.h based on the EXPORTS in
> moz.build.

There's a separate AutoMemMap.h in the loader directory, based on but slightly
different from the devtools version, that I added in bug 1359653 (since I didn't
feel comfortable using devtools helpers in XPConnect code).

I'm planning to export this header and update the devtools code to use the
XPConnect version sometime soon.
Assignee: nobody → kmaglione+bmo
https://hg.mozilla.org/mozilla-central/rev/4d78fccf03b4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.