Closed
Bug 568691
(data-driven-compreg)
Opened 14 years ago
Closed 14 years ago
Use manifests and data tables to register XPCOM components
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla2.0b2
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta2+ |
People
(Reporter: benjamin, Assigned: benjamin)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(34 files, 4 obsolete files)
606.37 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
648.09 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
2.30 KB,
text/plain
|
Details | |
112.67 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
3.51 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
10.28 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
171.49 KB,
patch
|
Details | Diff | Splinter Review | |
21.40 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
203.18 KB,
patch
|
mossop
:
review-
|
Details | Diff | Splinter Review |
9.85 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
13.91 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
7.59 KB,
patch
|
mossop
:
review+
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
2.59 KB,
patch
|
anodelman
:
review+
|
Details | Diff | Splinter Review |
747 bytes,
patch
|
anodelman
:
review+
|
Details | Diff | Splinter Review |
20.42 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
21.14 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
10.27 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
8.53 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
4.61 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
6.59 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
13.41 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
48.43 KB,
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
27.66 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
5.39 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
Details | Diff | Splinter Review | |
2.24 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
9.21 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
3.47 KB,
patch
|
Details | Diff | Splinter Review | |
15.59 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
2.82 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
687 bytes,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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•14 years ago
|
||
(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
Assignee | ||
Comment 5•14 years ago
|
||
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.
Attachment #447868 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
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.
Attachment #450177 -
Flags: review?(dtownsend)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #450178 -
Flags: review?(dtownsend)
Comment 8•14 years ago
|
||
bsmedberg: was the mechanical fixup generated with code, and if so, can you share for application in c-c?
Assignee | ||
Comment 9•14 years ago
|
||
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)
Assignee | ||
Comment 10•14 years ago
|
||
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.
Attachment #450728 -
Flags: review?(dtownsend)
Assignee | ||
Updated•14 years ago
|
Attachment #450728 -
Attachment is obsolete: true
Attachment #450728 -
Flags: review?(dtownsend)
Assignee | ||
Comment 11•14 years ago
|
||
Attachment #450732 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Comment 12•14 years ago
|
||
From today's platform meeting we should consider whether this needs to ship with beta1 or not.
blocking2.0: --- → ?
Updated•14 years ago
|
Alias: data-driven-compreg
Comment 13•14 years ago
|
||
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•14 years ago
|
||
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
Assignee | ||
Comment 15•14 years ago
|
||
> >-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.
Assignee | ||
Comment 16•14 years ago
|
||
> >+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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #450177 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 20•14 years ago
|
||
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).
Assignee | ||
Comment 21•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #452783 -
Flags: review?(vladimir)
Assignee | ||
Comment 25•14 years ago
|
||
Attachment #452790 -
Flags: review?(dtownsend)
Attachment #452783 -
Flags: review?(vladimir) → review+
Comment 26•14 years ago
|
||
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?
Attachment #450732 -
Flags: review?(dtownsend) → review+
Comment 27•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 29•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
Comment on attachment 452790 [details] [diff] [review] Fix XPCOMUtils, rev. 1 ># HG changeset patch ># User Benjamin Smedberg <benjamin@smedbergs.us> ># 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.
Attachment #452790 -
Flags: review?(dtownsend) → review+
Comment 32•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
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?
Attachment #450178 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 35•14 years ago
|
||
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.
Assignee | ||
Comment 36•14 years ago
|
||
Attachment #453086 -
Flags: review?(dtownsend)
Assignee | ||
Comment 37•14 years ago
|
||
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.
Attachment #453087 -
Flags: review?(dtownsend)
Assignee | ||
Comment 38•14 years ago
|
||
Attachment #453157 -
Flags: review?(dtownsend)
Assignee | ||
Updated•14 years ago
|
blocking2.0: ? → beta2+
Updated•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 39•14 years ago
|
||
Attachment #453400 -
Flags: review?(dtownsend)
Comment 40•14 years ago
|
||
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?
Attachment #453086 -
Flags: review?(dtownsend) → review+
Comment 41•14 years ago
|
||
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
Assignee | ||
Comment 42•14 years ago
|
||
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.
Assignee | ||
Comment 43•14 years ago
|
||
Attachment #453465 -
Flags: review?(dtownsend)
Comment 44•14 years ago
|
||
Comment on attachment 453465 [details] [diff] [review] Fix mockObjects.js r=sdwilsh on the test changes (mockObjects.js)
Attachment #453465 -
Flags: review+
Comment 45•14 years ago
|
||
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
Attachment #453087 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 46•14 years ago
|
||
Attachment #453731 -
Flags: review?(anodelman)
Assignee | ||
Updated•14 years ago
|
Attachment #453731 -
Attachment is obsolete: true
Attachment #453731 -
Flags: review?(anodelman)
Assignee | ||
Comment 47•14 years ago
|
||
Attachment #453733 -
Flags: review?(anodelman)
Assignee | ||
Comment 48•14 years ago
|
||
Attachment #453734 -
Flags: review?(anodelman)
Assignee | ||
Comment 49•14 years ago
|
||
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
Attachment #453748 -
Flags: review?(dtownsend)
Assignee | ||
Updated•14 years ago
|
Attachment #449782 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #453733 -
Flags: review?(anodelman) → review+
Comment 50•14 years ago
|
||
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.
Attachment #453734 -
Flags: review?(anodelman) → review+
Assignee | ||
Comment 51•14 years ago
|
||
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.
Attachment #453851 -
Flags: review?(dtownsend)
Assignee | ||
Comment 52•14 years ago
|
||
This implements a NSGetModule wrapper, and I'm able to load libxpcomsample.so in an older build.
Attachment #453858 -
Flags: review?(dtownsend)
Assignee | ||
Comment 53•14 years ago
|
||
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.
Attachment #454011 -
Flags: review?(dtownsend)
Assignee | ||
Comment 54•14 years ago
|
||
Argh, s/per-CID/per-manifest/ in that last comment.
Updated•14 years ago
|
Attachment #453157 -
Flags: review?(dtownsend) → review+
Comment 55•14 years ago
|
||
Comment on attachment 453157 [details] [diff] [review] Implement nsComponentManagerImpl::RereadChromeManifests, rev. 1 Should this be purging the chrome registry first? Otherwise this seems straightforward.
Assignee | ||
Comment 56•14 years ago
|
||
No. nsChromeRegistry::ReloadChrome is the only caller, and it does all the flushing first.
Assignee | ||
Comment 57•14 years ago
|
||
The pageloader CVS and build/tools changes landed Friday and I've verified them via tryserver.
Updated•14 years ago
|
Attachment #453400 -
Flags: review?(dtownsend) → review+
Comment 58•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #453465 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 59•14 years ago
|
||
Indeed, I'll change it to XRE_AddManifestLocation.
Assignee | ||
Comment 60•14 years ago
|
||
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.
Attachment #454365 -
Flags: review?(dtownsend)
Assignee | ||
Comment 61•14 years ago
|
||
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!
Attachment #454375 -
Flags: review?(dtownsend)
Comment 62•14 years ago
|
||
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);
Attachment #453748 -
Flags: review?(dtownsend) → review+
Comment 63•14 years ago
|
||
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?
Attachment #453851 -
Flags: review?(dtownsend) → review+
Comment 64•14 years ago
|
||
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?
Attachment #453858 -
Flags: review?(dtownsend) → review+
Comment 65•14 years ago
|
||
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.
Attachment #454011 -
Flags: review?(dtownsend) → review+
Comment 66•14 years ago
|
||
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" ?
Attachment #454365 -
Flags: review?(dtownsend) → review+
Comment 67•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #454375 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 68•14 years ago
|
||
> 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!
Assignee | ||
Comment 69•14 years ago
|
||
I had to change the parsing logic to keep line numbers correct (tests are good!), so I figure you should review this.
Attachment #454559 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Attachment #454559 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 70•14 years ago
|
||
Attachment #455222 -
Flags: review?(mwu)
Assignee | ||
Comment 71•14 years ago
|
||
Attachment #455476 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 72•14 years ago
|
||
Attachment #455481 -
Flags: review?(mark.finkle)
Updated•14 years ago
|
Attachment #455481 -
Flags: review?(mark.finkle) → review+
Updated•14 years ago
|
Attachment #455476 -
Flags: review?(mark.finkle) → review+
Comment 73•14 years ago
|
||
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.
Attachment #455222 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 74•14 years ago
|
||
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•14 years ago
|
||
(In reply to comment #74) > * Get rid of the EM restart Hawt! :-). /be
Assignee | ||
Comment 76•14 years ago
|
||
This is part of a --disable-libxul fix, so bienvenu can hack at some bits
Comment 77•14 years ago
|
||
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
ToLowerCase lives in unicharutils.
More accurately, the PRUnichar ToLowerCase lives in unicharutils, while the char ToLowerCase lives in xpcom/string. We should fix that.
Comment 80•14 years ago
|
||
Just curious if this patch is the cause of many of my add-ons not working?
Comment 81•14 years ago
|
||
(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•14 years ago
|
||
(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?
(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•14 years ago
|
||
Attachment #455633 -
Flags: review?
Updated•14 years ago
|
Attachment #455633 -
Flags: review? → review?(benjamin)
Comment 85•14 years ago
|
||
Attachment #455644 -
Flags: review?(benjamin)
Assignee | ||
Comment 86•14 years ago
|
||
Comment on attachment 455633 [details] [diff] [review] FastStartup manifest missing I'll push these. Who other than WINCE actually uses faststartup?
Attachment #455633 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #455644 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 87•14 years ago
|
||
Attachment #455678 -
Flags: review?(vladimir)
Comment 88•14 years ago
|
||
(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... ;)
Assignee | ||
Comment 89•14 years ago
|
||
Attachment #455708 -
Flags: review?(dtownsend)
Comment 90•14 years ago
|
||
(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•14 years ago
|
||
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 ';'
Updated•14 years ago
|
Attachment #455708 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 93•14 years ago
|
||
Attachment #455742 -
Flags: review?(mark.finkle)
Updated•14 years ago
|
Attachment #455742 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 94•14 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I pushed http://hg.mozilla.org/mozilla-central/rev/5e43adbea1db to fix test bustage and make --disable-libxul builds finish on Windows.
Updated•14 years ago
|
Target Milestone: --- → mozilla1.9.3b2
Comment 96•14 years ago
|
||
Should not changes of this sort landed BEFORE beta1? Just a thought.
Comment 97•14 years ago
|
||
These changes are for beta2 (if I understand correctly). See "Target Milestone:" field above.
Comment 98•14 years ago
|
||
I understand that, but, in the past, it hought we tried to get changes that would break extensions landed before beta1.
Comment 99•14 years ago
|
||
Attachment #455940 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #455940 -
Flags: review? → review+
Comment 100•14 years ago
|
||
Why not make it so that setting XPI_NAME automatically turns on USE_EXTENSION_MANIFEST and NO_JS_MANIFEST?
Comment 101•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
Attachment #456029 -
Flags: review?
Comment 104•14 years ago
|
||
Comment on attachment 456029 [details] [diff] [review] Possible static components bustage fix - checked in Bah, who is this bsterne guy making :bs ambiguous :-(
Attachment #456029 -
Flags: review? → review?(benjamin)
Comment 105•14 years ago
|
||
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.
Attachment #456030 -
Flags: review?
Comment 106•14 years ago
|
||
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 ...
Attachment #456030 -
Flags: review? → review?(roc)
Attachment #456030 -
Flags: review?(roc) → review+
Comment 107•14 years ago
|
||
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?
Assignee | ||
Comment 108•14 years ago
|
||
I don't know, but you could try adding DEFINES += -D_IMPL_NS_COM to chrome/src/Makefile.in
Updated•14 years ago
|
Assignee | ||
Updated•14 years ago
|
Attachment #456029 -
Flags: review?(benjamin) → review+
Comment 109•14 years ago
|
||
(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•14 years ago
|
||
Comment on attachment 456030 [details] [diff] [review] Windows static build bustage fix - checked in Checked in: http://hg.mozilla.org/mozilla-central/rev/fd6df9c6073e
Attachment #456030 -
Attachment description: Windows static build bustage fix → Windows static build bustage fix - checked in
Comment 111•14 years ago
|
||
Comment on attachment 456029 [details] [diff] [review] Possible static components bustage fix - checked in Checked in: http://hg.mozilla.org/mozilla-central/rev/246c4ef9d717
Attachment #456029 -
Attachment description: Possible static components bustage fix → Possible static components bustage fix - checked in
Comment 112•14 years ago
|
||
Attachment #457028 -
Flags: review?(benjamin)
Assignee | ||
Updated•14 years ago
|
Attachment #457028 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #455678 -
Flags: review?(vladimir)
Comment 113•14 years ago
|
||
Comment on attachment 457028 [details] [diff] [review] Actually register static components - checked in Pushed changeset 9c9f14997d9e to mozilla-central.
Attachment #457028 -
Attachment description: Actually register static components → Actually register static components - checked in
Comment 114•14 years ago
|
||
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?
Flags: in-testsuite?
Flags: in-litmus?
Assignee | ||
Comment 115•14 years ago
|
||
This is fully covered in the automated test suite.
Flags: in-testsuite? → in-testsuite+
Comment 116•14 years ago
|
||
(In reply to comment #115) > This is fully covered in the automated test suite. Thanks Benjamin, setting the in-litmus flag appropriately.
Flags: in-litmus? → in-litmus-
Comment 117•14 years ago
|
||
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.
Assignee | ||
Comment 118•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•13 years ago
|
Comment hidden (offtopic, spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•