Closed
Bug 844457
Opened 12 years ago
Closed 11 years ago
Move XSLTProcessor to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: Ms2ger, Assigned: evilpies)
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•12 years ago
|
Assignee: nobody → Ms2ger
![]() |
||
Comment 2•11 years ago
|
||
oops... sorry for blocking wrong bugs. My bad.
Reporter | ||
Comment 3•11 years ago
|
||
Someone to do the work; I never got around to actually starting.
Assignee: Ms2ger → nobody
Flags: needinfo?(Ms2ger)
Updated•11 years ago
|
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 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•11 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•11 years ago
|
||
I think I've got it now.
Assignee: peterv → evilpies
Attachment #8497533 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8497533 -
Flags: review?(bugs) → review?(peterv)
Comment 7•11 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•11 years ago
|
||
Attachment #8485557 -
Attachment is obsolete: true
Attachment #8497533 -
Attachment is obsolete: true
Attachment #8501701 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #8501701 -
Attachment is obsolete: true
Attachment #8501701 -
Flags: review?(peterv)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8501702 -
Flags: review?(peterv)
Comment 10•11 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•11 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•11 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•11 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•11 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•11 years ago
|
||
Attachment #8501702 -
Attachment is obsolete: true
Attachment #8502387 -
Flags: review?(peterv)
Comment 16•11 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•11 years ago
|
||
Comment 18•11 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•11 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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•