Use AutoMemMap in the component loader

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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+
(Assignee)

Comment 3

2 years ago
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

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d78fccf03b4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.