568691 - (data-driven-compreg) Use manifests and data tables to register XPCOM components

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

We're solving two problems:
* avoid the restart required by the extension manager after changing extensions
* fast startup of content processes in Fennec without a fastload cache

To this end, I want to make XPCOM component registration data-driven. Static/binary components will export a single data symbol "NSModule". Other components (JS/Python/whatever) will register themself in chrome.manifest with these lines:

cid file.js
contractid cid
category cname key value

Implementation details:
the chrome registry will end up linked into XPCOM so it can share manifest parsing code at a low level

Comment 1

[Benjamin Smedberg]

Attachment: Checkpoint

Comment 2

[Brendan Eich [:brendan]]

Frozen API breakage, I love it.

Formally, by the book, and without making anyone cringe about Tamarin's demise or whatever, we should pull the Mozilla, or really Gecko rv: 2.0.x trigger. Why not?

/be

Comment 3

[Benjamin Smedberg]

Yes. I'm not sure what that means in practice: I could rename/remove xpcom.dll. Does that also mean revving the gecko version to 2.0 from 1.9.3?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Yes. I'm not sure what that means in practice: I could rename/remove xpcom.dll.
> Does that also mean revving the gecko version to 2.0 from 1.9.3?

The Apple guys did WebKit2 (framework name change motivated by API and even ABI breakage) for that reason.

We should definitely talk about Gecko rv: changing. It's not a requirement that we go to "2.0" when we break @status FROZEN APIs but it does keep that promise and follow through in a way that has user-agent sniffing meaning. Maybe too much meaning tho -- would it break sniffers or sites?

/be

Comment 5

[Benjamin Smedberg]

Attachment: Checkpoint 2

