279057 - Implement hasFeature for XForms

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

We need to allow for http://www.w3.org/TR/xforms/index-all.html#expr-hasfeature
in the spec.  Looks like the change has to go in
nsGenericElement::InternalIsSupported() (which is called by
nsDOMImplementation::HasFeature()).  Might need to be more than just checking
for the extension (probably via do_CreateInstance) if we end up having to allow
for XForms being compiled out of XPath.  Because then XForms extension could be
installed but not have any of the XPath support it needs.

How does Mozilla handle this for extensions?  Who is the best person to talk to?
 Looks like I've got some legwork ahead of me.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

This adds support for has/getFeature extensions.
Implements a component for "org.w3c.xforms.dom" feature.
Comments?

Comment 2

[aaronr]

The only thing that popped into my rather empty head when I read through it was
what would happen if you have two extensions that have the same 'feature'
signature?  Like I assume that XForms 2.0 and 1.0 will both share the same
'feature' signature.  So if the specs aren't completely compatible, I could see
both needing to exist at the same time.

Comment 3

[Johnny Stenback (:jst)]

That's where the "version" argument comes in handy, no?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> That's where the "version" argument comes in handy, no?

Yes. I was also thinking to use
@mozilla.org/dom/feature-factory;1?feature=XXX&version=Y.Z syntax but
then I wanted to be able create (easily) services, which support many versions.
Another option could be to use categorymanager to get the CONTRACTIDs.
The category name could be something like "DOM-feature=XXX", the entry name
could be for example "1.0" and the value could be the CONTRACTID of the service.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wouldn't it be easier to just have some service with an interface like

interface DOMFeatureService {
    void SetHasFeature(in AString feature, in AString version)
    boolean HasFeature(in AString feature, in AString version)
}

Then plugins would call the first function and the DOM would check with the
second. This also makes it possible to implement domImpl.hasFeature("xforms",
"") which is apparently supposed to check for any version of xforms.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> Then plugins would call the first function and the DOM would check with the
> second. This also makes it possible to implement domImpl.hasFeature("xforms",
> "") which is apparently supposed to check for any version of xforms.

I think this is in a way almost the same thing as using categorymanager to
handle DOM feature factory contract ids.
With categorymanager approach we can also support hasfeature("xxx", "");
And contract ids (or something like that) are needed if we want to support
getFeature() (which is of course not needed in XForms nor in DOM2).

DOMImplementationRegistry etc. are then a bit different issue although related 
to this one.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2, using CategoryManager

This one is using CategoryManager.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 173862 [details] [diff] [review]
v2, using CategoryManager

Not sure who could review this, but all comments are welcome.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 173862 [details] [diff] [review]
v2, using CategoryManager

>+already_AddRefed<nsIDOMNSFeatureFactory>
>+nsGenericElement::GetDOMFeatureFactory(const nsAString& aFeature,
>+                                       const nsAString& aVersion)
>+{
>+  nsCOMPtr<nsICategoryManager> categoryManager =
>+    do_GetService(NS_CATEGORYMANAGER_CONTRACTID);
>+  if (categoryManager) {
>+    nsCAutoString featureCategory(NS_DOMNS_FEATURE_PREFIX);
>+    featureCategory.Append(NS_ConvertUTF16toUTF8(aFeature));

AppendUTF16toUTF8() is a bit faster.

>+    nsXPIDLCString contractID;
>+    nsresult rv = categoryManager->GetCategoryEntry(featureCategory.get(),
>+                                                    NS_ConvertUTF16toUTF8(aVersion).get(),
>+                                                    getter_Copies(contractID));
>+    if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<nsIDOMNSFeatureFactory> factory =
>+        do_GetService(contractID.get());
>+      if (factory) {
>+        nsIDOMNSFeatureFactory* ret = factory;
>+        NS_ADDREF(ret);
>+        return ret;

Since you're returning already_AddRefed<>, you should just return the reference
from the service manager.  For example,

nsIDOMNSFeatureFactory *factory = nsnull;
CallGetService(contractID.get(), &factory);  // addrefs
if (factory)
  return factory;

>--- extensions/xforms/nsXFormsModule.cpp	27 Jan 2005 23:02:21 -0000	1.8
>+++ extensions/xforms/nsXFormsModule.cpp	9 Feb 2005 15:50:28 -0000
>@@ -62,16 +62,21 @@ RegisterXFormsModule(nsIComponentManager
> {
>   nsCOMPtr<nsICategoryManager> catman =
>     do_GetService(NS_CATEGORYMANAGER_CONTRACTID);
> 
>   if (!catman)
>     return NS_ERROR_FAILURE;
> 
>   nsXPIDLCString previous;
>+  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));
>+

Check for a failure code on this and return if it fails.

Looks fine to me otherwise but I'd like jst to super review.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 173862 [details] [diff] [review]
v2, using CategoryManager

>Index: content/base/src/nsGenericElement.cpp

>+  else {
>+    nsCOMPtr<nsIDOMNSFeatureFactory> factory =
>+      GetDOMFeatureFactory(aFeature, aVersion);
>+      
>+    if (factory)
>+      factory->HasFeature(aObject, aFeature, aVersion, aReturn);

Nit: prevailing style in DOM land is to always use braces, even for one-line
if/else statements.

>+  }
>   return NS_OK;
> }
> 
>+nsresult
>+nsGenericElement::InternalGetFeature(nsISupports* aObject,
>+                                    const nsAString& aFeature,
>+                                    const nsAString& aVersion,
>+                                    nsISupports** aReturn)
>+{
>+  *aReturn = nsnull;
>+  nsCOMPtr<nsIDOMNSFeatureFactory> factory =
>+    GetDOMFeatureFactory(aFeature, aVersion);
>+
>+  if (factory)
>+    factory->GetFeature(aObject, aFeature, aVersion, aReturn);


Same here.


>+
>+NS_IMETHODIMP
>+nsXFormsElementFactory::HasFeature(nsISupports *aObject,
>+                                   const nsAString& aFeature,
>+                                   const nsAString& aVersion,
>+                                   PRBool *aReturn)
>+{
>+  if (aFeature.EqualsLiteral("org.w3c.xforms.dom") &&
>+      aVersion.EqualsLiteral("1.0")) {
>+    *aReturn = PR_TRUE;
>+  } else {
>+    *aReturn = PR_FALSE;
>+  }


Nit:

  if (foo && bar) {
    x = true;
  } else {
    x = false;
  }

reduces to:

  x = foo && bar;

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + comments from bryner and caillon

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 174238 [details] [diff] [review]
v2 + comments from bryner and caillon

- In nsGenericElement::GetDOMFeatureFactory():

+  if (categoryManager) {
[...]
+      nsIDOMNSFeatureFactory *factory = nsnull;
+      CallGetService(contractID.get(), &factory);  // addrefs
+      if (factory) {
+	 return factory;
+      }
+    }
+  }
+  return nsnull;

You could eliminate that if (factory) check if you'd move the declaration of
factory out to the otuermost scope and just return factory at the end of the
method.

sr=jst

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + comments from bryner, caillon and jst

Comment 14

[Doron Rosenberg (IBM)]

checked in