Make XForms work after XPCOM registration changes

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: imphil, Assigned: imphil)

Tracking

Trunk
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

8 years ago
The XPCOM registration changes landed on mozilla-central (bug 568691) and will soon appear on mozilla-2.0. Get the XForms extension ready for it.
(Assignee)

Updated

8 years ago
Depends on: 568691
(Assignee)

Comment 1

8 years ago
Created attachment 456911 [details] [diff] [review]
[xforms] WIP patch
Assignee: nobody → mail
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Created attachment 456912 [details] [diff] [review]
[schema-validation] patch v1
(Assignee)

Updated

8 years ago
Depends on: 547652
(Assignee)

Comment 3

8 years ago
so, here are the first tries... 

Prerequisites: 
* we're talking about mozilla-central
* apply all patches from bug 547652

What works
* the XTF-stuff looks ok (xforms and schema-validation is loaded), the calculator example (http://www.mozilla.org/projects/xforms/samples/calc_svg_1010.xhtml) actually works as expected

Known problems:
* There are tons of messages on the error console like "While registering XPCOM module, trying to re-register CID '/home/philipp/src/mozilla-central/obj-debug/dist/bin/extensions/{cf2812dc-6a7c-4402-b639-4d277dac4c36}/components/libschemavalidation.so' already registered by {45c28fed-b8d3-4fba-8105-e9659f138c0f}." Those appear with different "already registered by" CIDs, it looks like all of the ones used in nsSchemaValidatorModule.cpp.

* Our XPath functions (like instance() ) are not found. I couldn't figure out how the XPath engine finds our functions in the first place, probably the nsIClassInfo changes broke something here. Who knows how it worked and how we get it working again? I've attached a testcase that shows that problem.
(Assignee)

Comment 4

8 years ago
Created attachment 456916 [details]
testcase if our xpath extension functions are found
(Assignee)

Comment 5

8 years ago
OK, follow-up to known problem 1: the error message is wrong, it should read

While registering XPCOM module /home/philipp/src/mozilla-central/obj-debug/dist/bin/extensions/{cf2812dc-6a7c-4402-b639-4d277dac4c36}/components/libschemavalidation.so, trying to re-register CID '{eced2af3-fde9-4575-b5a4-e1c830b24611}' already registered by /home/philipp/src/mozilla-central/obj-debug/dist/bin/components/libschemavalidation.so.

So that's a probably harmless problem only happening during development when libschemavalidation.so is around twice.

bug 578237 should fix the error message



re problem 2: it's somewhere in the component registry, xpath seems to be innocent. still looking.
(Assignee)

Comment 6

8 years ago
OK, debugged problem 2 down a bit, but now I'm getting more and more lost ...

If the XPath implementation tries to resolve a function call, it goes like that (some steps left out)


TX_ResolveFunctionCallXPCOM
  /content/xslt/src/xpath/txXPCOMExtensionFunction.cpp:271

LookupFunction
  /content/xslt/src/xpath/txXPCOMExtensionFunction.cpp:197

nsComponentManagerImpl::GetClassObjectByContractID(contractID="@mozilla.org/xforms-xpath-functions;1", aIID=NS_ICLASSINFO_IID)
  /xpcom/components/nsComponentManager.cpp:1152


1152     nsCOMPtr<nsIFactory> factory = FindFactory(contractID, strlen(contractID));
1153     if (!factory)
1154         return NS_ERROR_FACTORY_NOT_REGISTERED;
1155 
1156     rv = factory->QueryInterface(aIID, aResult);


Now inside GetClassObjectByContractID, the code gets the nsIFactory for our nsXFormsXPathFunctions (a nsIGenericFactory as we don't specify our own one) and tries to QueryInterface it to nsIClassInfo - which fails (seems logical). 

Questions are:
1) Why does a method named GetClassObjectByContractID return the factory instead of an object of nsXFormsXPathFunctions?

2) How has this ever worked in the previous code (the nsIClassInfo-stuff was somehow in the registration, see http://hg.mozilla.org/xforms/file/f51a343e8228/nsXFormsModule.cpp#l147. From what I've seen in other commits it seemed enough to replace it with the new macros as I've done in the patch)

3) The changes in http://hg.mozilla.org/mozilla-central/file/90afd1e80d77/content/xslt/src/xslt/txEXSLTRegExFunctions.js suggest to create a own factory which supports nsIClassInfo in QueryInterface - is this the way to go or only a necessity in JS?


