Closed
Bug 844457
Opened 11 years ago
Closed 9 years ago
Move XSLTProcessor to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: Ms2ger, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
3.85 KB,
text/plain
|
Details | |
39.88 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → Ms2ger
![]() |
||
Comment 2•10 years ago
|
||
oops... sorry for blocking wrong bugs. My bad.
Reporter | ||
Comment 3•10 years ago
|
||
Someone to do the work; I never got around to actually starting.
Assignee: Ms2ger → nobody
Flags: needinfo?(Ms2ger)
Updated•9 years ago
|
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
nsIDOMDocumentFragment is builtinclass and all and we're calling some gecko internal method to get such object, so static_cast should be fine.
Assignee | ||
Comment 6•9 years ago
|
||
I think I've got it now.
Assignee: peterv → evilpies
Attachment #8497533 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8497533 -
Flags: review?(bugs) → review?(peterv)
Comment 7•9 years ago
|
||
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-
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8485557 -
Attachment is obsolete: true
Attachment #8497533 -
Attachment is obsolete: true
Attachment #8501701 -
Flags: review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8501701 -
Attachment is obsolete: true
Attachment #8501701 -
Flags: review?(peterv)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8501702 -
Flags: review?(peterv)
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
>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•9 years ago
|
||
For the namespace we could use |[TreatNullAs=EmpyString] DOMString namespaceURI|, but I don't think we should allow null for the name.
Comment 13•9 years ago
|
||
(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
Assignee | ||
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8501702 -
Attachment is obsolete: true
Attachment #8502387 -
Flags: review?(peterv)
Comment 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/012853bd80b7
Comment 18•9 years ago
|
||
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
Assignee | ||
Comment 19•9 years ago
|
||
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
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
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•