74800 - Implement FIXptr support

Status: RESOLVED FIXED

(Core :: XML, enhancement, P3)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

FIXptr is (yet to be published, if ever) subset of XPointer.

EBNF for FIXptr:

[1]    fix            ::=    (Name | init-selector) child-seq? char-offset?
[2]    init-selector  ::=    '/1'
[3]    child-seq      ::=    ('/' [1-9] [0-9]* )+
[4]    char-offset    ::=    '(' [1-9] [0-9]* ')'

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: As-fast-as-possible implementation of FIXptr (3h)

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, here is a pretty ugly and quickly done implementation of FIXptr. It took
about 3 hours to implement. It is probably possible to create a testcase where
it crashes, I haven't done any more testing than in the provided testcase. Pull
the latest Mozilla source, apply the patch, compile and add this line to your all.js

pref("layout.selectanchor",true); // Select link end on traversal so we can see

Run Mozilla, open the testcase, click on the links and be pleased ;)

So basically with this patch you can now make links that point into XML
documents use FIXptr syntax in the fragment part to point to content that does
not have an ID attribute, and even point to a character inside a text node.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Update diff, add support for latest FIXptr (untested)

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The FIXptr W3C NOTE is at:
http://lists.w3.org/Archives/Public/www-xml-linking-comments/2001AprJun/att-0074/01-NOTE-FIXptr-20010425.htm

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Somewhat tested patch

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, I think I am going to check this in after all. A little cleanup etc.,
scheduling for 0.9.5 for now.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Testcase with pointer pairs

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed patch (also scriptable)

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Scriptable testcase

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, this is starting to look like it is close to landing. Will try to get in as
soon as tree opens for 0.9.8 development.

I will still need to test this with FIXptr test suite, provided I can find it.

The implementation is not fast by any means, but I don't think that speed is an
issue at this point. It can be improved when needed.

Comment 12

[harishd]

Comment on attachment 61628 [details] [diff] [review]
Proposed patch (also scriptable)

>+// Get nth child element
>+static nsresult GetChild(nsIDOMNode *aParent, PRInt32 aChildNum, nsIDOMNode **aChild)
>+{

aChildNum could be a const. no?

>+  PRUint32 i;
>+  PRInt32 curChildNum = 0;
>+  for (i = 0; i < count; i++) {

Suggestion:
You can limit the scope of the variable "i" to the for-loop.
i.e. for (PRUint32 i = 0; i < count; i++)

>>+// Get range for char index
>+static nsresult GetCharRange(nsIDOMNode *aParent, PRInt32 aCharNum, nsIDOMRange **aRange)

use const wherever possible.

>+  nsCOMPtr<nsIDOMNode> node(aParent);
>+  while (!tumbler.IsEmpty() && node) {

Try to avoid the function overhead.

>+              nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID));
>+              nsCOMPtr<nsIDOMNode> node(do_QueryInterface(content));
>+              if (range && node) {
>+                range->SelectNode(node);
>+                SelectRange(range);
>+              

This could be written as
nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID));
if (range) {
 nsCOMPtr<nsIDOMNode> node(do_QueryInterface(content));
 if (node) {
  range->SelectNode(node);
  ....
 }
}

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I don't understand the comments about consts - these are integer parameters so
it doesn't really matter in my opinion.

Also the scope with for loops is a bit tricky, newer compilers get it right,
older compilers can stumble. It is one of our portability guidelines that you
should declare variables outside the for loop in situations like these to avoid
confusion.

Also the comment about function overhead does not make sense, since tumbler
variable changes inside the loop and it is one of the conditions of staying in
the loop to see if tumbler is empty.

The final thing is a style issue, which I can change if you want.

Comment 14

[harishd]

>I don't understand the comments about consts - these are integer parameters so
>it doesn't really matter in my opinion.
It doesn't matter whether it's an integer or not. If you're not planning on
changing the value why not use const. IMO, it is a better practice :-)

>Also the comment about function overhead does not make sense, since tumbler
>variable changes inside the loop and it is one of the conditions of staying in
>the loop to see if tumbler is empty.

doh!. Ignore my comment then.

Comment 15

[harishd]

Comment on attachment 61628 [details] [diff] [review]
Proposed patch (also scriptable)

Forgot to r=.

Comment 16

[vidur (gone)]

Comment on attachment 61628 [details] [diff] [review]
Proposed patch (also scriptable)

FIXPtr child and character offsets are 1-based, eh?

A small nit - could the block:

>+    nsCOMPtr<nsIDOMNode> begin,end;
>+    PRInt32 beginOffset, endOffset;
>+
>+    aRange->GetStartContainer(getter_AddRefs(begin));
>+    aRange->GetStartOffset(&beginOffset);
>+
>+    aRange->GetEndContainer(getter_AddRefs(end));
>+    aRange->GetEndOffset(&endOffset);
>+
>+    sel->Collapse(begin, beginOffset);
>+    sel->Extend(end, endOffset);    

not be replaced with:

	sel->RemoveAllRanges();
	rv = sel->AddRange(aRange);

Other than that, sr=vidur.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yes, it is 1-based. I'll see if the selection thing you suggested works, it
looks a lot nicer. Thanks!

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, I've discovered two bugs which I've fixed: if the char offset was too big we
should have stopped evaluation, and we were not checking if the charoffset ended
in a closing parenthesis (the latter I found by running the FIXptr test suite).
I I have fixed these in my tree, minor things... I also made the change vidur
suggested, it works.

There are a couple of things I am not sure, though.

Pointer pairs in reverse document order. evaluateFIXptr does return a range
object, but selection code will not select this. It could be argued that this is
a bug in the selection code.
Unbalanced pointers, meaning that the second one starts or ends somewhere inside
the area of the first pointer, or vice versa. Currently evaluateFIXptr returns a
range where the start is the start of the first FIXptr and the end the end of
the second FIXptr.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The selection problem is bug 117818, and unbalanced FIXptr is bug 117821.