88623 - Cleanup Transformiix code

Status: VERIFIED FIXED

(Core :: XSLT, defect, P4)

Description

[Peter Van der Beken [:peterv]]

First up, some warning fixes. See also bug 88612 for XPath code clean-up.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Warning fixes

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Warning fixes v2

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

nice! r=sicking

Comment 4

[Johnny Stenback (:jst)]

sr=jst

Comment 5

[Peter Van der Beken [:peterv]]

First patch checked in.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Comment clean-up

Comment 7

[Axel Hecht]

Comments like
 * $Id: AdditiveExpr.cpp,v 1.1 2000/04/06 07:44:37 kvisco%ziplink.net Exp $
 * @author <A HREF="mailto:[e-mail]">Keith Visco</A>
 * @version $Revision: 1.1 $ $Date: 2000/04/06 07:44:37 $
ought to go, giving a rs to peterv for removing those

Axel

Comment 8

[Axel Hecht]

de-rubberfying. r=me on attachement 40874

Axel

Comment 9

[Johnny Stenback (:jst)]

rs=jst

Comment 10

[Peter Van der Beken [:peterv]]

Attachment 40874 [details] [diff] was checked in.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: More small clean-up/warning fixes

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

r=sicking on attachment 52145 [details] [diff] [review]

However, I just noticed a pretty serious thing in 
txResultStringComparator::compareValues which would be nice if you could fix 
while you're at it. If createRawSortKey fails when creating 
mCaseKey/mCaseLength the StringValue ends up in a pretty bad state; mCaseKey 
gets set to null making us leak the String* key and mCaseLength is set to >0 
indicating that mCaseKey points to the mCollation-key.

Would be great if you could do something about that

Comment 13

[Peter Van der Beken [:peterv]]

Good catch. How about restoring the string pointer in mCaseKey and setting
mCaseLength to 0?
I'm attaching another patch. It's not cleanup in the strict sense of the word,
it adds String::isEmpty(). I've tried replacing all the length==0, >0, !=0 in
the tree where it made sense. I've avoided doing cleanup or restructuring,
except in some extreme cases. Looking for an r on that one too.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: Adding String::isEmpty()

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yes, restoring mCaseKey and mCaseLength seems like the safest thing (though not 
neccesarily the fastest when it comes to what happens if we fail, but i don't 
see any reason to optimize that anyway)

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

on attachment 52208 [details] [diff] [review]:

+MBool String::isEmpty() const
+{
+    return (!strBuffer || (strBuffer[0] == 0x0000));
+}

won't work (our strings arn't always nullterminated, see implementation 
of ::clear), just do |return strLength == 0|


urgl, horrible indentation in 

+            case 0x000D: // CR
+            break;
+            default:
+            toStrip = MB_FALSE;
+            break;

please add another level for the actual codelines


I don't like the changes to StringFunctionCall.cpp, why add an optimization 
(which indents the code one extra level) for a case that is very unlikly to 
happen?


in ProcessorState::getResultNameSpaceURI:

-    if (prefix.length() == 0) {
+    if (!prefix.isEmpty()) {

is wrong


appart from that and assuming the changes in ProcessorState::initialize are 
whitespace only, r=sicking

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove NamedMap::Equals

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

above patch removes NamedMap::equals which gives Nisheeth 14(!) warnings. The 
function isn't implemented and isn't used anywere. There is no risk for 
fallbacks to TxObject::equals since NamedMap::equals has a different signature

Comment 19

[Axel Hecht]

Comment on attachment 53310 [details] [diff] [review]
remove NamedMap::Equals

r=[e-mail]

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Combining previous patches

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 53670 [details] [diff] [review]
Combining previous patches

r=sicking

Comment 22

[Axel Hecht]

Comment on attachment 53670 [details] [diff] [review]
Combining previous patches

oh, another r=me.
no landing order requests, that patch
merges with nsdoom, so I'm all fine

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 53670 [details] [diff] [review]
Combining previous patches

sr=jst

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 53670 [details] [diff] [review]
Combining previous patches

Checked in.

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: And more

Comment 26

[Axel Hecht]

Comment on attachment 53736 [details] [diff] [review]
And more

nice. less warnings is always a good thing.

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 53736 [details] [diff] [review]
And more

another one bites the dust.

Comment 28

[Axel Hecht]

*** Bug 59853 has been marked as a duplicate of this bug. ***

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove txList::iterator()

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The attached patch cleans up some uses of txListIterator. Most of the fixes are 
changing |ListIterator iter = foo.iterator()| to |txListIterator iter(&foo)|. 
Since I touched almost all instantiations of ListIterators I took the 
opportunity to remove the "ListIterator" typedef (actually i'd like to name it 
txList::iterator instead of txListIterator, thoughts?).

To do the same for stackiterators I had to create an explicit txStackIterator 
class (since Stack can't be cast to txList since Stack inherits txList 
privately). There are other solutions to this, but I like this one best since 
it makes it impossible to make an iterator that adds/deletes elements elements 
down in the stack.

Comment 31

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: remove ArrayList/TxObjectWrapper, move URIUtils to base

I've removed ArrayList, only Node on standalone uses it and I've made a small,
simple replacement class. Also removed TxObjectWrapper, I know Axel does this
on his branch but doing it separately shouldn't give merge problems, right?
I've also moved URIUtils to base, we don't need a separate source directory for
two files.

Comment 33

[Peter Van der Beken [:peterv]]

New patches coming up.

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: remove ArrayList/TxObjectWrapper, move URIUtils to base

Comment 35

[Peter Van der Beken [:peterv]]

Attachment: remove XSLType

Comment 36

[Peter Van der Beken [:peterv]]

Attachment: remove XSLType (diff -w for reviewers)

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 87054 [details] [diff] [review]
remove ArrayList/TxObjectWrapper, move URIUtils to base

I don't understand how the new string-list works. Where are the actual strings
allocated? I can only find allocation of stringpointers. Couldn't you use a
txList instead? The only dissadvantage of that is that you'll have to manually
deallocate the strings, but that is IMHO not a big problem.

Comment 38

[Peter Van der Beken [:peterv]]

baseUrls.add(new String(((Document*)node)->getBaseURI()));

and

+String* txBaseURLArray::getCurrent()
+{
+  if (mCurrentURL) {
+    mCurrentURL = new String;
+  }

Comment 39

[Peter Van der Beken [:peterv]]

Oh, and make that if (!mCurrentURL).

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

on attachment 87057 [details] [diff] [review]:

+        // xsl:param
+        else if (localName == txXSLTAtoms::param) {
                     String name;
                     element->getAttr(txXSLTAtoms::name, kNameSpaceID_None,
                                      name);
                     if (name.isEmpty()) {
                         String err("missing required name attribute for...
                         aPs->receiveError(err, NS_ERROR_FAILURE);
-                        break;
                     }
-
-                    ExprResult* exprResult
-                        = processVariable(aSource, element, aPs);
+            else {
+                ExprResult* exprResult = processVariable(aSource, element...

I don't really like putting the non-error code inside an |else|, mind changing 
the |break| to a |return| instead?


ugh, it's such a pity that we don't have nsCOMPtrs in standalone :(.. I wonder 
if it would be possible to hack one that only handled atoms. (in a separate bug 
of course)

It might be a good idea to have a 
|XSLTProcessor::isXSLTElementOfType(Node* aNode, txAtom* aType)| function for 
when/choose/sort/with-param/etc. Your call.


-                    String err("error processing xsl:");
-                    err.append(PROC_INST);
+            String err("error processing xsl:processing-instruction");
                     err.append(", '");
merge the ", '" into the live above



@@ -2317,7 +2276,7 @@
 XSLTProcessor::TransformDocument(nsIDOMNode* aSourceDOM,
                                  nsIDOMNode* aStyleDOM,
                                  nsIDOMDocument* aOutputDoc,
-                                 nsIObserver* aObserver)
+                                 nsITransformObserver* aObserver)
 {
     // We need source and result documents but no stylesheet.
     NS_ENSURE_ARG(aSourceDOM);
@@ -2495,14 +2454,6 @@
         mScriptLoader = nsnull;
     }
 
-    nsresult rv;
-    nsCOMPtr<nsIObserverService> anObserverService = do_GetService
("@mozilla.org/observer-service;1", &rv);
-    if (NS_SUCCEEDED(rv)) {
-        nsCOMPtr<nsIContent> rootContent;
-        mOutputHandler->getRootContent(getter_AddRefs(rootContent));
-        anObserverService->AddObserver(mObserver, "xslt-done", PR_TRUE);
-        anObserverService->NotifyObservers(rootContent, "xslt-done", nsnull);
-    }

is this supposed to be part of this patch?


other then that, r=sicking

Comment 41

[Peter Van der Beken [:peterv]]

Attachment: remove XSLType

Adressed comments.

Comment 42

[Peter Van der Beken [:peterv]]

Attachment: remove XSLType (diff -w for reviewers)

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 87456 [details] [diff] [review]
remove XSLType (diff -w for reviewers)

sr=jst

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 87454 [details] [diff] [review]
remove XSLType

Checked in.

Comment 45

[Peter Van der Beken [:peterv]]

Attachment: remove ArrayList/TxObjectWrapper, move URIUtils to base

I didn't use StringList because I think it's overkill for this simple use.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

+  ~txBaseURLArray()
+  {
+    delete [] mURLs;
+  }

Wasn't the whole point of this class to delete the strings in the dtor so that 
you wouldn't have to do that manually?


+  String* get(PRUint32 aCount)
+  {
+    if (!mURLs) {
+      return 0;
+    }
+    return mURLs[aCount];
+  }

IMHO the |if (!mURLs) {| isn't really needed. mCount will always be 0 if mURLs 
is null, so all indexes to |get| will be illegal anyway.


+    memcpy(temp, mURLs, mCount * sizeof(String*));
+    delete mURLs;
+    mURLs = temp;
+    mMax = newMax;

should be |delete [] mURLs| i think


+        if (!current) {
+          current = new String;
+        }

|current| is always null here, right? Couldn't current just be a local variable 
in the |case Node::ELEMENT_NODE|?


+      case Node::DOCUMENT_NODE:
+      {
         baseUrls.add(new String(((Document*)node)->getBaseURI()));
         break;

you added an out-of-memory check for the |new| for elements, it'd be great if 
you did the same for documents.


+  PRUint32 i;
+  for (i = baseUrls.mCount - 2; i >= 0; --i) {

|i| being unsigned is a bad thing :-)

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove default ctor in ProcessorState

Now that we don't use ProcessorStates for standalone XPath we no longer need
the "empty" ctor for ProcessorState

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove txList::iterator() (sync with tip)

brought this one back to life again

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove txList::iterator() (sync with tip)

it's always a good idea to let cvs finish before attaching...

Comment 50

[Axel Hecht]

txBaseURLArray::get should check for aCount<mMax, at least in a assertion.
Then you can drop the check for mUrls, too.

Comment 51

[Peter Van der Beken [:peterv]]

> Wasn't the whole point of this class to delete the strings in the dtor so that
> you wouldn't have to do that manually?

Nope. The point to doing it this way is to loop once.

> |current| is always null here, right? Couldn't current just be a local
> variable in the |case Node::ELEMENT_NODE|?

Nope. Only in the first iteration or if the element from the previous iteration
has a base attribute.

> txBaseURLArray::get should check for aCount<mMax, at least in a assertion.

Only in an assertion. This class isn't meant to be used elsewhere, so we're sure
the indexes will be correct. I meant to remove the check for mURLs but I forgot.

Comment 52

[Peter Van der Beken [:peterv]]

Comment on attachment 87870 [details] [diff] [review]
remove default ctor in ProcessorState

r=peterv.
Should we assert about the documents?

Comment 53

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: remove default ctor in ProcessorState and assert documents

same as before and asserts that all documents are supplied

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 87958 [details] [diff] [review]
remove default ctor in ProcessorState and assert documents

carry forward r=

Comment 55

[Johnny Stenback (:jst)]

Comment on attachment 87958 [details] [diff] [review]
remove default ctor in ProcessorState and assert documents

sr=jst

Comment 56

[Axel Hecht]

Comment on attachment 87958 [details] [diff] [review]
remove default ctor in ProcessorState and assert documents

this didn't land yet, right?

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 87958 [details] [diff] [review]
remove default ctor in ProcessorState and assert documents

Now it has :-)

Comment 58

[Peter Van der Beken [:peterv]]

Attachment: remove ArrayList/TxObjectWrapper, move URIUtils to base

Comment 59

[Peter Van der Beken [:peterv]]

Comment on attachment 87876 [details] [diff] [review]
remove txList::iterator() (sync with tip)

r=peterv

Comment 60

[Axel Hecht]

Comment on attachment 88799 [details] [diff] [review]
remove ArrayList/TxObjectWrapper, move URIUtils to base

if we'd resolve baseURI from child to parent, the txBaseURIArray could be
dropped.
Logic should be pretty simple, resolve as usual and stop on first absolute URI.
The rest looks good to me. (It's a toughy after all ;-))

Comment 61

[Peter Van der Beken [:peterv]]

Attachment: remove ArrayList/TxObjectWrapper, move URIUtils to base

Something like this. Maybe I should try it out ;-). Let's sleep first.

Comment 62

[Peter Van der Beken [:peterv]]

Note: I probably inversed the base and the relative URI.

Comment 63

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

resolving from child and up won't work for module at least (not sure what
standalones url-resolver can handle). When the uri-resolver in mozilla always
operates by resolving a relative uri against an absolute one, so you always need
to start out with a absolute one.

Comment 64

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 88799 [details] [diff] [review]
remove ArrayList/TxObjectWrapper, move URIUtils to base

>+void txBaseURLArray::add(String* aURL)
>+{
>+  if (mCount == mMax) {
>+    PRUint32 newMax = mCount + 16;
>+    String** temp = new String*[newMax];
>+    if (!temp) {
>+      return;
>+    }
>+    memcpy(temp, mURLs, mCount * sizeof(String*));
>+    delete mURLs;

delete [] mURLs

other then that, r=sicking

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

in last sentence in comment 63 s/When the/The/

Comment 66

[Johnny Stenback (:jst)]

Comment on attachment 87876 [details] [diff] [review]
remove txList::iterator() (sync with tip)

sr=jst

Comment 67

[Axel Hecht]

Comment on attachment 89014 [details] [diff] [review]
remove ArrayList/TxObjectWrapper, move URIUtils to base

Re #63, we don't do the resolving at all for module, we use
nsIDOM3Node::GetBaseURI.
And for standalone, the document doesn't have a absolute URL, too.
Unobsoleting again, but I still have to take a real look at the patch.

Comment 68

[Peter Van der Beken [:peterv]]

Attachment: remove TxObjectWrapper, move URIUtils to base

Ok, this is getting too complicated for my small mind. Let's land the piece we
all agree on.

Comment 69

[Axel Hecht]

Comment on attachment 89065 [details] [diff] [review]
remove TxObjectWrapper, move URIUtils to base

Didn't try to build it, and can´t really do that right now.
Assuming that you checked this on the tier 1s,
r=[e-mail]

Comment 70

[Axel Hecht]

spawned bug 154172 for baseURI (and ArrayList, too?), as that thing is just 
broken for standalone.

Comment 71

[Johnny Stenback (:jst)]

Comment on attachment 89065 [details] [diff] [review]
remove TxObjectWrapper, move URIUtils to base

sr=jst

Comment 72

[Peter Van der Beken [:peterv]]

Comment on attachment 89065 [details] [diff] [review]
remove TxObjectWrapper, move URIUtils to base

Checked in

Comment 73

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 87876 [details] [diff] [review]
remove txList::iterator() (sync with tip)

checked in

Comment 74

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ok, this one has become pretty long so let's close it off and open a new one if
needed

Comment 75

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.