I hope someone with more XPCOM knowledge can bring some light into this...

Comment 7

8 years ago
Factory objects used to also be the classinfo for a class. This is no longer the case. I don't really understand what xforms is doing here, but you'll have to modify it to get the classinfo or whatever it's doing using another method.
(Assignee)

Comment 8

8 years ago
Created attachment 457414 [details] [diff] [review]
[xforms] patch v1
Attachment #456911 - Attachment is obsolete: true
Attachment #457414 - Flags: review?(surkov.alexander)
(Assignee)

Updated

8 years ago
Attachment #456912 - Attachment description: [schema-validation] WIP patch → [schema-validation] patch v1
Attachment #456912 - Flags: review?(surkov.alexander)
(Assignee)

Comment 9

8 years ago
Thanks Benjamin! I now create a own factory class that supports nsIClassInfo. It's essentially the same approach as taken in txEXSLTRegExFunctions.js.

I didn't find any more regressions, let's hope it stays that way. With this patch XForms compiles on mozilla-central.

Comment 10

8 years ago
We are probably the only users of the xpath extension function facility currently, but you should probably check with peterv to see if they had any desire to change that area of code now that factories aren't the classinfo anymore.  Otherwise a comment should probably be added in the xpath code as to this new requirement for xpath function factories to implement that interface.
(Assignee)

Comment 11

8 years ago
Well, txEXSLTRegExFunctions.js seems to use the same interface. 

peterv, do you want to/can you change the code in LookupFunction() or should we continue to implement or own nsIFactory (like in the patch or in txEXSLTRegExFunctions.js)?
I don't really care either way. It's trivial to change LookupFunction so it does |nsCOMPtr<nsISupports> helper = do_GetService(aContractID);| and do_QueryInterface to get the classinfo instead of using do_GetClassObject. Just make LookupFunction return the helper in an out param (after calling QueryInterface(iid)) and remove the call to CallGetService in TX_ResolveFunctionCallXPCOM.
(Assignee)

Comment 13

8 years ago
looks like the nicer solution as it saves a lot of code to create the factory. Do you know of other users other than the txEXSLTRegExFunctions.js?
But note that existing users of this API (txEXSLTRegExFunctions.js) wouldn't need any changes.
(Assignee)

Comment 16

8 years ago
Created attachment 457539 [details] [diff] [review]
xpath changes
Attachment #457539 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #457539 - Flags: review? → review?(peterv)
(Assignee)

Comment 17

8 years ago
Created attachment 457541 [details] [diff] [review]
[xforms] patch v2 (requires the xpath changes)
Attachment #457414 - Attachment is obsolete: true
Attachment #457414 - Flags: review?(surkov.alexander)
(Assignee)

Comment 18

8 years ago
Thanks Peter, it looks much nicer now (saves around 120 lines)
Comment on attachment 456912 [details] [diff] [review]
[schema-validation] patch v1

at quick glance looks ok, however it's worth to find reviewer more familiar with xpcom components registration code.
Attachment #456912 - Flags: review?(surkov.alexander) → review+
Comment on attachment 457539 [details] [diff] [review]
xpath changes

>+    nsresult rv = LookupFunction(aContractID.get(), aName, iid, methodIndex,
>+                                getter_AddRefs(helper));

