330660 - Template builder refuses to propagate assertions

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jan Varga [:janv]]

Comment 1

[Jan Varga [:janv]]

Attachment: testcase.xul

Comment 2

[Jan Varga [:janv]]

Attachment: testcase.rdf

Comment 3

[Jan Varga [:janv]]

Attachment: trunk patch provided by Neil Deakin

Comment 4

[Jan Varga [:janv]]

Neil, I slightly modified your patch in nsRDFPropertyTestNode.cpp
-            *aCantHandleYet = PR_TRUE;
+            *aCantHandleYet = PR_TRUE;
+            return NS_OK;

I think we can safely return here, because we're doing the same thing in nsRDFConMemberTestNode.cpp and the method will be called again anyway.

Comment 5

[Jan Varga [:janv]]

Attachment: patch for firefox 1.0

Comment 6

[Neil Deakin]

Actually, I want to make a patch here which doesn't use default arguments.

Comment 7

[Jan Varga [:janv]]

ok, but the functionality will be the same?

Comment 8

[Neil Deakin]

Yes, the same thing, but change the FilterInstantiations to not use a default argument, and then fix up the callers. You can get bz or vlad to review the  template code.

Comment 9

[Neil Deakin]

Attachment: if a rule doesn't have every variable specified, skip it and try again on the way down

The issue occurs when we have, for example, a template with rules like

?a -> ?b
?b -> ?c
?a -> ?d

When a new RDF assertion is made such that ?a -> ?d, the builder traverses up to fill in the remaining variables. This is done by the Constrain method. However, in the case above, it can't get any farther as neither ?b nor ?c are known.

This patche changes the navigation to skip such underspecified rules and carry on to the next earlier step ?a -> ?b which can be filled in since ?a is known. The middle step will be filled in on the way back down.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216348 [details] [diff] [review]
if a rule doesn't have every variable specified, skip it and try again on the way down

>Index: content/xul/templates/src/nsContentTestNode.cpp
>+nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations,
>+                                        PRBool* aCantHandleYet) const
>+
> {
>     return NS_OK;

Not setting out params and returning NS_OK is just not cool...  Please set it as needed?

>Index: content/xul/templates/src/nsRDFConInstanceTestNode.cpp

Same.

>Index: content/xul/templates/src/nsRDFConMemberTestNode.cpp

Same for any codepath that doesn't hit the new code you added.

>         if (! hasContainerBinding && ! hasMemberBinding) {
>             // Neither container nor member assignment!
>-            NS_ERROR("can't do open ended queries like that!");
>-            return NS_ERROR_UNEXPECTED;
>+            if (!aCantHandleYet) {
>+                return NS_ERROR_UNEXPECTED;

Would it make sense to log a message to the JS console here?  If so, followup bug on that?  If not, all's good.

>Index: content/xul/templates/src/nsRDFPropertyTestNode.cpp

Same comments as for MemberTestNode.

>Index: content/xul/templates/src/nsRuleNetwork.cpp
>         // if we still have instantiations, then ride 'em on up to the
>         // parent to narrow them.

Does this comment need adjusting too?

>Index: content/xul/templates/src/nsRuleNetwork.h

>+    virtual nsresult FilterInstantiations(InstantiationSet& aInstantiations,
>+                                          PRBool* aCantHandleYet) const = 0;

Please document the new arg (especially that it's allowed to be null and hence isn't an XPCOM out param and what it means for it to be null).

r+sr=bzbarsky with those nits picked.