248015 - template builder doesn't handle containerhood changes correctly

Status: NEW

(Core :: XUL, defect)

Description

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

The template builder right now does not handle having a resource becoming or
stop being a container (easy to fix), nor does it handle a container
transitioning from empty to not-empty or the other way around.  Filing this bug
to track this, as a few things are probably depending on it (bug 192624, bug
244078, bug 235665, possibly others).

It looks like there are a couple of sections of the template builder where
there's code that needs to be written to support this --
nsRdfConInstanceTestNode::Retract(), also nsXULTemplateBuilder::Retract().

A braindump, that might or might not be correct:

When a RDF container transitions from non-empty to empty, it transitions through
a non-container state: we transform OnChange into OnUnassert/OnAssert.  So, when
rdf#nextVal gets updated,  the OnUnassert removes it and makes it not a
container (though the presence of other attributes might still indicate it as
such), and the OnAssert puts it back and makes it a container again. 
OnUnassert() needs to retract the empty container rule, and OnAssert needs to
fire the non-empty-container rule.  RdfConInstanceTestNode's CanPropgate() needs
to propagate nextVal/instanceOf as well, though we don't add it to
mContainmentProperties (since they don't indicate containment, but they can
indicate a change in container status).

If this happens (as it does in my local tree full of hacks), the template
builder never looks at the container's children, thus child nodes don't get built.

Comment 1

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

Attachment: test chrome xul app to demonstrate problem

Sample test case.  Untar in chrome dir, register, run with firefox -chrome
chrome://ttest/content/

Drop down Test menu, note third item is a normal disabled menu item.  Note that
without the patch for bug 246908, it might appear as a container/submenu, even
though it's not.

Click Make Seq, check third menu item -- it should be an empty container
(submenu, one disabled Empty item)

Click Add Items, check third menu item -- it should have 3 disabled "Item"
elements in the submenu.  It will most likely end up with "Empty" followed by 3
disabled items, because the container-empty rule is still being selected.

Comment 2

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

More details on template builder brokenness:

1. nsRDFConInstanceTestNode uses nsRDFContainer's GetCount() method, which uses
the nextVal property.  nextVal is not updated if an element is removed without
renumbering (i.e. bookmarks removal with undo), so GetCount can return a value
that incorrectly indicates that the container isn't empty when it is.  Solution:
easy, stop using GetCount and instead look at outbound arcs directly (rdf:_1
first, then all arcs for any ordinal properties).  Already done in my tree.

2. nsRDFConInstanceTestNode::Retract() is not written.  It needs to check the
resource's empty/container status as in FilterInstantiations, and retract if the
rule doesn't apply.  Should be easy, already done in my tree.

3. Template builder doesn't pick the most specific rule that applies on a change
in the data source, if the current rule is still valid.  For example,

<rule container="true" isempty="true">...</rule>
<rule container="true">...</rule>
<rule>...</rule>

If a non-empty container transitions to empty, the container="true" rule is
still valid, and no transition will occur under certain instances (depending on
how the transition to empty occured).   Similarily if a non-container
transitions to being a container.

4. If a rule stops being valid as a result of an *unassertion* (only), we have
no way of walking all the rules again to see if another one matches. 
nsXULTemplateBuilder::Retract has a stub for code to fire rules "revealed" by
retractions, but these rules are only rules that were previously valid but for
which a better rule existed.  For example, for a non-empty container
transitioning to empty, the generic "<rule>" is still valid -- it ends up being
the one chosen by ::Retract(), because it has no way of walking through the tree
again to check whether any other rules are now valid.

The fix for #4 is the most difficult, as it would require a coalescing of
::Propagate() and ::Retract() into a single ::Evaluate() with a boolean
indicating whether the assertion is occuring or going away, and for it to walk
up the tree for either case.

Comment 3

[Axel Hecht]

(In reply to comment #2)
> More details on template builder brokenness:
> 
> 1. nsRDFConInstanceTestNode uses nsRDFContainer's GetCount() method, which uses
> the nextVal property.  nextVal is not updated if an element is removed without
> renumbering (i.e. bookmarks removal with undo), so GetCount can return a value
> that incorrectly indicates that the container isn't empty when it is.  Solution:
> easy, stop using GetCount and instead look at outbound arcs directly (rdf:_1
> first, then all arcs for any ordinal properties).  Already done in my tree.

rdf:_1 could be removed, with rdf:_34 still being there. So rdf:_1 not being
there probably not an indication for emptyness either.

Comment 4

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

(In reply to comment #3)
> rdf:_1 could be removed, with rdf:_34 still being there. So rdf:_1 not being
> there probably not an indication for emptyness either.

Yep, I wasn't clear in my original comments.. the logic I have looks like this:

1) check for rdf:_1 first (common case)
2) check for one of mContainmentProperties
3) go through all of ArcLabelsOut looking for ordinal arcs

2 and 3 could be combined.  Having to go through ArcLabelsOut to get this right
sort of sucks; would it be worth adding another arc to nsIRDFContainer-wrapped
containers to track the count, because GetCount is a fairly common operation? 
(Though it's most often used as an IsEmpty test more than a child count.)  This
would also make it easier to get at the total child count with aggregation.

Comment 5

[Axel Hecht]

Benjamin has container ideas, too.
Note that I myself live in a world where RDF containers come to life invalid.
That is, anything we do should work for the oddest set of rdf:_n arcs around, 
without us requiring to change those. At least we shouldn't require to change
them for read-only DSs and we shouldn't try to do so for composite DSs. Composite
DSs will have conflicting rdf:_n arcs in general.