Checkpoint: moving the binary registration bits around is done, and this builds completely on Windows (doesn't work).

I think I'm going to hook up static/binary registration, split this into the important bits and the makework module changes, and put those parts up for review. I'll layer the bits for JS registration as a separate patch on top.

Comment 6

[Benjamin Smedberg]

Attachment: Part A, revision 1 - the important bits

This bug will be divided up into the following patches:
A - the important core bits necessary to get static/binary components registering and working
B - mechanical changes to the in-tree binary/static components to get them registering and working correctly (xpcshell works)
C - changes to .manifest file loading so that components can be registered in manifest files
D - mechanical changes to in-tree JS components to be registered correctly
E - changes for backwards compatibility so the binary components can implement the old-style NSGetModule as well as the new-style NSModule
F... I'm developing this on Windows, so it's likely that non-Windows fixes will need to be introduced. Rather than refresh A-E, I'm going to tack those fixes on top.

Comment 7

[Benjamin Smedberg]

Attachment: Part B - mechanical binary component fixup

Comment 8

[David Ascher (:davida)]

bsmedberg: was the mechanical fixup generated with code, and if so, can you share for application in c-c?

Comment 9

[Benjamin Smedberg]

Attachment: Emacs macros for splitting binary module info

dascher, I did the splitting using the emacs macros that I've attached. They are finicky but good enough for me. I hope the comments I've added are sufficient to get you started.

Also, you should "M-x set-variable truncate-lines t" so that extra long lines don't screw up the macros.

I welcome somebody making this better.

And I've committed my "final" patches as http://hg.mozilla.org/users/bsmedberg_mozilla.com/static-xpcom-registration and I'll be committing review comments as followup patches, so you should be ok to pull that repo and build c-c against it. (JS components not working yet)

Comment 10

[Benjamin Smedberg]

Attachment: Part C - load and parse .manifest files from the components directory

This successfully loads nsSample.js component. It has the unfortunate side effect of not loading any chrome manifests, which I'll fix in a bit. It doesn't have any of the actual translation of existing JS components to manifest files, or the build machinery which will make that not suck.

Comment 11

[Benjamin Smedberg]

Attachment: Part C, rev. 2 - use .manifest files

Comment 12

[Dave Townsend [:mossop]]

From today's platform meeting we should consider whether this needs to ship with beta1 or not.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 450177 [details] [diff] [review]
Part A, revision 1 - the important bits

>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp
>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>@@ -200,16 +200,17 @@ static NS_DEFINE_CID(kXTFServiceCID, NS_

>+  if (nsHTMLMediaElement::IsOggEnabled()) {
>+    for (int i = 0; i < NS_ARRAY_LENGTH(nsHTMLMediaElement::gOggTypes); ++i) {
>+      const char* type = nsHTMLMediaElement::gOggTypes[i];
>+      if (!strcmp(aType, type)) {
>+        docFactory = do_GetService("@mozilla.org/content/document-loader-factory;1");
>+        if (docFactory && aLoaderType) {
>+          *aLoaderType = TYPE_CONTENT;
>+        }
>+        return docFactory.forget();
>+      }
>+    }
>+  }

At the very least this needs to be surrounded with ifdef MOZ_MEDIA. Maybe MOZ_OGG unless you do something different with the ifdefs I mention below.

>diff --git a/content/html/content/public/nsHTMLMediaElement.h b/content/html/content/public/nsHTMLMediaElement.h

>+  static bool IsOggEnabled();
>+  static bool IsOggType(const nsACString& aType);
>+  static const char gOggTypes[3][16];
>+  static char const *const gOggCodecs[3];
>+
>+  static bool IsWaveEnabled();
>+  static bool IsWaveType(const nsACString& aType);
>+  static const char gWaveTypes[4][16];
>+  static char const *const gWaveCodecs[2];

You need to ifdef MOZ_OGG and MOZ_WAVE the appropriate bits here, either that or always implement them and have the implementations behave sensibly in either case.

>diff --git a/dom/src/json/nsJSON.cpp b/dom/src/json/nsJSON.cpp
>--- a/dom/src/json/nsJSON.cpp
>+++ b/dom/src/json/nsJSON.cpp
>@@ -509,17 +509,17 @@ nsJSON::DecodeInternal(nsIInputStream *a
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = cc->SetReturnValueWasSet(PR_TRUE);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return NS_OK;
> }
> 
>-NS_IMETHODIMP
>+nsresult
> NS_NewJSON(nsISupports* aOuter, REFNSIID aIID, void** aResult)

There are lots of these changes throughout the patch, though doesn't seem to be over the entire tree. What is the reason for them?

>diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp

>     nsAutoPtr<ModuleEntry> entry(new ModuleEntry);
>     if (!entry)
>-        return NS_ERROR_OUT_OF_MEMORY;
>+        return NULL;

Not required but this might be shorter as NS_ENSURE_TRUE(entry, NULL).
Some of the code here uses braces around a single statement in the if block, some don't. This might be a good time to make that consistent.

>     nsCOMPtr<nsIXPConnect> xpc = do_GetService(kXPConnectServiceContractID,
>                                                &rv);
>     if (NS_FAILED(rv))
>-        return rv;
>+        return NULL;

Could make this and a bunch of others in this method a little shorter with NS_ENSURE_SUCCESS(rv, NULL)

>diff --git a/layout/build/nsLayoutModule.cpp b/layout/build/nsLayoutModule.cpp

> // static
> nsresult
>-Initialize(nsIModule* aSelf)
>+Initialize()
> {
>-  NS_PRECONDITION(!gInitialized, "module already initialized");
>   if (gInitialized) {
>-    return NS_OK;
>+    NS_ERROR("Recursive layout module initialization");
>+    return NS_ERROR_FAILURE;
>   }
> 
>   NS_ASSERTION(sizeof(PtrBits) == sizeof(void *),
>                "Eeek! You'll need to adjust the size of PtrBits to the size "
>                "of a pointer on your platform.");
> 
>   gInitialized = PR_TRUE;
> 
>-  nsresult rv = nsLayoutStatics::Initialize();
>+  nsresult rv;
>+  rv = xpcModuleCtor();
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  rv = nsLayoutStatics::Initialize();

Seems kind of weird to be initializing xpconnect from layout.

>+static const mozilla::Module::CIDEntry kLayoutCIDs[] = {
>+  XPCONNECT_CIDENTRIES

... SNIP ...

>+  { &kNS_SECURITYNAMESET_CID, false, NULL, nsSecurityNameSetConstructor },
>+  { NULL }
> };

The notes in Module.h say this should end with { NULL, false }

>+static const mozilla::Module::ContractIDEntry kLayoutContracts[] = {
>+  XPCONNECT_CONTRACTS
... SNIP ...

>+  { NS_SECURITYNAMESET_CONTRACTID, &kNS_SECURITYNAMESET_CID },
>+  { NULL }
> };

And this should end with { NULL, NULL }

>+static const mozilla::Module::CategoryEntry kLayoutCategories[] = {
>+  XPCONNECT_CATEGORIES
>+  { JAVASCRIPT_GLOBAL_CONSTRUCTOR_CATEGORY, "Image", NS_HTMLIMGELEMENT_CONTRACTID },
>+  { JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_ALIAS_CATEGORY, "Image", "HTMLImageElement" },
>+  { JAVASCRIPT_GLOBAL_CONSTRUCTOR_CATEGORY, "Option", NS_HTMLOPTIONELEMENT_CONTRACTID },
>+  { JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_ALIAS_CATEGORY, "Option", "HTMLOptionElement" },
>+#ifdef MOZ_MEDIA
>+  { JAVASCRIPT_GLOBAL_CONSTRUCTOR_CATEGORY, "Audio", NS_HTMLAUDIOELEMENT_CONTRACTID },
>+  { JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_ALIAS_CATEGORY, "Audio", "HTMLAudioElement" },
>+#endif
>+  { "content-policy", NS_DATADOCUMENTCONTENTPOLICY_CONTRACTID, NS_DATADOCUMENTCONTENTPOLICY_CONTRACTID },
>+  { "content-policy", NS_NODATAPROTOCOLCONTENTPOLICY_CONTRACTID, NS_NODATAPROTOCOLCONTENTPOLICY_CONTRACTID },
>+  { "content-policy", "CSPService", CSPSERVICE_CONTRACTID },
>+  { "net-channel-event-sinks", "CSPService", CSPSERVICE_CONTRACTID },
>+  { JAVASCRIPT_GLOBAL_STATIC_NAMESET_CATEGORY, "PrivilegeManager", NS_SECURITYNAMESET_CONTRACTID },
>+  { "app-startup", "Script Security Manager", "service," NS_SCRIPTSECURITYMANAGER_CONTRACTID },
>+  CONTENTDLF_CATEGORIES
>+  { NULL }
> };

And this { NULL, NULL, NULL }. I'm guessing it doesn't make a difference so maybe update the notes in Module.h?

Also I should hold up my hands and say I'm not thoroughly verifying these lists. Were they generated by hand? Are we able to get a dump of the component registry and category registry from before/after these patches and verify that they contain the same info?

>diff --git a/toolkit/library/nsStaticXULComponents.cpp b/toolkit/library/nsStaticXULComponents.cpp
>--- a/toolkit/library/nsStaticXULComponents.cpp
>+++ b/toolkit/library/nsStaticXULComponents.cpp
>@@ -35,22 +35,22 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #define XPCOM_TRANSLATE_NSGM_ENTRY_POINT 1
> 
>-#include "nsIGenericFactory.h"
>+#include "mozilla/Module.h"
> #include "nsXPCOM.h"
> #include "nsStaticComponents.h"
> #include "nsMemory.h"
> 
>-#define NSGETMODULE(_name) _name##_NSGetModule
>+// #define NSGETMODULE(_name) _name##_NSGetModule

No reason to leave this in now is there?

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>@@ -651,28 +651,30 @@ nsXREDirProvider::GetFilesInternal(const
>     LoadBundleDirectories();
>     LoadDirsIntoArray(mAppBundleDirectories,
>                       kAppendNothing, directories);
>     LoadDirsIntoArray(mExtensionDirectories,
>                       kAppendNothing, directories);
> 
>     rv = NS_NewArrayEnumerator(aResult, directories);
>   }
>+#if 0
>   else if (!strcmp(aProperty, NS_XPCOM_COMPONENT_DIR_LIST)) {
>     static const char *const kAppendCompDir[] = { "components", nsnull };
>     nsCOMArray<nsIFile> directories;
> 
>     LoadBundleDirectories();
>     LoadDirsIntoArray(mAppBundleDirectories,
>                       kAppendCompDir, directories);
>     LoadDirsIntoArray(mExtensionDirectories,
>                       kAppendCompDir, directories);
> 
>     rv = NS_NewArrayEnumerator(aResult, directories);
>   }
>+#endif

Any reason to leave this in?

Checkpoint: At the end of xpcom/build

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 450177 [details] [diff] [review]
Part A, revision 1 - the important bits

>diff --git a/xpcom/components/GenericFactory.cpp b/xpcom/components/GenericFactory.cpp

>+NS_IMETHODIMP
>+GenericFactory::LockFactory(PRBool aLock)
>+{
>+  NS_ERROR("Vestigial method, never called!");
>+  return NS_ERROR_FAILURE;
>+}

We're breaking everything else, why not get rid of this while we're at it?

>diff --git a/xpcom/components/Module.h b/xpcom/components/Module.h

>+struct Module
>+{
>+  static const int kVersion = 1;
>+
>+  struct CIDEntry;
>+
>+  typedef already_AddRefed<nsIFactory> (*GetFactoryProc)
>+    (const Module& module, const CIDEntry& entry);
>+
>+  typedef nsresult (*ConstructorProc)(nsISupports* aOuter,
>+                                      const nsIID& aIID,
>+                                      void** aResult);
>+
>+  typedef nsresult (*LoadedFunc)();
>+  typedef void (*UnloadedFunc)();
>+
>+  /**
>+   * The constructor callback is an implementation detail of the default binary
>+   * loader and may be null.
>+   */
>+  struct CIDEntry
>+  {
>+    const nsCID* cid;
>+    bool service;

What is this used for, I can't spot it in use anywhere and every component and service seems to have it set to false.

>diff --git a/xpcom/components/nsCategoryManager.cpp b/xpcom/components/nsCategoryManager.cpp

>-nsCategoryManager*
>-nsCategoryManager::Create()
>+NS_IMETHODIMP_(nsrefcnt)
>+nsCategoryManager::AddRef()
> {
>-  nsCategoryManager* manager = new nsCategoryManager();
>-  
>-  if (!manager)
>-    return nsnull;
>+  return 2;
>+}
> 
>-  PL_INIT_ARENA_POOL(&(manager->mArena), "CategoryManagerArena",
>-                     NS_CATEGORYMANAGER_ARENA_SIZE); // this never fails
>+NS_IMETHODIMP_(nsrefcnt)
>+nsCategoryManager::Release()
>+{
>+  return 1;
>+}

This seems nasty.

>diff --git a/xpcom/io/nsDirectoryServiceDefs.h b/xpcom/io/nsDirectoryServiceDefs.h
>--- a/xpcom/io/nsDirectoryServiceDefs.h
>+++ b/xpcom/io/nsDirectoryServiceDefs.h
>@@ -70,48 +70,30 @@
>                                                                                                                        
> /* This location is similar to NS_OS_CURRENT_PROCESS_DIR, however, 
>  * NS_XPCOM_CURRENT_PROCESS_DIR can be overriden by passing a "bin
>  * directory" to NS_InitXPCOM2(). 
>  */
> #define NS_XPCOM_CURRENT_PROCESS_DIR            "XCurProcD"
> 
> /* Property will return the location of the application components
>- * directory.  By default, this directory will be contained in the 
>- * NS_XPCOM_CURRENT_PROCESS_DIR.
>- */
>-#define NS_XPCOM_COMPONENT_DIR                  "ComsD"
>-
>-/* Property will return a list of components directories that will
>- * will be registered after the application components directory.
>- */
>-#define NS_XPCOM_COMPONENT_DIR_LIST             "ComsDL"
>-
>-/* Property will return the location of the application components
>  * registry file.
>  */
> #define NS_XPCOM_COMPONENT_REGISTRY_FILE        "ComRegF"

This isn't necessary anymore either is it?

Still to do: nsComponentManager.cpp and nsComponentManager.h

Comment 15

[Benjamin Smedberg]

> >-NS_IMETHODIMP
> >+nsresult
> > NS_NewJSON(nsISupports* aOuter, REFNSIID aIID, void** aResult)
> 
> There are lots of these changes throughout the patch, though doesn't seem to be
> over the entire tree. What is the reason for them?

I changed the signature of the constructor function from stdcall to default (cdecl). Most places use NS_GENERIC_FACTORY_CONSTRUCTOR, but I had to change the declaration of the hand-rolled ones.

> Not required but this might be shorter as NS_ENSURE_TRUE(entry, NULL).

I forbid NS_ENSURE_TRUE in my modules because it hides return statements.

> Seems kind of weird to be initializing xpconnect from layout.

xpconnect is linked into layout already. Currently they keep two separate nsIModules and aggregate them, but this is better and simpler.

> 
> >+static const mozilla::Module::CIDEntry kLayoutCIDs[] = {
> >+  XPCONNECT_CIDENTRIES
> 
> ... SNIP ...
> 
> >+  { &kNS_SECURITYNAMESET_CID, false, NULL, nsSecurityNameSetConstructor },
> >+  { NULL }
> > };
> 
> The notes in Module.h say this should end with { NULL, false }

Oh, well I've been using { NULL } throughout, since C++ 0-fills the rest of the structure. I'll change the docs in Module.h.

> Also I should hold up my hands and say I'm not thoroughly verifying these
> lists. Were they generated by hand? Are we able to get a dump of the component
> registry and category registry from before/after these patches and verify that
> they contain the same info?

They were generated by running the emacs macros in another attachment on this bug: I'll look into doing some sort of A/B verification, but it might not be completely simple.

Comment 16

[Benjamin Smedberg]

> >+NS_IMETHODIMP
> >+GenericFactory::LockFactory(PRBool aLock)
> >+{
> >+  NS_ERROR("Vestigial method, never called!");
> >+  return NS_ERROR_FAILURE;
> >+}
> 
> We're breaking everything else, why not get rid of this while we're at it?

I'm trying to avoid changing nsIFactory for FF4 so that binary components can be made compatible with FF3.6 and 4 still. I'm actually planning on reverting the nsIComponentRegistrar changes (the obsolete methods will assert/return NS_ERROR_NOT_IMPLEMENTED).

> >+  struct CIDEntry
> >+  {
> >+    const nsCID* cid;
> >+    bool service;
> 
> What is this used for, I can't spot it in use anywhere and every component and
> service seems to have it set to false.

This is for future implementation of a real service/component distinction, so that you may not .createInstance something with the service flag set to true. I'm not planning on implementing it in this patch, but I thought it would be easier to add the flag now, rather than doing another treewide change later. The default `false` value will work exactly as today (can be .createInstance or .getServiced).

> >+NS_IMETHODIMP_(nsrefcnt)
> >+nsCategoryManager::Release()
> >+{
> >+  return 1;
> >+}
> 
> This seems nasty.

Why? We do this a fair bit for special components that have static lifetime and don't actually need to be refcounted.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 450177 [details] [diff] [review]
Part A, revision 1 - the important bits

>diff --git a/xpcom/components/Module.h b/xpcom/components/Module.h

>+  /**
>+   * When the component manager tries to get the factory for a CID, it first
>+   * checks for this module-level getfactory callback. If this function is
>+   * not implemented, it checks the CIDEntry getfactory callback. If that is
>+   * also NULL, a generic factory is generated using the CIDEntry constructor
>+   * callback which must be non-NULL.
>+   */
>+  GetFactoryProc getfactory;

Any reason for not following the normal style of GetFactory?

>+  /**
>+   * Optional Function which are called when this module is loaded and
>+   * at shutdown. These are not C++ constructor/destructors to avoid
>+   * calling them too early in startup or too late in shutdown.
>+   */
>+  LoadedFunc loaded;
>+  UnloadedFunc unloaded;

These names look confusing, as if they are bools. How about just Load and Unload?

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>+already_AddRefed<nsIFactory>
>+nsFactoryEntry::GetFactory()
> {
>     if (!mFactory) {
>-        nsresult rv;
>+        // RegisterFactory then UnregisterFactory can leave an entry in mContractIDs
>+        // pointing to an unusable nsFactoryEntry.
>+        if (!mModule)
>+            return NULL;
> 
>-        if (mLoaderType == NS_LOADER_TYPE_INVALID)
>-            return NS_ERROR_FAILURE;
>+        if (!mModule->Load())
>+            return NULL;
> 
>-        nsCOMPtr<nsIModule> module;
>-
>-        if (mLoaderType == NS_LOADER_TYPE_STATIC) {
>-            rv = nsComponentManagerImpl::gComponentManager->
>-                mStaticModuleLoader.
>-                GetModuleFor(mLocationKey,
>-                             getter_AddRefs(module));
>+        if (mModule->Module()->getfactory) {
>+            mFactory = mModule->Module()->getfactory(*mModule->Module(),
>+                                                    *mCIDEntry);
>+        }
>+        if (mCIDEntry->getfactory) {
>+            mFactory = mCIDEntry->getfactory(*mModule->Module(), *mCIDEntry);
>         }

Shouldn't that be an else if?

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 450177 [details] [diff] [review]
Part A, revision 1 - the important bits

>diff --git a/xpcom/components/nsICategoryManager.idl b/xpcom/components/nsICategoryManager.idl
>--- a/xpcom/components/nsICategoryManager.idl
>+++ b/xpcom/components/nsICategoryManager.idl
>@@ -38,56 +38,28 @@
> #include "nsISupports.idl"
> #include "nsISimpleEnumerator.idl"
> 
> /*
>  * nsICategoryManager
>  * @status FROZEN
>  */
> 
>-[scriptable, uuid(3275b2cd-af6d-429a-80d7-f0c5120342ac)]
>+[scriptable, uuid(70F95BAF-8F93-4D24-B9E0-3428DF27A835)]
> interface nsICategoryManager : nsISupports
> {
>     /**
>      * Get the value for the given category's entry.
>      * @param aCategory The name of the category ("protocol")
>      * @param aEntry The entry you're looking for ("http")
>      * @return The value.
>      */
>     string getCategoryEntry(in string aCategory, in string aEntry);
>     
>     /**
>-     * Add an entry to a category.
>-     * @param aCategory The name of the category ("protocol")
>-     * @param aEntry The entry to be added ("http")
>-     * @param aValue The value for the entry ("moz.httprulez.1")
>-     * @param aPersist Should this data persist between invocations?
>-     * @param aReplace Should we replace an existing entry?
>-     * @return Previous entry, if any
>-     */
>-    string addCategoryEntry(in string aCategory, in string aEntry,
>-			    in string aValue, in boolean aPersist,
>-			    in boolean aReplace);
>-
>-    /**
>-     * Delete an entry from the category.
>-     * @param aCategory The name of the category ("protocol")
>-     * @param aEntry The entry to be added ("http")
>-     * @param aPersist Delete persistent data from registry, if present?
>-     */
>-    void deleteCategoryEntry(in string aCategory, in string aEntry,
>-                               in boolean aPersist);
>-
>-    /**
>-     * Delete a category and all entries.
>-     * @param aCategory The category to be deleted.
>-     */
>-    void deleteCategory(in string aCategory);
>-
>-    /**
>      * Enumerate the entries in a category.
>      * @param aCategory The category to be enumerated.
>      * @return a simple enumerator, each result QIs to
>      *         nsISupportsCString.
>      */
>     nsISimpleEnumerator enumerateCategory(in string aCategory);
> 
>     /**

So script can no longer change the category entries?

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 450177 [details] [diff] [review]
Part A, revision 1 - the important bits

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>+nsComponentManagerImpl::UnregisterFactory(const nsCID& aClass,
>+                                          nsIFactory* aFactory)
> {
>+    nsAutoMonitor mon(mMon);
>+    nsFactoryEntry* f = mFactories.Get(aClass);
>+    if (!f || f->mFactory != aFactory)
>+        return NS_ERROR_FACTORY_NOT_REGISTERED;
>+    // This might leave a stale contractid -> factory mapping in place, so null
>+    // out the factory entry (see nsFactoryEntry::GetFactory)
>+    f->mFactory = NULL;
>+    
>+    return NS_OK;
> }

Don't you need to null out f->mModule too to stop us trying to reload it?

Comment 20

[Benjamin Smedberg]

Category entries can no longer be dynamically changed, yes.
I don't need to null f->mModule because it will already be null (RegisterFactory doesn't set mModule to anything, because there is no module).

Comment 21

[Benjamin Smedberg]

Comment 13/14/17 addressed in http://hg.mozilla.org/users/bsmedberg_mozilla.com/static-xpcom-registration/rev/34a044089fae except for the #if 0 which I'm going to fix in the patch to make extension chrome/components work correctly.

Comment 22

[Alex Vincent [:WeirdAl]]

(In reply to comment #20)
> Category entries can no longer be dynamically changed, yes.

Peanut gallery observation:  does this mean categories like "JavaScript global property", and others listed in nsIScriptNameSpaceManager.h, we can no longer add JS-based components for?

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> (In reply to comment #20)
> > Category entries can no longer be dynamically changed, yes.
> 
> Peanut gallery observation:  does this mean categories like "JavaScript global
> property", and others listed in nsIScriptNameSpaceManager.h, we can no longer
> add JS-based components for?

It means you can't add them dynamically at runtime but I expect you wouldn't want to do that anyway. They can still be registered in the manifests at startup

Comment 24

[Benjamin Smedberg]

Attachment: build components.manifest instead of components.list, rev. 1

Comment 25

[Benjamin Smedberg]

Attachment: Fix XPCOMUtils, rev. 1

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 450732 [details] [diff] [review]
Part C, rev. 2 - use .manifest files

>diff --git a/chrome/src/nsChromeRegistry.cpp b/chrome/src/nsChromeRegistry.cpp

>+void
>+nsChromeRegistry::ManifestContent(ManifestProcessingContext& cx, int lineno,
>+                                  char *const * argv, bool platform, bool contentaccessible)

I think it would be clearer if this were named RegisterManifestContent, same for the others.

>+void
>+nsChromeRegistry::ManifestLocale(ManifestProcessingContext& cx, int lineno,
>+                                 char *const * argv, bool platform, bool contentaccessible)
>+{
>+  char* package = argv[0];
>+  char* provider = argv[1];

Can you use locale as the variable name here.

>+void
>+nsChromeRegistry::ManifestSkin(ManifestProcessingContext& cx, int lineno,
>+                               char *const * argv, bool platform, bool contentaccessible)
>+{
>+  char* package = argv[0];
>+  char* provider = argv[1];

Maybe skin or something else more descriptive as the name here.

>+void
>+nsChromeRegistry::ManifestResource(ManifestProcessingContext& cx, int lineno,
>+                                   char *const * argv, bool platform, bool contentaccessible)
>+{
>+  char* package = argv[0];
>+  char* uri = argv[1];
>+
>+  EnsureLowerCase(package);
>+  nsDependentCString host(package);
>+
>+  nsCOMPtr<nsIIOService> io = mozilla::services::GetIOService();
>+  if (!io) {
>+    NS_WARNING("No IO service trying to process chrome manifests");
>+    return;
>+  }
> 
>   nsCOMPtr<nsIProtocolHandler> ph;
>+  nsresult rv = io->GetProtocolHandler("resource", getter_AddRefs(ph));
>+  if (NS_FAILED(rv))
>+    return;
>   
>+  nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);
> 
>+  PRBool exists = PR_FALSE;
>+  rv = rph->HasSubstitution(host, &exists);
>+  if (exists) {
>+    LogMessageWithContext(cx.GetManifestURI(), lineno, nsIScriptError::warningFlag,
>+                          "Duplicate resource declaration for '%s' ignored.", package);
>+    return;
>   }

I realise this was there originally but I wonder if this is overly restrictive. I can imagine cases where an extensions might want to override a resource mapping from the application (I believe weave wants to be able to do this in fact). What do you think about dropping it?

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 450732 [details] [diff] [review]
Part C, rev. 2 - use .manifest files

>diff --git a/xpcom/components/ManifestParser.cpp b/xpcom/components/ManifestParser.cpp

>+void
>+ParseManifest(NSLocationType aType, nsILocalFile* aFile, char* buf)
>+{
>+  nsresult rv;
>+
>+  nsComponentManagerImpl::ManifestProcessingContext mgrcx(aType, aFile);
>+  nsChromeRegistry::ManifestProcessingContext chromecx(aType, aFile);

Is there a reason we can't just have a single shared one of these?

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 450732 [details] [diff] [review]
Part C, rev. 2 - use .manifest files

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>+void
>+nsComponentManagerImpl::ManifestComponent(ManifestProcessingContext& cx, int lineno, char *const * argv)
>+{
>+    char* id = argv[0];
>+    char* file = argv[1];
>+
>+    nsID cid;
>+    if (!cid.Parse(id)) {
>+        // XXX report parse error
>+        return;
>+    }

One of the comments made on my blog were that other uses of GUID in the manifest files require the full {...} form (like application={...}). I think it would make sense to use the full form for the CID as well for consistency.

Comment 29

[Benjamin Smedberg]

The chromereg processing context and the compreg processing context contain different information, and when we're first registering components chromereg may not be registered yet. Otherwise you'd need friend declarations or something. This way seemed simpler, although I can change it if you really think it's better.

nsID::Parse takes either the {} or the non-{} form. I really don't want to go hacking some requirement for {} into it (my current patch for JS components uses the non-{} form). The only reason application={} required the {} is because we're reading it as a string, not as a GUID.

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #29)
> nsID::Parse takes either the {} or the non-{} form. I really don't want to go
> hacking some requirement for {} into it (my current patch for JS components
> uses the non-{} form). The only reason application={} required the {} is
> because we're reading it as a string, not as a GUID.

Not requiring it seems fine, I would prefer it if the manifests we generate include it though.

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 452790 [details] [diff] [review]
Fix XPCOMUtils, rev. 1

># HG changeset patch
># User Benjamin Smedberg <[e-mail]>
># Date 1277144972 14400
># Node ID b9a87d218a736c1fd1be4a66a77d19f28299a669
># Parent  5e71dd583552ec7f772dc6cca137273b12fe524c
>Bug 568691 - Fix XPCOMUtils.jsm to generate NSGetFactory, and fix nsSample.js to use XPCOMUtils
>
>diff --git a/js/src/xpconnect/loader/XPCOMUtils.jsm b/js/src/xpconnect/loader/XPCOMUtils.jsm

This all looks fine. One thought that occurs though is that an alternate would be to allow the js component to do something like:

XPCOMUtils.addXPCOMSymbols(this, components)

Which would then define the NSGetFactory in the scope and anything else needed. This would perhaps protect us from future changes, though I hope that is unlikely.

>diff --git a/xpcom/sample/nsSample.js b/xpcom/sample/nsSample.js

>+/**
>+ * XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
>+ * XPCOMUtils.generateNSGetModule is for Mozilla 1.9.2 (Firefox 3.6).
>+ */
>+if (XPCOMUtils.generateNSGetFactory)
>+    NSGetFactory = XPCOMUtils.generateNSGetFactory([mySample]);
>+else
>+    NSGetModule = XPCOMUtils.generateNSGetModule([mySample]);

You need to declare these variables with var or let.

Comment 32

[Brendan Eich [:brendan]]

(In reply to comment #31)
> >+/**
> >+ * XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
> >+ * XPCOMUtils.generateNSGetModule is for Mozilla 1.9.2 (Firefox 3.6).
> >+ */
> >+if (XPCOMUtils.generateNSGetFactory)
> >+    NSGetFactory = XPCOMUtils.generateNSGetFactory([mySample]);
> >+else
> >+    NSGetModule = XPCOMUtils.generateNSGetModule([mySample]);
> 
> You need to declare these variables with var or let.

JS lets you common more aggressively than C or C++:

  let NSGetFactory = XPCOMUtils[XPCOMUtils.generateNSGetFactory ? "generateNSGetFactory" : "generateNSGetModule"]([mySample]);

which is fast enough as is, assuming this is a one-time initialization.

/be

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #32)
> (In reply to comment #31)
> > >+/**
> > >+ * XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
> > >+ * XPCOMUtils.generateNSGetModule is for Mozilla 1.9.2 (Firefox 3.6).
> > >+ */
> > >+if (XPCOMUtils.generateNSGetFactory)
> > >+    NSGetFactory = XPCOMUtils.generateNSGetFactory([mySample]);
> > >+else
> > >+    NSGetModule = XPCOMUtils.generateNSGetModule([mySample]);
> > 
> > You need to declare these variables with var or let.
> 
> JS lets you common more aggressively than C or C++:
> 
>   let NSGetFactory = XPCOMUtils[XPCOMUtils.generateNSGetFactory ?
> "generateNSGetFactory" : "generateNSGetModule"]([mySample]);
> 
> which is fast enough as is, assuming this is a one-time initialization.

But doesn't work because the symbol names need to be different.

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 450178 [details] [diff] [review]
Part B - mechanical binary component fixup

This all appears to be correct, it all starts to look the same after a while though.

>diff --git a/intl/uconv/src/nsUConvModule.cpp b/intl/uconv/src/nsUConvModule.cpp

>+static const mozilla::Module kUConvModule = {
>+  mozilla::Module::kVersion,
>+  kUConvCIDs,
>+  kUConvContracts,
>+  kUConvCategories,
>+  NULL,
>+  NULL,
>+  NULL
>+};

These extra NULLs aren't needed.

>diff --git a/modules/libpr0n/decoders/icon/nsIconModule.cpp b/modules/libpr0n/decoders/icon/nsIconModule.cpp

> static void
>-IconDecoderModuleDtor(nsIModule* aSelf)
>+IconDecoderModuleDtor()
> {
> #ifdef MOZ_WIDGET_GTK2
>   nsIconChannel::Shutdown();
> #endif
> }

Why not just ifdef out the whole destructor?

Comment 35

[Benjamin Smedberg]

Attachment: JS component manifest files, checkpoint

Checkpoint of translation from JS components to manifest files. I'm hitting an unrelated fastload problem which is killing us, but in general this kinda works.

Comment 36

[Benjamin Smedberg]

Attachment: Re-add CID to classinfo

Comment 37

[Benjamin Smedberg]

Attachment: JS manifests, rev. 1

This patch fixes the in-tree JS components to use manifest files. I haven't yet done the {} thing, but I can do that with python before pushing this for real.

Comment 38

[Benjamin Smedberg]

Attachment: Implement nsComponentManagerImpl::RereadChromeManifests, rev. 1

Comment 39

[Benjamin Smedberg]

Attachment: Register extension chrome.manifest again

Comment 40

[Dave Townsend [:mossop]]

Comment on attachment 453086 [details] [diff] [review]
Re-add CID to classinfo

>diff --git a/js/src/xpconnect/src/xpcjsid.cpp b/js/src/xpconnect/src/xpcjsid.cpp

>+#define NULL_CID \
>+{ 0x00000000, 0x0000, 0x0000, \
>+  { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 } }
>+
> NS_DECL_CI_INTERFACE_GETTER(nsJSIID)
>-NS_IMPL_CLASSINFO(nsJSIID, GetSharedScriptableHelperForJSIID, nsIClassInfo::THREADSAFE)
>+NS_IMPL_CLASSINFO(nsJSIID, GetSharedScriptableHelperForJSIID,
>+                  nsIClassInfo::THREADSAFE, NULL_CID)
> 
> NS_DECL_CI_INTERFACE_GETTER(nsJSCID)
>-NS_IMPL_CLASSINFO(nsJSCID, NULL, nsIClassInfo::THREADSAFE)
>+NS_IMPL_CLASSINFO(nsJSCID, NULL, nsIClassInfo::THREADSAFE, NULL_CID)

Does this NULL_CID have some special relevance I'm not seeing?

Comment 41

[Eric Shepherd [:sheppy]]

Initial work on docs for this has begun here; I'll finish them once I have more details on how this works when it's all done:

https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_1.9.3

Comment 42

[Benjamin Smedberg]

The null CID has no special relevance. It only matters because I needed *some* CID for the structure, and these components are not available by CID, so I used this bogus one.

Comment 43

[Benjamin Smedberg]

Attachment: Fix mockObjects.js

Comment 44

[Shawn Wilsher :sdwilsh]

Comment on attachment 453465 [details] [diff] [review]
Fix mockObjects.js

r=sdwilsh on the test changes (mockObjects.js)

Comment 45

[Dave Townsend [:mossop]]

Comment on attachment 453087 [details] [diff] [review]
JS manifests, rev. 1

There is at least one part missing here so I guess I need to look at it again, but just seeing the differences would be vastly preferable.

>diff --git a/browser/components/BrowserComponents.manifest b/browser/components/BrowserComponents.manifest

Need some newlines splitting up the different components in here, but presumably that's best done on landing. Same goes for the other manifest files.

>diff --git a/browser/components/feeds/src/FeedConverter.js b/browser/components/feeds/src/FeedConverter.js

> /**
>  * Keeps parsed FeedResults around for use elsewhere in the UI after the stream
>  * converter completes. 
>  */
>-var FeedResultService = {
>+function FeedResultService()
>+{ }

Open brace at the end of the function line please. And bleh, we should make XPCOMUtils support plain objects rather than needing prototypes for single instances sometime.

>-function FeedProtocolHandler(scheme) {
>-  this._scheme = scheme;
>-  var ios = 
>+function GenericProtocolHandler() {
>+}
>+FeedProtocolHandler.prototype = {

I think this needs to be GenericProtocolHandler.prototype.

>+  _init: function FPH_init() {

Change the function name prefix to GPH throughout this.

>+function FeedProtocolHandler()
>+{
>+  this._init();
>+}
>+FeedProtocolHandler.prototype = new GenericProtocolHandler();
>+FeedProtocolHandler.prototype._scheme = "feed";
>+FeedProtocolHandler.classID = Components.ID("{4f91ef2e-57ba-472e-ab7a-b4999e42d6c0}");

I think it makes more sense to pass the desired scheme to GeneralProtocolHandler._init rather than setting it on the prototype like this.

>+function PCastProtocolHandler()
>+{
>+  this._init();
>+}
>+PCastProtocolHandler.prototype = new GenericProtocolHandler();
>+PCastProtocolHandler.prototype._scheme = "pcast";
>+PCastProtocolHandler.prototype.classID = Components.ID("{1c31ed79-accd-4b94-b517-06e0c81999d5}");

I don't think the abbreviation saves us much so just go with PodCastProtocolHandler.

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

>+function nsBrowserContentHandler()
>+{
>+}

Starting brace at the end of the line.

>   /* nsISupports */
>   QueryInterface : function bch_QI(iid) {
>     if (!iid.equals(nsISupports) &&
>         !iid.equals(nsICommandLineHandler) &&
>         !iid.equals(nsIBrowserHandler) &&
>         !iid.equals(nsIContentHandler) &&
>-        !iid.equals(nsICommandLineValidator) &&
>-        !iid.equals(nsIFactory))
>+        !iid.equals(nsICommandLineValidator))
>       throw Components.results.NS_ERROR_NO_INTERFACE;
> 
>     return this;
>   },

Not required for landing, but this might be a good opportunity to switch these to use XPCOMUtils.generateQI too.

>+function nsDefaultCommandLineHandler()
>+{
>+}

Starting brace at the end of the line.

>diff --git a/browser/components/sidebar/src/nsSidebar.manifest b/browser/components/sidebar/src/nsSidebar.manifest
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/sidebar/src/nsSidebar.manifest
>@@ -0,0 +1,4 @@
>+component 22117140-9c6e-11d3-aaf1-00805f8a4905 nsSidebar.js
>+contract @mozilla.org/sidebar;1 22117140-9c6e-11d3-aaf1-00805f8a4905
>+category JavaScript-global-property sidebar @mozilla.org/sidebar;1
>+category JavaScript-global-property sidebar @mozilla.org/sidebar;1

One of these needs to be "external" instead of "sidebar"

>diff --git a/browser/fuel/src/fuelApplication.manifest b/browser/fuel/src/fuelApplication.manifest

>+component fe74cf80-aa2d-11db-abbd-0800200c9a66 fuelApplication.js
>+contract @mozilla.org/fuel/application;1 fe74cf80-aa2d-11db-abbd-0800200c9a66

You need to add to the "JavaScript global privileged property" too (from extApplication.js).

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in

Can we just combine all the manifest files together?

>diff --git a/content/xslt/src/xslt/txEXSLTRegExFunctions.js b/content/xslt/src/xslt/txEXSLTRegExFunctions.js

>+const kFactory = {

Define this as _xpcom_factory on txEXSLTRegExFunctions instead.

>diff --git a/layout/tools/pageloader/tp-cmdline.manifest b/layout/tools/pageloader/tp-cmdline.manifest
>new file mode 100644
>--- /dev/null
>+++ b/layout/tools/pageloader/tp-cmdline.manifest
>@@ -0,0 +1,3 @@
>+component 8AF052F5-8EFE-4359-8266-E16498A82E8B tp-cmdline.js
>+contract @mozilla.org/commandlinehandler/general-startup;1?type=tp 8AF052F5-8EFE-4359-8266-E16498A82E8B
>+category command-line-handler m-tp @mozilla.org/commandlinehandler/general-startup;1?type=tp

Is command-line-argument-handlers no longer in use?

>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js
>--- a/netwerk/test/httpserver/httpd.js
>+++ b/netwerk/test/httpserver/httpd.js
>@@ -5132,89 +5132,27 @@ function makeFactory(ctor)
>              if (Ci.nsIFactory.equals(aIID) ||
>                  Ci.nsISupports.equals(aIID))
>                return this;
>              throw Cr.NS_ERROR_NO_INTERFACE;
>            }
>          };
> }
> 
>-/** The XPCOM module containing the HTTP server. */
>-const module =
>+const kServerCID = Components.ID("{54ef6f81-30af-4b1d-ac55-8ba811293e41}");
>+const kServerFactory = makeFactory(nsHttpServer);

Seems like you can get rid of this special factory too and just use the normal XPCOMUtils.generateGetFactory.

>diff --git a/xpcom/ds/nsINIProcessor.manifest b/xpcom/ds/nsINIProcessor.manifest
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/ds/nsINIProcessor.manifest
>@@ -0,0 +1,2 @@
>+component {6ec5f479-8e13-4403-b6ca-fe4c2dca14fd} nsINIProcessor.js
>+contract @mozilla.org/xpcom/ini-processor-factory;1 {6ec5f479-8e13-4403-b6ca-fe4c2dca14fd}

I see no fixes for nsINIProcessor.js

Comment 46

[Benjamin Smedberg]

Attachment: Backwards-compatible changes to pageloader component in CVS, rev. 1

Comment 47

[Benjamin Smedberg]

Attachment: Backwards-compatible changes to pageloader component in CVS, rev. 1.1

Comment 48

[Benjamin Smedberg]

Attachment: Package up the new tp file, rev. 1

Comment 49

[Benjamin Smedberg]

Attachment: Review fixup from comment 45

I believe this addresses the relevant review in comment 45: additional notes/replies:

* I do plan on combining all the .manifest files together at package time, using similar logic to how we combine all the .xpt files into browser.xpt. I'll do that as a followup.
* command-line-argument-handlers is no longer in use, it was for seamonkey but nobody uses it any more

Comment 50

[alice nodelman [:alice] [:anode]]

Comment on attachment 453734 [details] [diff] [review]
Package up the new tp file, rev. 1

This will require an update to the buildfarm directory on talos-master01/talos-master02/talos-staging-master02.  That should be coordinated with releng.

Comment 51

[Benjamin Smedberg]

Attachment: Revert the interface changes, rev. 1

Mostly restoring code that I removed. I did change AutoRegister so that it will now dynamically register .xpt or .manifest files (it's equivalent to calling XRE_AddComponentLocation), because there are a few xpcshell tests that require something like it.

Comment 52

[Benjamin Smedberg]

Attachment: Binary compatibility with 1.9.2

This implements a NSGetModule wrapper, and I'm able to load libxpcomsample.so in an older build.

Comment 53

[Benjamin Smedberg]

Attachment: Register contracts after cids, per-manifest

This solves the problem we talked about a while back, where if your manifest says

contract @foo CID1
component CID1 mycomponent.js

It won't get registered. This only solves the problem per-CID, not globally, but I believe this is sufficient.

Comment 54

[Benjamin Smedberg]

Argh, s/per-CID/per-manifest/ in that last comment.

Comment 55

[Dave Townsend [:mossop]]

Comment on attachment 453157 [details] [diff] [review]
Implement nsComponentManagerImpl::RereadChromeManifests, rev. 1

Should this be purging the chrome registry first? Otherwise this seems straightforward.

Comment 56

[Benjamin Smedberg]

No. nsChromeRegistry::ReloadChrome is the only caller, and it does all the flushing first.

Comment 57

[Benjamin Smedberg]

The pageloader CVS and build/tools changes landed Friday and I've verified them via tryserver.

Comment 58

[Dave Townsend [:mossop]]

Comment on attachment 453400 [details] [diff] [review]
Register extension chrome.manifest again

> static void
>-LoadPlatformDirectory(nsIFile* aBundleDirectory,
>-                      nsCOMArray<nsIFile> &aDirectories)
>-{
>-  nsCOMPtr<nsIFile> platformDir;
>-  nsresult rv = aBundleDirectory->Clone(getter_AddRefs(platformDir));
>-  if (NS_FAILED(rv))
>-    return;
>-
>-  platformDir->AppendNative(NS_LITERAL_CSTRING("platform"));
>-
>-#ifdef TARGET_OS_ABI
>-  nsCOMPtr<nsIFile> platformABIDir;
>-  rv = platformDir->Clone(getter_AddRefs(platformABIDir));
>-  if (NS_FAILED(rv))
>-    return;
>-#endif
>-
>-  platformDir->AppendNative(NS_LITERAL_CSTRING(OS_TARGET));
>-
>-  PRBool exists;
>-  if (NS_SUCCEEDED(platformDir->Exists(&exists)) && exists)
>-    aDirectories.AppendObject(platformDir);
>-
>-#ifdef TARGET_OS_ABI
>-  platformABIDir->AppendNative(NS_LITERAL_CSTRING(TARGET_OS_ABI));
>-  if (NS_SUCCEEDED(platformABIDir->Exists(&exists)) && exists)
>-    aDirectories.AppendObject(platformABIDir);
>-#endif
>-}

I actually wasn't expecting that we'd drop the platform subdirectories. I guess it is unnecessary now but we should make sure to include this in the list of things dropped.

>   nsCOMPtr<nsIFile> subdir;
>   while (NS_SUCCEEDED(files->GetNextFile(getter_AddRefs(subdir))) && subdir) {
>     mAppBundleDirectories.AppendObject(subdir);
>-    LoadPlatformDirectory(subdir, mAppBundleDirectories);
>+
>+    nsCOMPtr<nsILocalFile> manifest =
>+      CloneAndAppend(subdir, "chrome.manifest");
>+    XRE_AddComponentLocation(NS_COMPONENT_LOCATION, manifest);

I'm starting to think that this should be called XRE_AddManifestLocation, what do you think?

Comment 59

[Benjamin Smedberg]

Indeed, I'll change it to XRE_AddManifestLocation.

Comment 60

[Benjamin Smedberg]

Attachment: Normalize slashes on Windows, and add an "xpt" directive for extensions

With this patch, and a bunch of typo/leak/test fixes that you don't need to review, I think I have only 4-8 test failures on tryserver to track down.

Comment 61

[Benjamin Smedberg]

Attachment: ABI registration modifier

I was writing docs about removing platform directories, and realized that while we have an OS modifier, we don't have an ABI modifier. This fixes it, which should allow x86/x86-64 differentiation if appropriate. With tests, even!

Comment 62

[Dave Townsend [:mossop]]

Comment on attachment 453748 [details] [diff] [review]
Review fixup from comment 45

>diff --git a/browser/components/BrowserComponents.manifest b/browser/components/BrowserComponents.manifest

I think as well as the file comments it'd be useful to put a linebreak before every component line so you can see a bit more clearly what contracts map to which component.

>diff --git a/browser/components/feeds/src/FeedConverter.js b/browser/components/feeds/src/FeedConverter.js

>+function FeedProtocolHandler() {
>+  this._init('feed');
> }
> FeedProtocolHandler.prototype = new GenericProtocolHandler();
>-FeedProtocolHandler.prototype._scheme = "feed";
> FeedProtocolHandler.classID = Components.ID("{4f91ef2e-57ba-472e-ab7a-b4999e42d6c0}");

Missed this last time around, I think you need to use FeedProtocolHandler.prototype.classID here. I think it might be worth adding some kind of logging or error to XPCOMUtils, even if only in DEBUG builds, to handle the case where component.prototype.classID is undefined, currently I think it'll just carry on without complaint you'll just never be able to get the factory for that case.

>diff --git a/browser/fuel/src/fuelApplication.manifest b/browser/fuel/src/fuelApplication.manifest
>--- a/browser/fuel/src/fuelApplication.manifest
>+++ b/browser/fuel/src/fuelApplication.manifest
>@@ -1,2 +1,3 @@
> component {fe74cf80-aa2d-11db-abbd-0800200c9a66} fuelApplication.js
> contract @mozilla.org/fuel/application;1 {fe74cf80-aa2d-11db-abbd-0800200c9a66}
>+category JavaScript-global-privileged-property extApplication @mozilla.org/fuel/application;1

You've corrected this already I believe, but that should be Application rather than extApplication.

var NSGetFactory = XPCOMUtils.generateNSGetFactory(component);

Comment 63

[Dave Townsend [:mossop]]

Comment on attachment 453851 [details] [diff] [review]
Revert the interface changes, rev. 1

> void
> nsCategoryManager::AddCategoryEntry(const char *aCategoryName,
>                                     const char *aEntryName,
>-                                    const char *aValue)
>+                                    const char *aValue,
>+                                    bool aReplace,
>+                                    char** aOldValue)

Looks like you're not using the new aReplace parameter at all. Intentional?

Comment 64

[Dave Townsend [:mossop]]

Comment on attachment 453858 [details] [diff] [review]
Binary compatibility with 1.9.2

>+#define NS_IMPL_MOZILLA192_NSGETMODULE(module)     \
>+extern "C" NS_EXPORT nsresult                      \
>+NSGetModule(nsIComponentManager* aCompMgr,         \
>+            nsIFile* aLocation,                    \
>+            nsIModule** aResult)                   \
>+{                                                  \
>+    *aResult = new mozilla::GenericModule(module); \
>+    NS_ADDREF(*aResult);                           \
>+    return NS_OK;                                  \
>+}

Would NS_IMPL_MOZILLA1_NSGETMODULE be a better name since this should work for 1.9, 1.8 etc?

>diff --git a/xpcom/components/GenericFactory.cpp b/xpcom/glue/GenericFactory.cpp
>rename from xpcom/components/GenericFactory.cpp
>rename to xpcom/glue/GenericFactory.cpp
>diff --git a/xpcom/components/GenericFactory.h b/xpcom/glue/GenericFactory.h
>rename from xpcom/components/GenericFactory.h
>rename to xpcom/glue/GenericFactory.h

Why the move out of interest?

Comment 65

[Dave Townsend [:mossop]]

Comment on attachment 454011 [details] [diff] [review]
Register contracts after cids, per-manifest

Fine as-is if you like, might be worth making the changes below in case we re-use this for another directive in the future.

>diff --git a/xpcom/components/ManifestParser.cpp b/xpcom/components/ManifestParser.cpp
>--- a/xpcom/components/ManifestParser.cpp
>+++ b/xpcom/components/ManifestParser.cpp
>@@ -79,24 +79,26 @@ struct ManifestDirective
>   // hasn't learned how to initialize unions in a sane way.
>   void (nsComponentManagerImpl::*mgrfunc)
>     (nsComponentManagerImpl::ManifestProcessingContext& cx,
>      int lineno, char *const * argv);
>   void (nsChromeRegistry::*regfunc)
>     (nsChromeRegistry::ManifestProcessingContext& cx,
>      int lineno, char *const *argv,
>      bool platform, bool contentaccessible);
>+
>+  bool isContract;

Maybe this should be "shouldDefer" or something more generic. I could anticipate that we might want to use a similar mechanism in the future.

> #elif defined(MOZ_WIDGET_GTK2)
>   nsTextFormatter::ssprintf(osVersion, NS_LITERAL_STRING("%ld.%ld").get(),
>                                        gtk_major_version,
>                                        gtk_minor_version);
> #endif
> 
>+  // Because contracts must be registered after CIDs, we save and process them
>+  // at the end.
>+  nsTArray<CachedDirective> contracts;

And defered maybe.

Comment 66

[Dave Townsend [:mossop]]

Comment on attachment 454365 [details] [diff] [review]
Normalize slashes on Windows, and add an "xpt" directive for extensions

I have some vague concerns over whether allowing any relative path is ok, means an extension can attempt to refer to stuff outside of its directory, but I guess I can't think of a problem with that off the top of my head. Just wondering if a possible alternative would be to just split the path on "/" and successively append the parts instead. Not terribly concerned though.

> static const ManifestDirective kParsingTable[] = {
>   { "binary-component", 1, true, false, false,
>     &nsComponentManagerImpl::ManifestBinaryComponent, NULL },
>+  { "xpt",              1, true, false, false,
>+    &nsComponentManagerImpl::ManifestXPT, NULL },

Would this be better named "interfaces" ?

Comment 67

[Dave Townsend [:mossop]]

Comment on attachment 454375 [details] [diff] [review]
ABI registration modifier

>+
>+      rv = xruntime->GetXPCOMABI(s);
>+      if (NS_SUCCEEDED(rv) && osTarget.Length()) {
>+        CopyUTF8toUTF16(s, abi);
>+        ToLowerCase(abi);
>+        abi.Insert(PRUnichar('_'), 0);
>+        abi.Insert(osTarget, 0);
>+      }

Why combine OS and ABI in this?

Comment 68

[Benjamin Smedberg]

> Would NS_IMPL_MOZILLA1_NSGETMODULE be a better name since this should work for
> 1.9, 1.8 etc?

I don't strongly care, although "mozilla1" sounds strange to me, maybe NS_IMPL_NSGETMODULE_COMPAT ?

> >rename from xpcom/components/GenericFactory.cpp
> >rename to xpcom/glue/GenericFactory.cpp

> Why the move out of interest?

Because the GenericFactory is used by the GenericModule, which is statically linked into xpcomglue_s for components.

> guess I can't think of a problem with that off the top of my head. Just
> wondering if a possible alternative would be to just split the path on "/" and
> successively append the parts instead. Not terribly concerned though.

I hadn't considered that possibility. Let me ponder it.

> >+  { "xpt",              1, true, false, false,
> >+    &nsComponentManagerImpl::ManifestXPT, NULL },
> 
> Would this be better named "interfaces" ?


Hrm. It's certainly more readable, but I wonder whether it will be confused for .idl files. I'll ponder!

Comment 69

[Benjamin Smedberg]

Attachment: Sorry, one more for logging

I had to change the parsing logic to keep line numbers correct (tests are good!), so I figure you should review this.

Comment 70

[Benjamin Smedberg]

Attachment: Reenable omnijar stuff for chrome/components

Comment 71

[Benjamin Smedberg]

Attachment: Mechanical changes for mobile-browser

Comment 72

[Benjamin Smedberg]

Attachment: The manifests for mobile-browser

Comment 73

[Michael Wu [:mwu]]

Comment on attachment 455222 [details] [diff] [review]
Reenable omnijar stuff for chrome/components

Looks reasonable to me. BTW, mozilla/Omnijar.h #ifdefs itself out without MOZ_OMNIJAR, so it can be included unconditionally in files.

Comment 74

[Benjamin Smedberg]

This landed in mozilla-central. It looks like it will stick: the following issues are things I intend to fix tomorrow:

* --disable-libxul
* The Qt build
* land the omnijar patch and fix the android widget factory

Followups to be filed:
* Don't troll components/*.manifest or chrome/*.manifest at all, just use a root chrome.manifest and allow "manifest" directives to do recursive loading when necessary (requires l10n repack-fu)
* Get rid of the EM restart

Comment 75

[Brendan Eich [:brendan]]

(In reply to comment #74)
> * Get rid of the EM restart

Hawt! :-).

/be

Comment 76

[Benjamin Smedberg]

Attachment: Part of a --disable-libxul fix

This is part of a --disable-libxul fix, so bienvenu can hack at some bits

Comment 77

[David :Bienvenu]

on Windows, I get an undefined symbol linking xpcom_core.dll (there a ton of locally defined symbols warnings, but only one error.
chrome_s.lib(nsChromeRegistryChrome.obj) : warning LNK4217: locally defined symb
ol ?EnsureCapacity@nsTArray_base@@IAEHII@Z (protected: int __thiscall nsTArray_b
ase::EnsureCapacity(unsigned int,unsigned int)) imported in function "public: cl
ass nsCString * __thiscall nsTArray<class nsCString>::AppendElements<class nsCSt
ring>(class nsCString const *,unsigned int)" (??$AppendElements@VnsCString@@@?$n
sTArray@VnsCString@@@@QAEPAVnsCString@@PBV1@I@Z)
xpcomcomponents_s.lib(ManifestParser.obj) : error LNK2019: unresolved external s
ymbol "void __cdecl ToLowerCase(class nsAString_internal &)" (?ToLowerCase@@YAXA
AVnsAString_internal@@@Z) referenced in function "void __cdecl ParseManifest(enu
m NSLocationType,class nsILocalFile *,char *,bool)" (?ParseManifest@@YAXW4NSLoca
tionType@@PAVnsILocalFile@@PAD_N@Z)
xpcom_core.dll : fatal error LNK1120: 1 unresolved externals
make[6]: *** [xpcom_core.dll] Error 96

Comment 78

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ToLowerCase lives in unicharutils.

Comment 79

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

More accurately, the PRUnichar ToLowerCase lives in unicharutils, while the char ToLowerCase lives in xpcom/string.  We should fix that.

Comment 80

[Gary [:streetwolf52]]

Just curious if this patch is the cause of many of my add-ons not working?

Comment 81

[Dave Townsend [:mossop]]

(In reply to comment #80)
> Just curious if this patch is the cause of many of my add-ons not working?

Likely yes. As I blogged about a while ago this will break any add-ons that include their own xpcom components.

Comment 82

[Gary [:streetwolf52]]

(In reply to comment #81)
> (In reply to comment #80)
> > Just curious if this patch is the cause of many of my add-ons not working?
> 
> Likely yes. As I blogged about a while ago this will break any add-ons that
> include their own xpcom components.

Which seems like alot.:-)  Is this something the add-on developer has to change or is their a way for a user to do it?

Comment 83

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

(In reply to comment #82)
> (In reply to comment #81)
> > (In reply to comment #80)
> > > Just curious if this patch is the cause of many of my add-ons not working?
> > 
> > Likely yes. As I blogged about a while ago this will break any add-ons that
> > include their own xpcom components.
> 
> Which seems like alot.:-)  Is this something the add-on developer has to change
> or is their a way for a user to do it?

https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_1.9.3

Comment 84

[Oleg Romashin (:romaxa)]

Attachment: FastStartup manifest missing

Comment 85

[Oleg Romashin (:romaxa)]

Attachment: Qt build bustage

Comment 86

[Benjamin Smedberg]

Comment on attachment 455633 [details] [diff] [review]
FastStartup manifest missing

I'll push these. Who other than WINCE actually uses faststartup?

Comment 87

[Benjamin Smedberg]

Attachment: Android widget changes, untested

Comment 88

[Oleg Romashin (:romaxa)]

(In reply to comment #86)
> (From update of attachment 455633 [details] [diff] [review])
> I'll push these. Who other than WINCE actually uses faststartup?
Maemo... ;)

Comment 89

[Benjamin Smedberg]

Attachment: Fix --disable-libxul

Comment 90

[Michael Wu [:mwu]]

(In reply to comment #87)
> Created an attachment (id=455678) [details]
> Android widget changes, untested

widget/src/android/nsWidgetFactory.cpp:39:31: error: nsIGenericFactory.h: No such file or directory
widget/src/android/nsWidgetFactory.cpp:55: error: expected constructor, destructor, or type conversion before 'NS_GENERIC_FACTORY_CONSTRUCTOR'

Comment 92

[Michael Wu [:mwu]]

Also, fwiw, I got these messages after trying to fix those errors:

widget/src/android/nsWidgetFactory.cpp:68: error: 'CIDEntry' in class 'mozilla::Module' does not name a type
widget/src/android/nsWidgetFactory.cpp:77: warning: extra ';'

Comment 93

[Benjamin Smedberg]

Attachment: Update interfaces.manifest correctly in non-manifest packaging

Comment 94

[Benjamin Smedberg]

Merge landing:
http://hg.mozilla.org/mozilla-central/rev/836fd3f8feba
services/*
http://hg.mozilla.org/mozilla-central/rev/05b326ca0396
QT:
http://hg.mozilla.org/mozilla-central/rev/2c381fcd78a7
http://hg.mozilla.org/mozilla-central/rev/c3b56a18d305
faststartup:
http://hg.mozilla.org/mozilla-central/rev/6b5b08f6856b
omnijar:
http://hg.mozilla.org/mozilla-central/rev/a5f7f9e82281
--disable-libxul:
http://hg.mozilla.org/mozilla-central/rev/d92fd4a1ddf5
no-manifest packaging:
http://hg.mozilla.org/mozilla-central/rev/72fcbf4fd271

Please take any remaining issues to followup bugs.

Comment 95

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I pushed http://hg.mozilla.org/mozilla-central/rev/5e43adbea1db to fix test bustage and make --disable-libxul builds finish on Windows.

Comment 96

[Bill Gianopoulos [:WG9s]]

Should not changes of this sort landed BEFORE beta1?  Just a thought.

Comment 97

[Pavel Cvrcek [:JasnaPaka]]

These changes are for beta2 (if I understand correctly). See "Target Milestone:" field above.

Comment 98

[Bill Gianopoulos [:WG9s]]

I understand that, but, in the past, it hought we tried to get changes that would break extensions landed before beta1.

Comment 99

[Darryl Miles]

Attachment: Java XPCOM bustage

Comment 100

[[e-mail]]

Why not make it so that setting XPI_NAME automatically turns on USE_EXTENSION_MANIFEST and NO_JS_MANIFEST?

Comment 101

[[e-mail]]

Comment on attachment 455708 [details] [diff] [review]
Fix --disable-libxul

>+NS_IMPL_CLASSINFO(xpcTestCallJS, NULL, 0, NS_XPCTESTCALLJS_CID);
[2x] Macro includes trailing semicolon already.

Comment 102

[Mark Banner (:standard8)]

(In reply to comment #101)
> (From update of attachment 455708 [details] [diff] [review])
> >+NS_IMPL_CLASSINFO(xpcTestCallJS, NULL, 0, NS_XPCTESTCALLJS_CID);
> [2x] Macro includes trailing semicolon already.

The fix was simple, and affected test-only code, so I pushed a bustage fix for that:

http://hg.mozilla.org/mozilla-central/rev/30efe2f7e288

Comment 103

[[e-mail]]

Attachment: Possible static components bustage fix - checked in

Comment 104

[[e-mail]]

Comment on attachment 456029 [details] [diff] [review]
Possible static components bustage fix - checked in

Bah, who is this bsterne guy making :bs ambiguous :-(

Comment 105

[[e-mail]]

Attachment: Windows static build bustage fix - checked in

PCOM now depends on chrome registry, which depends on layout, which depends on widget, which is where things start getting messy, because MSVC wants to link all the out-of-line methods that are referenced by the nsRect inline methods.

Comment 106

[[e-mail]]

Comment on attachment 456030 [details] [diff] [review]
Windows static build bustage fix - checked in

I'm not having much luck with these requestees today am I ...

Comment 107

[David :Bienvenu]

We're trying to get static builds (non-libxul) working for Thunderbird. On the mac, I get the following warning (one of many) and link error:

ld warning: codegen in nsChromeProtocolHandler::NewChannel(nsIURI*, nsIChannel**)       (offset 0x00000025) prevents image from loading in dyld shared cache
ld warning: codegen in nsChromeProtocolHandler::NewChannel(nsIURI*, nsIChannel**)       (offset 0x00000123) prevents image from loading in dyld shared cache
ld: absolute addressing (perhaps -mdynamic-no-pic) used in nsChromeRegistry::FlushAllCaches()       from nsChromeRegistry.o not allowed in slidable image. Use '-read_only_relocs suppress' to enable text relocs
collect2: ld returned 1 exit status

Anyone have an idea of what might be causing this?

Comment 108

[Benjamin Smedberg]

I don't know, but you could try adding

DEFINES         += -D_IMPL_NS_COM

to chrome/src/Makefile.in

Comment 109

[David :Bienvenu]

(In reply to comment #108)
> I don't know, but you could try adding
> 
> DEFINES         += -D_IMPL_NS_COM
> 
> to chrome/src/Makefile.in

That didn't fix it, but adding FORCE_USE_PIC=1 to that same Makefile did fix it. I think Standard8 is going to post a patch for that.

Comment 110

[Mark Banner (:standard8)]

Comment on attachment 456030 [details] [diff] [review]
Windows static build bustage fix - checked in

Checked in: http://hg.mozilla.org/mozilla-central/rev/fd6df9c6073e

Comment 111

[Mark Banner (:standard8)]

Comment on attachment 456029 [details] [diff] [review]
Possible static components bustage fix - checked in

Checked in: http://hg.mozilla.org/mozilla-central/rev/246c4ef9d717

Comment 112

[[e-mail]]

Attachment: Actually register static components - checked in

Comment 113

[[e-mail]]

Comment on attachment 457028 [details] [diff] [review]
Actually register static components - checked in

Pushed changeset 9c9f14997d9e to mozilla-central.

Comment 114

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Benjamin, is there anything we can test automatically vs. manually?

That the extension manager doesn't require a restart I will cover in Litmus tests to make sure extensions can be installed, disabled, enabled, and uninstalled. But what about other components? Is there anything QA should take care of?

Comment 115

[Benjamin Smedberg]

This is fully covered in the automated test suite.

Comment 116

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #115)
> This is fully covered in the automated test suite.

Thanks Benjamin, setting the in-litmus flag appropriately.

Comment 117

[[e-mail]]

I wanted to make a comment on one of these patches, but I couldn't work out which patch contained the code change that I wanted to comment on, and the comment isn't worth my while reading all of the patches for, so I gave up.

Comment 118

[Benjamin Smedberg]

Neil, I'm not sure I understand. If you know where the code is you want to comment on, I'm happy to look at a blame or mxr link.

Comment 119

[Nickolay_Ponomarev]

I don't quite understand https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0#Platform-specific_directories

Do these sections need an update?
https://developer.mozilla.org/en/Shipping_a_plugin_as_a_Toolkit_bundle#Platform-specific_files
https://developer.mozilla.org/en/Bundles#Platform-specific_Subdirectories

Comment 120

[Eric Shepherd [:sheppy]]

I've updated these articles:

https://developer.mozilla.org/en/Bundles
https://developer.mozilla.org/en/Shipping_a_plugin_as_a_Toolkit_bundle

To mention that platform-specific subdirectories are obsolete, and to show how to use manifest flags to select platform-specific files on Gecko 2.0 and later.

Comment 121

[a22494945]

    prefs.js文件：（使用about：config中編輯這些）// 顯示內部快捷user_pref（“keyconfig.devmode”，真）; //反轉，其中子菜單名稱的使用順序，默認值為：// 雷鳥“菜單>菜單項“// 其他”菜單項[菜單]“user_pref（”keyconfig.nicenames.reverse_order“，真）; //使用其他keyconfig配置文件，可用於臨時禁用所有按鍵user_pref（”keyconfig.profile“，”1“ ）; //禁用警告，如果已經使用的鍵輸入user_pref（“keyconfig.warnOnDuplicate”，FALSE）; //不要等待其他擊鍵，立即跳轉到應用比唐user_pref（“keyconfig.allowAltCodes” FALSE）; //代碼將運行在每一個窗口（僅在非空）user_pref（“keyconfig.global.20110323”，“ 模塊（”UIHook'）;“）; //模塊（”UIHook'）; 增加了一鍵匹配這個PREF所有窗口user_pref（“keyconfig.UIHook”，“”）; //隱藏禁用<鍵> S在該keyconfig窗口user_pref（“keyconfig.hideDisabled”，真正的）; userChrome.css：/ *風格不同類型的鍵不同* /＃keyconfig 窗口treechildren :: - MOZ樹-細胞的文本（硬編碼）{顏色：GrayText; }＃keyconfig窗口treechildren :: - MOZ樹-細胞的文本（禁用）{文字修飾：線通過; }＃keyconfig窗口treechildren :: - MOZ樹-細胞的文本（用戶）{字體重量：大膽的; }＃keyconfig窗口treechildren :: - MOZ樹小區用文本（副本）{背景色：RGB（80％，80％，80％）重要; ！顏色：繼承重要; }＃keyconfig窗口treechildren :: - MOZ樹-細胞的文本（自定義）{字體樣式：斜體; }


































    prefs.js文件：（使用about：config中編輯這些）// 顯示內部快捷user_pref（“keyconfig.devmode”，真）; //反轉，其中子菜單名稱的使用順序，默認值為：// 雷鳥“菜單>菜單項“// 其他”菜單項[菜單]“user_pref（”keyconfig.nicenames.reverse_order“，真）; //使用其他keyconfig配置文件，可用於臨時禁用所有按鍵user_pref（”keyconfig.profile“，”1“ ）; //禁用警告，如果已經使用的鍵輸入user_pref（“keyconfig.warnOnDuplicate”，FALSE）; //不要等待其他擊鍵，立即跳轉到應用比唐user_pref（“keyconfig.allowAltCodes” FALSE）; //代碼將運行在每一個窗口（僅在非空）user_pref（“keyconfig.global.20110323”，“ 模塊（”UIHook'）;“）; //模塊（”UIHook'）; 增加了一鍵匹配這個PREF所有窗口user_pref（“keyconfig.UIHook”，“”）; //隱藏禁用<鍵> S在該keyconfig窗口user_pref（“keyconfig.hideDisabled”，真正的）; userChrome.css：/ *風格不同類型的鍵不同* /＃keyconfig 窗口treechildren :: - MOZ樹-細胞的文本（硬編碼）{顏色：GrayText; }＃keyconfig窗口treechildren :: - MOZ樹-細胞的文本（禁用）{文字修飾：線通過; }＃keyconfig窗口treechildren :: - MOZ樹-細胞的文本（用戶）{字體重量：大膽的; }＃keyconfig窗口treechildren :: - MOZ樹小區用文本（副本）{背景色：RGB（80％，80％，80％）重要; ！顏色：繼承重要; }＃keyconfig窗口treechildren :: - MOZ樹-細胞的文本（自定義）{字體樣式：斜體; }