1358846 - Merge extensions.ini, extensions.xpiState, extensions.bootstrappedAddons, and extensions.enabledAddons into a single JSON file

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review135504 ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:427 (Diff revision 1) > + PropertyIter iter(cx, obj); > + MOZ_RELEASE_ASSERT(mAddons.reserve(iter.Length())); > + > + for (auto addonObj : iter) { > + JS::RootedValue value(cx, addonObj.Value()); > + > + if (value.isObject()) { > + Addon addon(*this, mCx, addonObj.Name(), &value.toObject()); > + mAddons.infallibleAppend(Move(addon)); > + } > + } This is all a bit less than ideal. I'd really rather have another iterator subclass that creates Addon instances rather than pre-create a vector of them, but the rooting requirements make it tricky. Trying to add another level of subclassing to the property iterators gave me a headache, and the only other option I had was to just duplicate all of the iterator code.

Comment hidden (mozreview-request)

Kris Maglione [:kmag]

Assignee

Comment 12

•

8 years ago

Some additional notes: My plan for the addonsStartup.json file is to eventually add more information that we may need early in startup for lazy initialization of add-ons. For WebExtensions, for instance, we'll probably need to store a list of content script and webRequest URL match patterns, to make sure that extensions are initialized in time to modify those resources. I'd also like to move the metadata we store in stage directories into this file, so we can get rid of those directory scans entirely, or deal with anything that's there unexpectedly during the sideload scan after startup is complete. My plan for the native add-on startup service is to make it easy for us to move expensive operations that we need at early startup between JS and C++ as necessary. Most of the code there at this point just makes it easy to access add-on data in the format it's stored in on the JS side.

Robert Helmer [:rhelmer]

Comment 13

•

8 years ago

mozreview-review

Comment on attachment 8860706 [details] Bug 1358846: Part 1 - Remove old database migration code. https://reviewboard.mozilla.org/r/132644/#review135856 ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js (Diff revision 1) > - * > - * @param aLiteral > - * The RDF object to convert > - * @return a string if the object could be converted or null > - */ > -function getRDFValue(aLiteral) { Isn't this used by `loadManifestFromRDF`?

Attachment #8860706 - Flags: review?(rhelmer)

Kris Maglione [:kmag]

Assignee

•

8 years ago

mozreview-review

Comment on attachment 8860751 [details] Bug 1358846: Part 5 - Clean up some path manipulation code. https://reviewboard.mozilla.org/r/132678/#review135982

Attachment #8860751 - Flags: review?(rhelmer) → review+

Kris Maglione [:kmag]

Assignee

Comment 20

•

8 years ago

mozreview-review-reply

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review135970 > Hm. Isn't this going to fail if `manifest.theme` is defined? Nope. Promise.all() immediately returns the value of mPromise. Since all we care about is that we only wait for promiseAddonStartup() for non-themes, that does exactly what we need.

Kris Maglione [:kmag]

Assignee

Comment 21

•

8 years ago

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. Tom, would you mind taking a look at the JSAPI portions of this? Some of the iterator stuff is a little hairy, so if you have any ideas about how to handle it better, I'd be happy to hear them.

Attachment #8860709 - Flags: review?(evilpies)

Tom S [:evilpie]

Comment 22

•

8 years ago

mozreview-review

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review136192 ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:233 (Diff revision 2) > + } > + > + if (val.isNumber()) { > + return val.toNumber(); > + } > + if (val.isInt32()) { isNumber already means isDouble or isInt32.

Attachment #8860709 - Flags: review?(evilpies)

Tom S [:evilpie]

Comment 23

•

8 years ago

Jon is going to pick up this review, because there are some problem with the Rooting/GC.

Jon Coppeard (:jonco)

Comment 24

•

8 years ago

mozreview-review

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review136200 This is mostly fine apart from the use of Heap<> and CustomAutoRooter. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:32 (Diff revision 2) > +public: > + typedef T SelfType; > + > + PropertyType begin() const > + { > + PropertyType elem(*static_cast<const SelfType*>(this)); suggestion: factor out |static_cast<const SelfType*>(this)| into a separate method here and in BaseIterElem. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:70 (Diff revision 2) > + return mIter.Length(); > + } > + > + JS::Value Value() > + { > + JS::RootedValue value(mIter.mCx, JS::UndefinedValue()); Maybe assert mIndex < Lenth() here too. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:128 (Diff revision 2) > + PropertyIter(JSContext* cx, JS::HandleObject object) > + : BaseIter(cx, object) > + , mIds(cx, JS::IdVector(cx)) > + { > + if (!JS_Enumerate(cx, object, &mIds)) { > + JS_ClearPendingException(cx); Is it OK to ignore failures here? I guess so as this happens in a bunch of places below. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:233 (Diff revision 2) > + > +protected: > + bool > + GetValue(JS::MutableHandleValue value) > + { > + return JS_GetElement(mIter.mCx, mIter.mObject, mIndex, value); Can we assert the index is in range here? ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:193 (Diff revision 2) > + JS::TraceEdge(trc, &mObject, "addon object"); > + } > + > +protected: > + JSContext* mCx; > + JS::Heap<JSObject*> mObject; JS::Heap doesn't go in a stack class. Can you use Rooted<JSObject*> here instead? ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:275 (Diff revision 2) > + } > + return nullptr; > +} > + > +template<class T> > +class MOZ_RAII RootedWrapper final : public T, public JS::CustomAutoRooter Please don't use CustomAutoRooter. You should be able to use Rooted<JSObject*> in the base class to take care of rooting objects. ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:302 (Diff revision 2) > + if (!JS_SetProperty(mCx, obj, "changed", val)) { > + JS_ClearPendingException(mCx); > + } > + } > + > + JS::GCVector<Addon>& Addons() { return mAddons.get(); } Can you make this return a const reference? ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:318 (Diff revision 2) > +typedef RootedWrapper<InstallLocation> RootedInstallLocation; > + > + > +class MOZ_STACK_CLASS Addon : public WrapperBase { > +public: > + Addon(InstallLocation& location, JSContext* cx, const nsAString& id, JSObject* object) nit: in js/src the style is to always pass JSontext* as the first arugment but that may not apply here.

Bug 1358846: Part 1 - Remove old database migration code. 8 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	rhelmer : review+	Details
Bug 1358846: Part 2 - Allow using file compression with JSONFile.jsm. 8 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	rhelmer : review+	Details
Bug 1358846: Part 3 - Trivial cleanups. 8 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	rhelmer : review+	Details
Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. 8 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	rhelmer : review+ jonco : review+	Details
Bug 1358846: Part 5 - Clean up some path manipulation code. 8 years ago Kris Maglione [:kmag] 59 bytes, text/x-review-board-request	rhelmer : review+	Details