246908 - template builder incorrectly identifies containers if isempty is present in rule

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8


If a template rule is specified such as <rule iscontainer="true"
isempty="true">, the test for containerhood is never done, and all
non-containers are identified as empty containers.  (see note in
browser-menubar.inc regarding explicitly specifying the Folder type in the
bookmarks template, otherwise everything looks like a container).

Attached patch fixes the issue in nsRDFConInstanceTestNode.cpp.


Reproducible: Always
Steps to Reproduce:

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: proposed fix

Comment 2

[[e-mail]]

Comment on attachment 150842 [details] [diff] [review]
proposed fix

I don't pretend to understand this patch, but it might be possible to sneak in
a small optimization. Note that mEmpty and mContainer have three values, true,
false and don't care, while empty and container have two values true and false.
Currently you have a rather complex condition to determine whether the rule
matches. It is in fact simpler to tell if the rule does not match. An empty of
true only mismatches an mEmpty of false and vice versa. Thus instead of empty
and container, use haschildren and leaf. Either mEmpty == haschildren or
mContainer == leaf would indicate a mismatch.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: proposed fix

I misunderstood the usage of ArcLabelsOut; this patch does the ArcLabelsOut
check as well, as necessary.  I didn't incorporate the suggestion for using a
haschildren/leaf type test, though it would be a valid approach -- the logic is
convoluted enough that negating the test in order to get rid of 3-4 simple
compares doesn't seem worth it, though I can make the change if there is
demand..

Comment 4

[[e-mail]]

Comment on attachment 150914 [details] [diff] [review]
proposed fix

>+                        NS_ASSERTION(property != nsnull, "not a property");
>+                        if (! property)
>+                            return NS_ERROR_UNEXPECTED;
Any potential reviewer would probably appreciate a diff -w otherwise they're
going to wonder at some of the less usual constructs, such as this one, which
is normally written as NS_ENSURE_TRUE(property, NS_ERROR_UNEXPECTED);

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: diff -w version of patch

As suggested, diff -w version of patch..

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 151273 [details] [diff] [review]
diff -w version of patch

sr=shaver, but let's get a XUL or RDF expert to review for the trunk.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 151273 [details] [diff] [review]
diff -w version of patch

r=[e-mail]

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

in on trunk and aviary.