46134 - allow >1 value per attribute in XUL template substitution

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Chris Waterson]

XUL templates need to support >1 "substitution property"; for example, using 
the simple template syntax:

  <treeitem
    class="rdf:http://home.netscape.com/NC-rdf#isMessage,
           rdf:http://home.netscape.com/NC-rdf#isRead" />

Or in the extended syntax:

  <treeitem class="$isMessage $isRead" />

Besides being nifty, this enables two key optimizations for large templates 
(like mail/news):

  1. It requires fewer attributes to be created on XUL elements, this
     accounts for the bulk of the expense in time when loading a
     mailnews folder.

  2. It allows the style rules to be written based on class, rather than
     on attribute. The style system hashes rules by class, so this makes
     style resolution O(1) rather than O(n).

We need to be a bit careful about how we do this to avoid excessive string 
munging and memory allocation. Ideally, supporting this will not degrade 
performance of the simple, single-substitution case.

Comment 1

[Alec Flett]

I was looking at this last night, and there's one thing that threw up a red flag
for me..

in mail, we have a few extra "static" classes for all treecells, currently in
the class attribute as class="treecell-indent tree-cell-threadpane-icon" and so
forth.

This means that we'll need to have support for both static text and
rdf-substituted text within the same attribute, so we should be sure to handle
the case where a token in the attribute does not contain rdf:

Comment 2

[Chris Waterson]

Attachment: first cut

Comment 3

[Chris Waterson]

cc'ing scc for string fu.

Comment 4

[Chris Waterson]

Notes to self after re-reading patch...

- the case where we run off the end of the string is handled wrong.
- need to clarify & think about what might separate a variable name from other
content in the string

Comment 5

[Chris Waterson]

Attachment: second cut, deal with above issues

Comment 6

[Robert John Churchill]

(I like nsPromiseString)  r=rjc (on 2nd cut)

Comment 7

[Chris Waterson]

fix checked in

Comment 8

[John Morrison]

Hey Chris. I have a simple test case (stolen from pref-themes.xul) that shows
me that this is working. However, the substituted value for a multi-sustitution
is the concatenated string from the matches. In other words:

<treecell 
 value="rdf:http://ww...rg/rdf/chrome#name  rdf:http://ww..rg/rdf/chrome#display
Name"/>

gets the result "Classicclassic/1.0" instead of "Classic classic/1.0".

That, of course, doesn't give you the right thing, should you want to
use these matches to set css class="class-one class-two class-three".

I'll attach my testcase (and hopefully it doesn't have some bone-headed 
mistake).

Comment 9

[John Morrison]

Attachment: simple template to test multiple substitutions

Comment 10

[Chris Waterson]

Believe it or not, this was actually what I intended to do. The reasoning goes
something like this: the "end" of the variable is indicated using a space; that
space is consumed during processing; if you want to have a space in the
resulting template, you need to put *two* spaces in.

In retrospect, I think it was a dumb idea! I'll attach a patch shortly that will
fix this; if we ever do need the ability to jam something "right up next to" a
variable, then we'll add another special character that means "the variable ends
here".

Comment 11

[Chris Waterson]

Attachment: Use "^" character to provide concatenation instead of two spaces; fix math error with substrings

Comment 12

[Chris Waterson]

What the hell. The above patch gets it right (I think), and fixes another match
error to boot.

If you really want to concatenate, with no spaces in between, you can terminate
your variable name with a caret ("^") character (which, per RFC2396, must be
%-encoded if it appears in a URI for real). Otherwise, a variable simply ends
when a space is encountered.

The math error was a misunderstanding of how nsPromiseSubstring<*> worked: I'd
assumed the ctor took a start and end position; it actually takes a start
position and a length.

Comment 13

[Scott Collins]

Sorry about that; I was following the old syntax from |Substring|, I think.  Is 
this not intuitive?  My bad.  In the future, the API will probably change to take 
a pair of iterators, as with |nsSlidingSubstring| now.  A special variation for 
flat strings will take a pair of character pointers.  This combination will be 
more efficient in every case, and will not encourage confusion since, when using 
iterators, points and distances have different data types; this is not so when 
using indexes.

Comment 14

[Robert John Churchill]

Blessing with a r=rjc.

Comment 15

[Chris Waterson]

fix checked in.

Comment 16

[Chris Waterson]

no, really!

Comment 17

[John Morrison]

verified fixed. A space between two RDF URL remains a space, '^' can be used 
as a catenation operator, and a non-rdf string is not altered as an attribute
value, so 

 class="someClass  rdf:http...#prop1  rdf:http:...#prop2^rdf:http:...#prop3"
   appears as 
 class="someClass value1 value2value3"

This also works using the advanced syntax for templates. Sweet. 

(The "^" operator is not consumed when it trails a static value:
    |foobar^rdf:http://...#prop1|    appears as   |foobar^value1|             
although, it is consumed in this case
    |rdf:http://...#prop1^foobar|    appears as   |value1foobar|             
)

Comment 18

[Chris Waterson]

In the last case, you can put two carets in; e.g.,

  |rdf:http://...#prop1^^foobar|

to produce:

  |value1^foobar|

Thanks for the extensive testing! I'll need to go update the template docs at
some point...

Comment 19

[John Morrison]

Actually, it would be nice to have the ^ operator work in the case of 
  
   staticname^rdf:..#prop1   producing   staticnameValue1

So that a datasource which, for example, has a property with two values, can
be convinced to produce a reasonably legible classname:

     message-status-^rdf:..#prop1   producing   message-status-unread

Comment 20

[Alec Flett]

good lord, that would kick ass. I was just starting to think about that. this 
will allow us to speed up mail just by fixing XUL, not our datasources.
Once again, nice job chris!

Comment 21

[Chris Waterson]

jgrm: can't you just do "message-status-rdf:..#prop1" to get
"message-status-unread". The delimiter is really only needed on the back-end of
the variable name so I can know when to stop parsing the RDF property...

Comment 22

[John Morrison]

Indeed you can (I thought the ' rdf:' was the delimiter on the front end. 
So you produce classnames like:

    twostate-rdf:...#p1^-rdf...#p2    ->   twostate-true-false

Comment 23

[Chris Waterson]

I screwed this up, which I discovered when I was working on bug 53627 :-(. The
case that I missed was AddSimpleRuleBindings(), which computes the bindings for
the "simple" syntax. Attaching a fix that works, and also cleans up a bunch of
copy-n-paste of complicated code.

Comment 24

[Chris Waterson]

Attachment: Factor attribute parsing logic; make sure to call from AddSimpleRuleBindings()

Comment 25

[Chris Waterson]

Removing [nsbeta3+], but we'll need to reconsider if we decide to include the
fix for bug 53627...

Comment 26

[Chris Waterson]

Attachment: reworked to compile on egcs-1.1.2, which can't stomach operator->*() and the string template foo in the same bite.

Comment 27

[Chris Waterson]

Had to re-work the patch because egcs-1.1.2 can't stomach operator->*() and the
string template foo at the same time. So I made the callbacks be static members
of nsXULTemplateBuilder, and explicitly pass a "this" parameter to them. Gonna
verify the fancy C++ on gcc-2.7.2.3.

Oh, also fixed an ommission where I should've been looking for "..." as well as
"rdf:*".

Comment 28

[Chris Waterson]

Fix checked in on the trunk. Nominating for RTM because of 53627 dependency.

Comment 29

[Michael La Guardia]

Marking rtm++.  Please check this in as soon as possible.

Comment 30

[Chris Waterson]

Attachment: patch for the branch; includes subsequent gcc-2.7.2.3 & OS/2 cleanup

Comment 31

[Chris Waterson]

For posterity's sake (and in case we need to back it out), the last patch is the
patch that I am going to check in on the branch: it includes some gcc-2.7.2.3
cleanup, as well as mkaply's OS/2 cleanup.

Comment 32

[Chris Waterson]

Fixed on branch.

Comment 33

[John Morrison]

Verified fixed as far as my pea-brain tells me (at least until waterson finds 
the next thing to fix :-]). marking vtrunk.