Nit: line up the |getter_...| with |(aCon...|.
Attachment #457539 - Flags: review?(peterv) → review+
(Assignee)

Comment 21

8 years ago
Created attachment 459001 [details] [diff] [review]
[for checkin] xpath changes v1.1 (only whitespace fixes, r=peterv)

approval2.0 requested because we need this patch to get XForms working after the XPCOM changes. I have no access to a tryserver, but it should be fairly safe.
Attachment #457539 - Attachment is obsolete: true
Attachment #459001 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #457541 - Flags: review?(surkov.alexander)

Comment 22

8 years ago
Comment on attachment 459001 [details] [diff] [review]
[for checkin] xpath changes v1.1 (only whitespace fixes, r=peterv)

Is this covered by existing tests? If not, this will need test coverage before landing. Please re-request approval as appropriate.
Attachment #459001 - Flags: approval2.0?
(Assignee)

Comment 23

8 years ago
What needs to be tested? That the API works as expected or that the in-tree consumers of this API continue to work?

Comment 24

8 years ago
The TX_ResolveFunctionCallXPCOM codepath needs to be tested somehow. It's not clear to me whether there are in-tree consumers of txXPCOMExtensionFunction.cpp or whether XForms is the consumer. If it's just XForms, you'll need to write an API test.
EXSLT is the other consumer, we have at least two mochitests for EXSLT so those should cover it.
Actually, that's wrong. We have mochitests for exslt:common, but that doesn't use txXPCOMExtensionFunction.cpp. We need a mochitest for exslt:regexp (see content/xslt/tests/mochitest/test_bug551654.html or content/xslt/tests/mochitest/test_bug551412.html for examples of exslt mochitests).
(Assignee)

Updated

8 years ago
Depends on: 581305
(Assignee)

Updated

8 years ago
Attachment #459001 - Attachment is obsolete: true
(Assignee)

Comment 27

8 years ago
When writing the test I saw that the changes broke the regexp functions, so I updated those as well. To keep this bug focused on xforms I put the new patch into bug 581305.
Comment on attachment 457541 [details] [diff] [review]
[xforms] patch v2 (requires the xpath changes)

Philipp, could you describe the patch changes to make my review easier please?
(Assignee)

Comment 29

8 years ago
Comment on attachment 457541 [details] [diff] [review]
[xforms] patch v2 (requires the xpath changes)

I hope the following comments help ...

>From: Philipp Wagner <mail@philipp-wagner.com>
>
>xpcom changes v2
>
>diff --git a/Makefile.in b/Makefile.in
>--- a/Makefile.in
>+++ b/Makefile.in
>+DEFINES += -DXFORMS_SHARED_LIBRARY=$(SHARED_LIBRARY) \
>+           -DSCHEMAVALIDATION_SHARED_LIBRARY=$(DLL_PREFIX)schemavalidation$(DLL_SUFFIX) \
>+           -DTARGET_PLATFORM=$(OS_TARGET)_$(TARGET_XPCOM_ABI)

these are used in chrome.manifest (generated from jar.mn below) for component registration. Suggestion from bsmedberg (http://groups.google.com/group/mozilla.dev.platform/msg/01f11ad27947d627)


>diff --git a/jar.mn b/jar.mn
>--- a/jar.mn
>+++ b/jar.mn
>@@ -1,13 +1,21 @@
>+#filter substitution
> xforms.jar:
>-% overlay	chrome://browser/content/preferences/preferences.xul	chrome://xforms/content/xforms-prefs.xul
>-% content	xforms	%content/xforms/ contentaccessible=yes
>-% locale	xforms	en-US	%locale/en-US/xforms/
>-% skin	xforms	classic/1.0	%skin/xforms/
>+% overlay chrome://browser/content/preferences/preferences.xul chrome://xforms/content/xforms-prefs.xul
>+% content xforms %content/xforms/ contentaccessible=yes
>+% locale xforms en-US %locale/en-US/xforms/
>+% skin xforms classic/1.0 %skin/xforms/
>+% interfaces components/schemavalidation.xpt
>+% interfaces components/xforms.xpt
>+% binary-component components/@XFORMS_SHARED_LIBRARY@ ABI=@TARGET_PLATFORM@
>+% binary-component components/@SCHEMAVALIDATION_SHARED_LIBRARY@ ABI=@TARGET_PLATFORM@
>+% component {26d69f7e-f7cf-423d-afb9-43d8a9ebf3ba} components/nsSchemaValidatorRegexp.js
>+% contract @mozilla.org/xmlextras/schemas/schemavalidatorregexp;1 {26d69f7e-f7cf-423d-afb9-43d8a9ebf3ba}
>+% category profile-after-change nsSchemaValidatorRegexp	@mozilla.org/xmlextras/schemas/schemavalidatorregexp;1

register our components and interfaces as they are not auto-registered any more. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0 for details. I added the ABI to make it easier for my scripts to created merged XPIs that contain binary parts for different platforms. There is no functional difference (obviously the binaries wouldn't work on other platforms anyway).


>diff --git a/nsXFormsDOMEvent.cpp b/nsXFormsDOMEvent.cpp
>--- a/nsXFormsDOMEvent.cpp
>+++ b/nsXFormsDOMEvent.cpp
>@@ -180,8 +180,24 @@ nsXFormsDOMEvent::GetInternalNSEvent()
> }
> 
> NS_METHOD
> nsXFormsDOMEvent::SetTrusted(PRBool aTrusted)
> {
>   nsCOMPtr<nsIPrivateDOMEvent> privEvent = do_QueryInterface(mInner);
>   return privEvent->SetTrusted(aTrusted);
> }
>+
>+void
>+nsXFormsDOMEvent::Serialize(IPC::Message* aMsg,
>+                        PRBool aSerializeInterfaceType)
>+{
>+  nsCOMPtr<nsIPrivateDOMEvent> privEvent = do_QueryInterface(mInner);
>+  privEvent->Serialize(aMsg, aSerializeInterfaceType);
>+}
>+
>+PRBool
>+nsXFormsDOMEvent::Deserialize(const IPC::Message* aMsg, void** aIter)
>+{
>+  nsCOMPtr<nsIPrivateDOMEvent> privEvent = do_QueryInterface(mInner);
>+  return privEvent->Deserialize(aMsg, aIter);
>+}

nsIPrivateDOMEvent now requires those two methods. Simply forward them the same way we did the other methods. Not really XPCOM related, just didn't want to open another bug for those little changes. If that's a problem, I can open another bug report as well.

>diff --git a/nsXFormsModule.cpp b/nsXFormsModule.cpp
>--- a/nsXFormsModule.cpp
>+++ b/nsXFormsModule.cpp
>-#include "nsIGenericFactory.h"
>+#include "mozilla/ModuleUtils.h"
> #include "nsXFormsElementFactory.h"
> #include "nsXFormsUtilityService.h"
> #include "nsXFormsAtoms.h"
> #include "nsXFormsModelElement.h"
> #include "nsXFormsUtils.h"
>-#include "nsICategoryManager.h"
>-#include "nsIServiceManager.h"
>+#include "nsXFormsXPathFunctions.h"
> #include "nsIClassInfoImpl.h"
>-#include "nsXFormsXPathFunctions.h"
>-#include "nsServiceManagerUtils.h"
>-
>-// bb0d9c8b-3096-4b66-92a0-6c1ddf80e65f
>-#define NS_XFORMSUTILITYSERVICE_CID \
>-{ 0xbb0d9c8b, 0x3096, 0x4b66, { 0x92, 0xa0, 0x6c, 0x1d, 0xdf, 0x80, 0xe6, 0x5f }}
>-
>-#define NS_XFORMSUTILITYSERVICE_CONTRACTID \
>-"@mozilla.org/xforms-utility-service;1"
>-
>-#define XFORMSXPATHFUNCTIONS_CID \
>-{ 0x8edc8cf1, 0x69a3, 0x11d9, { 0x97, 0x91, 0x00, 0x0a, 0x95, 0xdc, 0x23, 0x4c } }
>-#define XFORMSXPATHFUNCTIONS_CONTRACTID \
>-"@mozilla.org/xforms-xpath-functions;1"
>+#include "mozilla/Module.h"
>+#include "nsIFactory.h"

CID and CONTRACTID are now in their corresponding implementation headers, like it seems to be usually done.

> NS_GENERIC_FACTORY_CONSTRUCTOR(nsXFormsElementFactory)
> NS_GENERIC_FACTORY_CONSTRUCTOR(nsXFormsUtilityService)
> NS_GENERIC_FACTORY_CONSTRUCTOR(nsXFormsXPathFunctions)
>-NS_DECL_CLASSINFO(nsXFormsXPathFunctions)

that one does not exist any more, it's now done nsXFormsXPathFunctions

>-static NS_IMETHODIMP
>-RegisterXFormsModule(nsIComponentManager *aCompMgr,
>-                     nsIFile *aPath,
>-                     const char *aRegistryLocation,
>-                     const char *aComponentType,
>-                     const nsModuleComponentInfo *aInfo)
>-{
>-#ifdef DEBUG
>-  printf("XFORMS Module: Registering\n");
>-#endif
>+NS_DEFINE_NAMED_CID(NS_XFORMSELEMENTFACTORY_CID);
>+NS_DEFINE_NAMED_CID(NS_XFORMSUTILITYSERVICE_CID);
>+NS_DEFINE_NAMED_CID(NS_XFORMSXPATHFUNCTIONS_CID);

gives us the k... constants used below

>-  nsCOMPtr<nsICategoryManager> catman =
>-    do_GetService(NS_CATEGORYMANAGER_CONTRACTID);
> 
>-  if (!catman)
>-    return NS_ERROR_FAILURE;
>-
>-  nsCString previous;
>-  nsresult rv =
>-    catman->AddCategoryEntry(NS_DOMNS_FEATURE_PREFIX "org.w3c.xforms.dom",
>-                             "1.0",
>-                             NS_XTF_ELEMENT_FACTORY_CONTRACTID_PREFIX NS_NAMESPACE_XFORMS,
>-                             PR_TRUE, PR_TRUE, getter_Copies(previous));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  rv =
>-    catman->AddCategoryEntry(NS_DOMNS_FEATURE_PREFIX NS_XFORMS_INSTANCE_OWNER,
>-                             "1.0",
>-                             NS_XTF_ELEMENT_FACTORY_CONTRACTID_PREFIX NS_NAMESPACE_XFORMS,
>-                             PR_TRUE, PR_TRUE, getter_Copies(previous));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  return catman->AddCategoryEntry("agent-style-sheets",
>-                                  "xforms stylesheet",
>-                                  "chrome://xforms/content/xforms.css",
>-                                  PR_TRUE, PR_TRUE, getter_Copies(previous));
>-}
>-
>-static NS_IMETHODIMP
>-UnregisterXFormsModule(nsIComponentManager *aCompMgr,
>-                       nsIFile *aPath,
>-                       const char *aRegistryLocation,
>-                       const nsModuleComponentInfo *aInfo)
>-{
>-#ifdef DEBUG
>-  printf("XFORMS Module: Unregistering\n");
>-#endif
>-
>-  nsXFormsUtils::Shutdown();
>-
>-  nsCOMPtr<nsICategoryManager> catman =
>-    do_GetService(NS_CATEGORYMANAGER_CONTRACTID);
>-
>-  if (!catman)
>-    return NS_ERROR_FAILURE;
>-
>-  catman->DeleteCategoryEntry(NS_DOMNS_FEATURE_PREFIX "org.w3c.xforms.dom",
>-                              "1.0", PR_TRUE);
>-
>-  catman->DeleteCategoryEntry(NS_DOMNS_FEATURE_PREFIX NS_XFORMS_INSTANCE_OWNER,
>-                              "1.0", PR_TRUE);
>-
>-  return catman->DeleteCategoryEntry("agent-style-sheets",
>-                                     "xforms stylesheet", PR_TRUE);
>-}

registration with the category manager is now done below in kXFormsCategories. Everything else is in the new Startup/Shutdown methods below.

>-static const nsModuleComponentInfo components[] = {
>-  { "XForms element factory",
>-    NS_XFORMSELEMENTFACTORY_CID,
>-    NS_XTF_ELEMENT_FACTORY_CONTRACTID_PREFIX NS_NAMESPACE_XFORMS,
>-    nsXFormsElementFactoryConstructor,
>-    RegisterXFormsModule,
>-    UnregisterXFormsModule
>-  },
>-  { "XForms Utility Service",
>-    NS_XFORMSUTILITYSERVICE_CID,
>-    NS_XFORMSUTILITYSERVICE_CONTRACTID,
>-    nsXFormsUtilityServiceConstructor
>-  },
>-  { "XForms XPath extension functions",
>-    XFORMSXPATHFUNCTIONS_CID,
>-    XFORMSXPATHFUNCTIONS_CONTRACTID,
>-    nsXFormsXPathFunctionsConstructor,
>-    nsnull, nsnull, nsnull,
>-    NS_CI_INTERFACE_GETTER_NAME(nsXFormsXPathFunctions),
>-    nsnull,
>-    &NS_CLASSINFO_NAME(nsXFormsXPathFunctions)
>-  }

replaced with the new structs below, no real functional changes. The xpath extension functions are special, because the nsIClassInfo (required by the extensible xpath implementation) used to be in the nsIFactory. Now it's in the in nsXFormsXPathFunctions itself. This requires some changes to XPath in bug 581305 and was suggested by peterv.

>+const mozilla::Module::CIDEntry kXFormsCIDs[] = {
>+  { &kNS_XFORMSELEMENTFACTORY_CID, false, NULL, nsXFormsElementFactoryConstructor },
>+  { &kNS_XFORMSUTILITYSERVICE_CID, false, NULL, nsXFormsUtilityServiceConstructor },
>+  { &kNS_XFORMSXPATHFUNCTIONS_CID, false, NULL, nsXFormsXPathFunctionsConstructor },
>+  { NULL }
> };
> 
>-PR_STATIC_CALLBACK(nsresult)
>-XFormsModuleCtor(nsIModule* aSelf)
>+const mozilla::Module::ContractIDEntry kXFormsContracts[] = {
>+  { NS_XTF_ELEMENT_FACTORY_CONTRACTID_PREFIX NS_NAMESPACE_XFORMS, &kNS_XFORMSELEMENTFACTORY_CID },
>+  { NS_XFORMSUTILITYSERVICE_CONTRACTID, &kNS_XFORMSUTILITYSERVICE_CID },
>+  { NS_XFORMSXPATHFUNCTIONS_CONTRACTID, &kNS_XFORMSXPATHFUNCTIONS_CID },
>+  { NULL }
>+};
>+
>+const mozilla::Module::CategoryEntry kXFormsCategories[] = {
>+  { NS_DOMNS_FEATURE_PREFIX "org.w3c.xforms.dom",
>+    "1.0",
>+    NS_XTF_ELEMENT_FACTORY_CONTRACTID_PREFIX NS_NAMESPACE_XFORMS },
>+
>+  { NS_DOMNS_FEATURE_PREFIX NS_XFORMS_INSTANCE_OWNER,
>+    "1.0",
>+    NS_XTF_ELEMENT_FACTORY_CONTRACTID_PREFIX NS_NAMESPACE_XFORMS },
>+
>+  { "agent-style-sheets",
>+    "xforms stylesheet",
>+    "chrome://xforms/content/xforms.css" },
>+
>+  { NULL }
>+};
>+
>+static nsresult
>+nsXFormsStartup()
> {
>   nsXFormsAtoms::InitAtoms();
>   nsXFormsUtils::Init();
>   nsXFormsModelElement::Startup();
>   return NS_OK;
> }
> 
>-NS_IMPL_NSGETMODULE_WITH_CTOR(xforms, components, XFormsModuleCtor)
>+static void
>+nsXFormsShutdown()
>+{
>+  nsXFormsUtils::Shutdown();
>+}

simular to the old Register/Unregister methods, only use more common naming and remove all component manager stuff.

>+
>+static const mozilla::Module kXFormsModule = {
>+  mozilla::Module::kVersion,
>+  kXFormsCIDs,
>+  kXFormsContracts,
>+  kXFormsCategories,
>+  NULL,
>+  nsXFormsStartup,
>+  nsXFormsShutdown
>+};
>+
>+NSMODULE_DEFN(nsXFormsModule) = &kXFormsModule;

>diff --git a/nsXFormsXPathFunctions.cpp b/nsXFormsXPathFunctions.cpp
>--- a/nsXFormsXPathFunctions.cpp
>+++ b/nsXFormsXPathFunctions.cpp
>+NS_IMPL_CLASSINFO(nsXFormsXPathFunctions, NULL, 0, NS_XFORMSXPATHFUNCTIONS_CID)

replaces NS_DECL_CLASSINFO from nsXFormsModule
Comment on attachment 457541 [details] [diff] [review]
[xforms] patch v2 (requires the xpath changes)

thanks for the patch and detailed description. looks fine with me, r=me
Attachment #457541 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 31

8 years ago
Created attachment 474412 [details] [diff] [review]
[schema-validation] v1.1

remove unnecessary profile-after-change listener
Attachment #456912 - Attachment is obsolete: true
(Assignee)

Comment 32

8 years ago
Created attachment 474413 [details] [diff] [review]
[xforms] v2.1

Now *.manifest files are auto-generated and included into the chrome.manifest by the build system. If we declare the components twice, Firefox crashes (bug 589992). The new patch versions fix this by not declaring the components in jar.mn any more (as it was before) and let the auto-generation do its work. It's the nicer solution anyway.

Alex: The changes are minor, but I still didn't want to carry over the r+ flags.
Attachment #457541 - Attachment is obsolete: true
Attachment #474413 - Flags: review?(surkov.alexander)
(Assignee)

Updated

8 years ago
Attachment #474412 - Flags: review?(surkov.alexander)
Comment on attachment 474412 [details] [diff] [review]
[schema-validation] v1.1

rs=me if you're fine with it
Attachment #474412 - Flags: review?(surkov.alexander) → review+
Comment on attachment 474413 [details] [diff] [review]
[xforms] v2.1

rs=me if you're fine with it
Attachment #474413 - Flags: review?(surkov.alexander) → review+
(Assignee)

Updated

8 years ago
Attachment #474412 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

8 years ago
Attachment #474413 - Flags: review?(Olli.Pettay)
Comment on attachment 474412 [details] [diff] [review]
[schema-validation] v1.1

rs=me
Attachment #474412 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 474413 [details] [diff] [review]
[xforms] v2.1

>+
>+void
>+nsXFormsDOMEvent::Serialize(IPC::Message* aMsg,
>+                        PRBool aSerializeInterfaceType)
Fix the indentation. PRBool should be under IPC
Attachment #474413 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 37

8 years ago
pushed http://hg.mozilla.org/xforms/rev/3bd9353a5347
http://hg.mozilla.org/schema-validation/rev/ea31fd50c36b

(my first two pushes! I hope I didn't miss anything)

I'll leave this open until bug 581305 is fixed, as we currently don't compile without the patch there.
(In reply to comment #37)
> pushed http://hg.mozilla.org/xforms/rev/3bd9353a5347
> http://hg.mozilla.org/schema-validation/rev/ea31fd50c36b
> 
> (my first two pushes! I hope I didn't miss anything)

congrats! usually you don't until you have new files (that's common thing that you forget to add them) :)
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.