844457 - Move XSLTProcessor to WebIDL

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WebIDL file

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ms2ger, is this blocked on something?

Comment 2

[Al Tsai [:atsai]]

oops... sorry for blocking wrong bugs. My bad.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Someone to do the work; I never got around to actually starting.

Comment 4

[Tom S [:evilpie]]

Attachment: WIP

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.

Comment 5

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

nsIDOMDocumentFragment is builtinclass and all and we're calling some gecko internal method to
get such object, so static_cast should be fine.

Comment 6

[Tom S [:evilpie]]

Attachment: v1

I think I've got it now.

Comment 7

[Peter Van der Beken [: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.

Comment 8

[Tom S [:evilpie]]

Attachment: v2

Comment 9

[Tom S [:evilpie]]

Attachment: xsltprocessor

Comment 10

[Peter Van der Beken [: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());

Comment 11

[Tom S [:evilpie]]

>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."

Comment 12

[Peter Van der Beken [:peterv]]

For the namespace we could use |[TreatNullAs=EmpyString] DOMString namespaceURI|, but I don't think we should allow null for the name.

Comment 13

[Peter Van der Beken [:peterv]]

(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

Comment 14

[Tom S [:evilpie]]

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.

Comment 15

[Tom S [:evilpie]]

Attachment: v3

Comment 16

[Peter Van der Beken [: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()) {

Comment 17

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/012853bd80b7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-dt failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cff6335a6009

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2893403&repo=mozilla-inbound

Comment 19

[Tom S [:evilpie]]

I accidentally pushed a totally blank commit first, sorry.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6714ef82d55 (blank)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4fa7b1b3f3f
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24240490e404

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/f6714ef82d55
https://hg.mozilla.org/mozilla-central/rev/f4fa7b1b3f3f