121485 - nsXULContentSink is too nsAutoString happy...

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Johnny Stenback (:jst)]

NormalizeAttributeString(), PushNameSpacesFrom(), and ParseTag() in
nsXULContentSink create nsAutoString's (3-4 per attribute, 2 per tag) and copy
string data for doing simple string manipulation that can be done much faster
and w/o any string copying by using string iterators. Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Use string iterators in stead of temporary nsAutoString's...

Comment 2

[Johnny Stenback (:jst)]

dp, hyatt, would you guys [s]r=?

Comment 3

[Chris Waterson]

Oy. Lose the paranoid NS_ENSURE_ stuff already! ;-)

Comment 4

[Johnny Stenback (:jst)]

Attachment: Less paranoid error checking...

Well, I removed some of the NS_ENSURE_* stuff, moved some a bit, and replaced
some with other error checking, but none of them were really paranoid unless we
don't care about OOM situations in this code.

Comment 5

[Chris Waterson]

Comment on attachment 66170 [details] [diff] [review]
Less paranoid error checking...

I still despise NS_ENSURE_*, because it hides flow-of-control changes, and
would prefer:

  if (NS_FAILED(rv)) return rv;

wherever they are used. But I'll grant my sr= anyway and grin.

>+    if (mNameSpaceStack.Count() > 0) {
>+        nameSpace =
>+            (nsINameSpace*)mNameSpaceStack.ElementAt(mNameSpaceStack.Count() - 1);
>+    } else {
>+        gNameSpaceManager->CreateRootNameSpace(*getter_AddRefs(nameSpace));
>+        NS_ENSURE_TRUE(nameSpace, NS_ERROR_OUT_OF_MEMORY);
>+    }

The above idiom makes me green around the gills. How about:

  if (! nameSpace)
    return NS_ERROR_OUT_OF_MEMORY;

instead of |NS_ENSURE_TRUE(...)|.


>+    static const NS_NAMED_LITERAL_STRING(kNameSpaceDef, "xmlns");
>+    static const PRUint32 xmlns_len = kNameSpaceDef.Length();

FWIW, the above is going to cause a global variable access for each use
of xmlns_len, instead of something that can be computed at compile time.

The rest of the string-fu changes look fine.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Fix the issues waterson brought up.

I know there are mixed feelings about the NS_ENSURE_* macros, I tend to like
them in most cases since they will actually tell you that something that wasn't
supposed to go wrong went wrong, but you're right, here I was over-using them.

I changed the static length to be a hard coded integer and added an assert that
makes sure that integer matches the length of the string in question.

Comment 7

[Nisheeth Ranjan]

Comment on attachment 66206 [details] [diff] [review]
Fix the issues waterson brought up.

r=nisheeth

Comment 8

[Johnny Stenback (:jst)]

Fixed.