Closed Bug 74800 Opened 23 years ago Closed 23 years ago

Implement FIXptr support

Categories

(Core :: XML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: hjtoi-bugzilla, Assigned: hjtoi-bugzilla)

Details

(Keywords: testcase, Whiteboard: [fix in hand])

Attachments

(4 files, 3 obsolete files)

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]* ')'
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.
Target Milestone: --- → Future
Keywords: patch, testcase
Whiteboard: [first cut fix in hand]
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: Future → mozilla0.9.5
Ok, I think I am going to check this in after all. A little cleanup etc.,
scheduling for 0.9.5 for now.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attachment #29765 - Attachment is obsolete: true
Attachment #46936 - Attachment is obsolete: true
Attachment #46953 - Attachment is obsolete: true
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.
Whiteboard: [first cut fix in hand] → [fix in hand]
Target Milestone: mozilla0.9.7 → mozilla0.9.8
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);
  ....
 }
}
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.
>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 on attachment 61628 [details] [diff] [review]
Proposed patch (also scriptable)

Forgot to r=.
Attachment #61628 - Flags: review+
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.
Yes, it is 1-based. I'll see if the selection thing you suggested works, it
looks a lot nicer. Thanks!
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.
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
The selection problem is bug 117818, and unbalanced FIXptr is bug 117821.
QA Contact: petersen → rakeshmishra
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: