Closed Bug 844457 Opened 12 years ago Closed 11 years ago

Move XSLTProcessor to WebIDL

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Ms2ger, Assigned: evilpies)

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+
Blocks: 1080503
Status: ASSIGNED → RESOLVED
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: