Move extensions/transformiix to content/xslt

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XSLT
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
xslt/xpath are part of our core browsing experience: the code should be moved to
content/xsl and the impl made non-optional. I'm not sure exactly how standalone
transformiix works, but I don't think this change will really affect it all that
much.
(Assignee)

Comment 1

12 years ago
Created attachment 192817 [details] [diff] [review]
CVSMoves file

This is the cvsmoves file to move extensions/transformiix to content/xsl... the
patch (against the existing files) is forthcoming.
Attachment #192817 - Flags: superreview?(jst)
Attachment #192817 - Flags: review?(peterv)
(Assignee)

Comment 2

12 years ago
Created attachment 192818 [details] [diff] [review]
Move extensions/transformiix

This is the patch. xslt-standalone does not work yet (needs changes to the root
Makefile and a few additional makefile changes), but mozilla xslt works fine
AFAICT.
Attachment #192818 - Flags: superreview?(jst)
Attachment #192818 - Flags: review?(peterv)
Should we take this opportunity to move xslt/util/* and xslt/functions/* into
xslt/ ? (at least the former should be moved at some point)
(Assignee)

Comment 4

12 years ago
I was doing the least amount of work possible ;-)

If you want to do more reorganization, let me know what the consensus is: I do
want to get this done very quickly, as it blocks some aspects of libxul/xulrunner.
Yeah, on second thought i think it might be better to leave such things for
later. There are quite a bit of rearranging that could be done so lets do all
that in a single later step instead.
I do think this needs buy-in from more people than just the XSLT owner/peers.
Did you get that from someone else?
Note also bug 192773 were people actually argued for a way to disable XSLT
completely.

See attachment 148461 [details] for the moves we were planning to do. I personally also
wanted to change most of the third list of
https://bugzilla.mozilla.org/show_bug.cgi?id=235748#c5 to all have a 'tx'
prefix. This will cause a lot of pain due to all the patches the XSLT peers have
in their own trees, so we might want to combine the moves to make it quick.
(Assignee)

Comment 7

12 years ago
Buy-in from the DOM owner (jst) is all else I need.

I already did s/TxLog/txLog/ just because it looked so weird. Note that renaming
*files* is probably pretty easy right now (change my local symlinks and add some
sed), but I am not up to the task of combining headers and whatnot.
Note that all the other stuff was already checked in (see attachment 148771 [details] [diff] [review]), so
we're only talking about renaming. Renaming does mean also changing includes and
Makefiles though.
(Assignee)

Updated

12 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
I think we should just do that stuff later. We've talked about doing some major
restructuring in the source/xml folder too so i think we can just do all the
internal cleanups at one time then.
However, I *do* think that this might be a good time to rename content/xsl to
content/xslt.
(In reply to comment #9)
> I think we should just do that stuff later. We've talked about doing some major
> restructuring in the source/xml folder too so i think we can just do all the
> internal cleanups at one time then.

And I think if we can do it now, we should. I'd rather go through only one of
these renaming/move changes to all my trees.
(Assignee)

Comment 12

12 years ago
Created attachment 192951 [details] [diff] [review]
CVSMoves file
Attachment #192817 - Attachment is obsolete: true
Attachment #192951 - Flags: superreview?(bugmail)
Attachment #192951 - Flags: review?(peterv)
(Assignee)

Comment 13

12 years ago
Created attachment 192952 [details] [diff] [review]
Move extensions/transformiix
Attachment #192818 - Attachment is obsolete: true
Attachment #192952 - Flags: superreview?(bugmail)
Attachment #192952 - Flags: review?(peterv)
(Assignee)

Comment 14

12 years ago
Comment on attachment 192817 [details] [diff] [review]
CVSMoves file

I got MOA for the general concept of moving everything to content/xslt from
jst... all I need are actual code reviews on the little bit that has actually
changed (nsLayoutModule is the bit I'm least sure about).
Attachment #192817 - Flags: superreview?(jst)
Attachment #192817 - Flags: review?(peterv)
(Assignee)

Updated

12 years ago
Attachment #192818 - Flags: superreview?(jst)
Attachment #192818 - Flags: review?(peterv)
Comment on attachment 192951 [details] [diff] [review]
CVSMoves file

>mozilla/extensions/transformiix/source/xml/dom/standalone/Document.cpp mozilla/content/xslt/src/xml/dom/txDocument.cpp
>mozilla/extensions/transformiix/source/xml/dom/standalone/Element.cpp mozilla/content/xslt/src/xml/dom/txElement.cpp
>mozilla/extensions/transformiix/source/xml/dom/standalone/dom.h mozilla/content/xslt/src/xml/dom/txDOM.h
>mozilla/extensions/transformiix/source/xml/dom/standalone/Makefile.in mozilla/content/xslt/src/xml/dom/Makefile.in
>mozilla/extensions/transformiix/source/xml/dom/standalone/NodeListDefinition.cpp mozilla/content/xslt/src/xml/dom/txNodeListDefinition.cpp

NodeListDefinition.cpp is dead.

>mozilla/extensions/transformiix/source/xml/dom/standalone/ProcessingInstruction.cpp mozilla/content/xslt/src/xml/dom/txProcessingInstruction.cpp
>mozilla/extensions/transformiix/source/xml/dom/standalone/Attr.cpp mozilla/content/xslt/src/xml/dom/txAttr.cpp
>mozilla/extensions/transformiix/source/xml/dom/standalone/NamedNodeMap.cpp mozilla/content/xslt/src/xml/dom/txNamedNodeMap.cpp

NamedNodeMap.cpp too.

>mozilla/extensions/transformiix/source/xml/dom/standalone/NodeDefinition.cpp mozilla/content/xslt/src/xml/dom/txNodeDefinition.cpp
>mozilla/extensions/transformiix/source/xml/parser/Makefile.in mozilla/content/xslt/src/xml/parser/Makefile.in
>mozilla/extensions/transformiix/source/xml/parser/txXMLParser.h mozilla/content/xslt/src/xml/parser/txXMLParser.h
>mozilla/extensions/transformiix/source/xml/parser/txXMLParser.cpp mozilla/content/xslt/src/xml/parser/txXMLParser.cpp

I'd prefer to have no subdirectories of xml, so move everything from
xml/dom/standalone/ and xml/parser into xml/

>mozilla/extensions/transformiix/source/xslt/functions/ElementAvailableFnCall.cpp mozilla/content/xslt/src/xslt/functions/txElementAvailableFnCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/FunctionAvailableFnCall.cpp mozilla/content/xslt/src/xslt/functions/txFunctionAvailableFnCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/GenerateIdFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txGenerateIdFunctionCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/SystemPropertyFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txSystemPropertyFunctionCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/Makefile.in mozilla/content/xslt/src/xslt/functions/Makefile.in
>mozilla/extensions/transformiix/source/xslt/functions/XSLTFunctions.h mozilla/content/xslt/src/xslt/functions/txXSLTFunctions.h
>mozilla/extensions/transformiix/source/xslt/functions/txFormatNumberFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txFormatNumberFunctionCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/txKeyFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txKeyFunctionCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/txKey.h mozilla/content/xslt/src/xslt/functions/txKey.h
>mozilla/extensions/transformiix/source/xslt/functions/CurrentFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txCurrentFunctionCall.cpp
>mozilla/extensions/transformiix/source/xslt/functions/DocumentFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txDocumentFunctionCall.cpp

Same for xslt, move everything from xslt/functions into xslt/

Otherwise this looks great.
Attachment #192951 - Flags: superreview?(bugmail)
Attachment #192951 - Flags: superreview+
Attachment #192951 - Flags: review?(peterv)
Attachment #192951 - Flags: review?(bugmail)
Comment on attachment 192952 [details] [diff] [review]
Move extensions/transformiix

You seem to be using txdom.h in this patch and txDOM.h in the attachment
192951 [details] [diff] [review]. (I prefer txDOM.h)

>Index: extensions/transformiix/source/xml/XMLUtils.h
>===================================================================

>@@ -168,41 +167,41 @@ public:
>         int result = MOZ_XMLCheckQName(NS_REINTERPRET_CAST(const char*,
>                                                            aQName.get()),
>                                        NS_REINTERPRET_CAST(const char*,
>                                                            end),
>                                        PR_TRUE, &colonPtr);
>         *aColon = NS_REINTERPRET_CAST(const PRUnichar*, colonPtr);
>         return result == 0;
> #else
>-        return NS_SUCCEEDED(gTxParserService->CheckQName(aQName, PR_TRUE, aColon));
>+        return NS_SUCCEEDED(nsContentUtils::GetParserServiceWeakRef()->CheckQName(aQName, PR_TRUE, aColon));

Need to null-check the result of nsContentUtils::GetParserServiceWeakRef()

> #endif
>     }
> 
>     /**
>      * Returns true if the given character represents an Alpha letter
>      */
>     static PRBool isLetter(PRUnichar aChar)
>     {
> #ifdef TX_EXE
>         return MOZ_XMLIsLetter(NS_REINTERPRET_CAST(const char*, &aChar));
> #else
>-        return gTxParserService->IsXMLLetter(aChar);
>+        return nsContentUtils::GetParserServiceWeakRef()->IsXMLLetter(aChar);

Need to null-check the result of nsContentUtils::GetParserServiceWeakRef()

> #endif
>     }
> 
>     /**
>      * Returns true if the given character is an allowable NCName character
>      */
>     static PRBool isNCNameChar(PRUnichar aChar)
>     {
> #ifdef TX_EXE
>         return MOZ_XMLIsNCNameChar(NS_REINTERPRET_CAST(const char*, &aChar));
> #else
>-        return gTxParserService->IsXMLNCNameChar(aChar);
>+        return nsContentUtils::GetParserServiceWeakRef()->IsXMLNCNameChar(aChar);

Need to null-check the result of nsContentUtils::GetParserServiceWeakRef()

>Index: extensions/transformiix/source/xpath/Makefile.in
>===================================================================

>-ifndef DISABLE_XFORMS_HOOKS
>+# The following files really ought to be provided through an extensible xpath
>+# function mechanism.

See bug 278981, so please leave the ifdef.

> EXPORTS = nsIXFormsUtilityService.h \
>           nsIXFormsXPathEvaluator.h
> 
> CPPSRCS += nsXFormsXPathEvaluator.cpp \
>-           XFormsFunctionCall.cpp
>-endif
>+           txXFormsFunctionCall.cpp

>Index: extensions/transformiix/source/xpath/txXPathAtomList.h
>===================================================================

>-#ifndef DISABLE_XFORMS_HOOKS
>+
> // XForms XPath Extensions
>+// The following functions really ought to be provided by xforms through
>+// an extensibility mechanism.

>-#endif

See above.

>Index: extensions/transformiix/source/xpath/txXPathNode.h
>===================================================================

>@@ -126,30 +125,26 @@ public:

>+    nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, namespaceID);

Long line.

>Index: extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp
>===================================================================

>@@ -524,17 +524,17 @@ txMozillaXSLTProcessor::SetParameter(con
> 
>         default:
>         {
>             return NS_ERROR_FAILURE;
>         }        
>     }
> 
>     PRInt32 nsId = kNameSpaceID_Unknown;
>-    nsresult rv = gTxNameSpaceManager->RegisterNameSpace(aNamespaceURI, nsId);
>+    nsresult rv = nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, nsId);

Long line.

>@@ -547,34 +547,34 @@ txMozillaXSLTProcessor::SetParameter(con
> }
> 
> NS_IMETHODIMP
> txMozillaXSLTProcessor::GetParameter(const nsAString& aNamespaceURI,
>                                      const nsAString& aLocalName,
>                                      nsIVariant **aResult)
> {
>     PRInt32 nsId = kNameSpaceID_Unknown;
>-    nsresult rv = gTxNameSpaceManager->RegisterNameSpace(aNamespaceURI, nsId);
>+    nsresult rv = nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, nsId);

Long line.

> NS_IMETHODIMP
> txMozillaXSLTProcessor::RemoveParameter(const nsAString& aNamespaceURI,
>                                         const nsAString& aLocalName)
> {
>     PRInt32 nsId = kNameSpaceID_Unknown;
>-    nsresult rv = gTxNameSpaceManager->RegisterNameSpace(aNamespaceURI, nsId);
>+    nsresult rv = nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, nsId);

Long line.

>Index: layout/build/nsLayoutModule.cpp
>===================================================================

>@@ -331,16 +484,28 @@ Initialize(nsIModule* aSelf)
>   if (NS_FAILED(rv)) {
>     NS_ERROR("Could not initialize nsTextTransformer");
> 
>     Shutdown();
> 
>     return rv;
>   }
> 
>+  gXPathExceptionProvider = new nsXPathExceptionProvider();
>+  if (!gXPathExceptionProvider || !txXSLTProcessor::init()) {
>+    Shutdown();
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

This will cause a problem if txXSLTProcessor::init fails, you'll NS_RELEASE
gXPathExceptionProvider in Shutdown without it ever being addref'ed.

The stuff in nsLayoutModule.cpp should all be rewritten to use content-specific
DOMCI macros, feel free to file a followup bug on that.

I don't really understand why we have both TX_EXE and MOZ_XSLT_STANDALONE. And
I suppose this breaks building transformiix standalone?
Attachment #192952 - Flags: superreview?(bugmail)
Attachment #192952 - Flags: superreview-
Attachment #192952 - Flags: review?(peterv)
Comment on attachment 192951 [details] [diff] [review]
CVSMoves file

r=me with the moves requested by peterv
Attachment #192951 - Flags: review?(bugmail) → review+
Hmm.. and what about the suggestion to use content/xsl*t*? I'd hate to see all
these files go into a bad place in the content tree.
doh! I missed the fact that you are moving the files there. My bad :)
(Assignee)

Comment 20

12 years ago
Created attachment 193072 [details] [diff] [review]
Move extensions/transformiix

This builds standalone and integrated. I have changed
--enable-application=content/xslt to match future changes I have planned for
the configure script. I know that the changes to mozilla/Makefile.in are a
little hackish, but that will get better soon.
Attachment #192952 - Attachment is obsolete: true
Attachment #193072 - Flags: review?(peterv)
(Assignee)

Comment 21

12 years ago
Created attachment 193073 [details]
CVSMoves file (final?)
Attachment #192951 - Attachment is obsolete: true
Attachment #193073 - Flags: review?(peterv)
Attachment #193073 - Flags: review?(peterv) → review+
(Assignee)

Updated

12 years ago
Depends on: 305503
Comment on attachment 193072 [details] [diff] [review]
Move extensions/transformiix

> Index: extensions/transformiix/source/base/Makefile.in
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/transformiix/source/base/Makefile.in,v
> retrieving revision 1.40
> diff -u -4 -r1.40 Makefile.in
> --- extensions/transformiix/source/base/Makefile.in	4 Apr 2005 18:38:01 -0000	1.40
> +++ extensions/transformiix/source/base/Makefile.in	18 Aug 2005 17:29:21 -0000
> @@ -49,12 +49,14 @@
>  REQUIRES	= string \
>  		  xpcom \
>  		  $(NULL)
>  
> -ifndef TX_EXE
> +ifndef MOZ_XSLT_STANDALONE
>  REQUIRES	+= unicharutil \
>  		  dom \
>  		  content \

I guess we can remove content from all the REQUIRES?

> Index: extensions/transformiix/source/main/Makefile.in
> ===================================================================

> -INCLUDES += -I$(srcdir)/../xslt -I$(srcdir)/../base -I$(srcdir)/../net \
> -  -I$(srcdir)/../xml -I$(srcdir)/../xml/dom \
> -  -I$(srcdir)/../xml/parser \
> -  -I$(srcdir)/../xpath -I$(srcdir)/../xslt/util \
> -  -I$(srcdir)/../xslt/functions $(MARK_INC)
> +INCLUDES += \
> +	-I$(srcdir)/../base \
> +	-I$(srcdir)/../xml \
> +  -I$(srcdir)/../xpath \

Remove one space

> Index: extensions/transformiix/source/xslt/functions/Makefile.in
> ===================================================================

>  ifndef TX_EXE

I think this should be MOZ_XSLT_STANDALONE? I haven't checked all
Makefile.in's, please make sure you converted them all.
Attachment #193072 - Flags: review?(peterv) → review+
Once this is done we can remove the dynamic extension of nsDocument and always
support QIing to nsIDOMXPathEvaluator

Updated

12 years ago
Summary: Move extensions/transformiix to content/xsl → Move extensions/transformiix to content/xslt

Comment 24

12 years ago
How about telling Camino that this was going to happen?
(Assignee)

Comment 25

12 years ago
I've got lots of build-config patchery coming up, and I didn't realize in particular that this was going to break camino. FYI, there are going to be similar bugs for xmlextras and webservices coming down the pipe soonish.
Status: NEW → ASSIGNED
(Assignee)

Comment 26

12 years ago
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 27

12 years ago
In Camino build, Tinderbox burns.
http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1130953920.28438.gz

Please update Camino.xcode.
http://lxr.mozilla.org/mozilla/source/camino/Camino.xcode/
(In reply to comment #27)
> In Camino build, Tinderbox burns.
> http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1130953920.28438.gz
> 
> Please update Camino.xcode.
> http://lxr.mozilla.org/mozilla/source/camino/Camino.xcode/
> 

This has been fixed by pink's checkin.

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=Camino&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1130962860&maxdate=1130963160&who=pinkerton%25aol.net
(Assignee)

Updated

12 years ago
Blocks: 314931
(Assignee)

Comment 29

12 years ago
Followup bug on the DOMCI macros is bug 314931.
Err.. so it seems like extensions/transformiix was cvs-removed without all files being moved to the new location. I thought you were going to keep the directory in extensions until the remaining stuff was moved.

There were quite a lot of stuff in transformiix/resouces that was still used, and some nice designdocs in transformiix/docs.

I'd say that at the very least tx/resources should be moved to content/xslt/resources. The docs stuff should probably move into the devmo wiki.
(Assignee)

Updated

12 years ago
Depends on: 316658

Comment 31

12 years ago
Strangely with all those moves configure still accepts (don't break at) --enable-extensions="transformix" which leads to build boostage if this option remained in .mozconfig file.
(Assignee)

Comment 32

12 years ago
--enable-extensions=unknown is allowed on purpose so that people can add custom bits to the build without altering configure
hmm, maybe configure should check that all such subdirectories exist:

  for i in $MOZ_EXTENSIONS; do
    if ! test -d extensions/$i; then
      AC_MSG_ERROR([Extension "$i" does not exist]);
    fi
  done

or something
You need to log in before you can comment on or make changes to this bug.