Closed Bug 844457 Opened 11 years ago Closed 9 years ago

Move XSLTProcessor to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Ms2ger, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Attached file WebIDL file
      No description provided.
Assignee: nobody → Ms2ger
Depends on: 962489
Ms2ger, is this blocked on something?
Flags: needinfo?(Ms2ger)
No longer depends on: 962489
oops... sorry for blocking wrong bugs. My bad.
Someone to do the work; I never got around to actually starting.
Assignee: Ms2ger → nobody
Flags: needinfo?(Ms2ger)
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
This seems to work for some stuff. I am not sure how to go from nsIDOMDocumentFragment to mozilla::dom::DocumentFragment. What about those nsIVariant? I just saw Ms2ger's webidl before uploading the patch.
nsIDOMDocumentFragment is builtinclass and all and we're calling some gecko internal method to
get such object, so static_cast should be fine.
Attached patch v1 (obsolete) — Splinter Review
I think I've got it now.
Assignee: peterv → evilpies
Attachment #8497533 - Flags: review?(bugs)
Attachment #8497533 - Flags: review?(bugs) → review?(peterv)
Comment on attachment 8497533 [details] [diff] [review]
v1

Review of attachment 8497533 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Bindings.conf
@@ +1507,5 @@
>  },
>  
> +'XSLTProcessor': {
> +    'nativeType': 'txMozillaXSLTProcessor',
> +    'headerFile': 'txMozillaXSLTProcessor.h'

headerFile shouldn't be needed.

::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp
@@ +341,2 @@
>  {
> +  SetIsDOMBinding();

Looks like you used 2-space indentation, whereas the rest of the file uses 4-space (here and in most of the other changes in this file).

@@ +1274,5 @@
> +txMozillaXSLTProcessor::Constructor(const GlobalObject& aGlobal,
> +                                    mozilla::ErrorResult& aRv)
> +{
> +  nsRefPtr<txMozillaXSLTProcessor> processor = new txMozillaXSLTProcessor(aGlobal.GetAsSupports());
> +  processor->Init(nsContentUtils::SubjectPrincipal());

Since this seems to change which principal we use anyway, why doesn't this get the principal from aGlobal?

@@ +1294,5 @@
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +  aRv = TransformToFragment(source.AsDOMNode(), domDoc, getter_AddRefs(fragment));
> +  return fragment.forget().downcast<DocumentFragment>();

I really wish you'd made the XPCOM methods call the WebIDL ones :-(.

@@ +1310,5 @@
> +void
> +txMozillaXSLTProcessor::SetParameter(JSContext* aCx,
> +                                     const nsAString& aNamespaceURI,
> +                                     const nsAString& aLocalName,
> +                                     JS::Handle<JS::Value> aValue,

If you're going to use a variant (as opposed to a union) this should at least use nsIVariant in the WebIDL.

::: dom/xslt/xslt/txMozillaXSLTProcessor.h
@@ +56,5 @@
>  public:
>      /**
>       * Creates a new txMozillaXSLTProcessor
>       */
> +    txMozillaXSLTProcessor(nsISupports* aOwner = nullptr);

Two constructors please, no default argument value.
Attachment #8497533 - Flags: review?(peterv) → review-
Depends on: 1076836
No longer blocks: 842498
Attached patch v2 (obsolete) — Splinter Review
Attachment #8485557 - Attachment is obsolete: true
Attachment #8497533 - Attachment is obsolete: true
Attachment #8501701 - Flags: review?(peterv)
Attachment #8501701 - Attachment is obsolete: true
Attachment #8501701 - Flags: review?(peterv)
Attached patch xsltprocessor (obsolete) — Splinter Review
Attachment #8501702 - Flags: review?(peterv)
Comment on attachment 8501702 [details] [diff] [review]
xsltprocessor

Review of attachment 8501702 [details] [diff] [review]:
-----------------------------------------------------------------

Could you do |hg copy dom/xslt/nsIXSLTProcessor.idl dom/webidl/XSLTProcessor.webidl| and then replace the contents of XSLTProcessor.webidl with what you currently have? And the same with txXMLUtils.* -> txExpandedName.*.

::: dom/webidl/XSLTProcessor.webidl
@@ +17,5 @@
> +   *              If the argument is an element node it must be the
> +   *              xsl:stylesheet (or xsl:transform) element of an XSLT
> +   *              stylesheet.
> +   *
> +   * @exception nsIXSLTException

You can drop all the @exception bits.

@@ +63,5 @@
> +   *                                   not supported
> +   */
> +  [Throws]
> +  void setParameter(DOMString? namespaceURI,
> +                    DOMString? localName,

I don't think these should be nullable (here and in the other parameter APIs).

::: dom/xslt/base/txExpandedName.cpp
@@ +9,5 @@
> +#include "nsGkAtoms.h"
> +#include "txStringUtils.h"
> +#include "txNamespaceMap.h"
> +#include "txXPathTreeWalker.h"
> +#include "nsContentUtils.h"

Can't you remove some of these includes? Why do you need nsContentUtils.h for example? Or nsGkAtoms.h?

::: dom/xslt/base/txExpandedName.h
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef TRANSFRMX_EXPANDEDNAME_H
> +#define TRANSFRMX_EXPANDEDNAME_H
> +

This looks like it should include mozilla/dom/NameSpaceConstants.h and nsCOMPtr at least.

::: dom/xslt/xml/txXMLUtils.h
@@ +14,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsDependentSubstring.h"
>  #include "nsIAtom.h"
>  #include "txXPathNode.h"
> +#include "txExpandedName.h"

Can this just forward declare txExpandedName? And remove the nsIAtom.h and nsCOMPtr includes.

::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp
@@ +309,5 @@
>      txOwningExpandedNameMap<txIGlobalParameter>::iterator iter(tmp->mVariables);
>      while (iter.next()) {
>          cb.NoteXPCOMChild(static_cast<txVariable*>(iter.value())->getValue());
>      }
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Now that I've landed bug 1079787 this should just become:

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(txMozillaXSLTProcessor,
                                      mOwner, mEmbeddedStylesheetRoot,
                                      mSource, mVariables)

@@ +1268,5 @@
> +/* static */ already_AddRefed<txMozillaXSLTProcessor>
> +txMozillaXSLTProcessor::Constructor(const GlobalObject& aGlobal,
> +                                    mozilla::ErrorResult& aRv)
> +{
> +    nsRefPtr<txMozillaXSLTProcessor> processor = new txMozillaXSLTProcessor(aGlobal.GetAsSupports());

Maybe wrap this as:
    nsRefPtr<txMozillaXSLTProcessor> processor =
        new txMozillaXSLTProcessor(aGlobal.GetAsSupports());
Attachment #8501702 - Flags: review?(peterv) → review+
>I don't think these should be nullable (here and in the other parameter APIs).
So after searching for some code I found this: https://developer.mozilla.org/en-US/docs/The_XSLT_JavaScript_Interface_in_Gecko/Setting_Parameters.
Seems like that requires nullable. However that example doesn't work anyway, because setParameter complains about:
"Argument 3 of XSLTProcessor.setParameter is not an object."
For the namespace we could use |[TreatNullAs=EmpyString] DOMString namespaceURI|, but I don't think we should allow null for the name.
(In reply to Tom Schuster [:evilpie] from comment #11)
> However that example doesn't work anyway,
> because setParameter complains about:
> "Argument 3 of XSLTProcessor.setParameter is not an object."

If that's with your patch, you might need something like:

diff --git a/dom/bindings/BindingUtils.cpp b/dom/bindings/BindingUtils.cpp
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -2602,10 +2602,21 @@ AssertReturnTypeMatchesJitinfo(const JSJ
 
 bool
 CallerSubsumes(JSObject *aObject)
 {
   nsIPrincipal* objPrin = nsContentUtils::ObjectPrincipal(js::UncheckedUnwrap(aObject));
   return nsContentUtils::SubjectPrincipal()->Subsumes(objPrin);
 }
 
+template <>
+inline nsresult
+UnwrapArg<nsIVariant, nsIVariant>(JSContext* cx, JS::Handle<JS::Value> v,
+                                  nsIVariant** ppArg, nsIVariant** ppArgRef,
+                                  JS::MutableHandle<JS::Value> vp)
+{
+  nsresult rv = nsContentUtils::XPConnect()->JSToVariant(cx, v, ppArgRef);
+  *ppArg = *ppArgRef;
+  return rv;
+}
+
 } // namespace dom
 } // namespace mozilla
diff --git a/dom/bindings/BindingUtils.h b/dom/bindings/BindingUtils.h
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -65,16 +65,24 @@ UnwrapArg(JSContext* cx, JS::Handle<JS::
   nsISupports* argRef = *ppArgRef;
   nsresult rv = xpc_qsUnwrapArgImpl(cx, v, NS_GET_TEMPLATE_IID(Interface),
                                     reinterpret_cast<void**>(ppArg), &argRef,
                                     vp);
   *ppArgRef = static_cast<StrongRefType*>(argRef);
   return rv;
 }
 
+template <>
+inline nsresult
+UnwrapArg<nsIVariant, nsIVariant>(JSContext* cx,
+                                            JS::Handle<JS::Value> v,
+                                            nsIVariant** ppArg,
+                                            nsIVariant** ppArgRef,
+                                            JS::MutableHandle<JS::Value> vp);
+
 inline const ErrNum
 GetInvalidThisErrorForMethod(bool aSecurityError)
 {
   return aSecurityError ? MSG_METHOD_THIS_UNWRAPPING_DENIED :
                           MSG_METHOD_THIS_DOES_NOT_IMPLEMENT_INTERFACE;
 }
 
 inline const ErrNum
This doesn't seem to work with primitives, because before we unwrap we check if the parameter is an object. I think I am just going back to any for setParameter.
Attached patch v3Splinter Review
Attachment #8501702 - Attachment is obsolete: true
Attachment #8502387 - Flags: review?(peterv)
Comment on attachment 8502387 [details] [diff] [review]
v3

Review of attachment 8502387 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp
@@ +1330,5 @@
> +{
> +    if (!nsContentUtils::XPConnect()) {
> +        aRv = NS_ERROR_FAILURE;
> +        return;
> +    }

No need for this.

@@ +1336,5 @@
> +    nsCOMPtr<nsIVariant> val;
> +    nsresult rv =
> +        nsContentUtils::XPConnect()->JSToVariant(aCx, aValue, getter_AddRefs(val));
> +    if (NS_FAILED(rv)) {
> +        aRv = rv;

Just do

    aRv =
        nsContentUtils::XPConnect()->JSToVariant(aCx, aValue, getter_AddRefs(val));
    if (aRv.Failed()) {
Attachment #8502387 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/f6714ef82d55
https://hg.mozilla.org/mozilla-central/rev/f4fa7b1b3f3f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.