misc decomtamination in nsHypertextAccessible

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 596296 [details] [diff] [review]
start

There's plenty more to do here, but I think this is an improvement (atleast I do at 0600)
Attachment #596296 - Flags: review?(surkov.alexander)

Comment 2

6 years ago
Comment on attachment 596296 [details] [diff] [review]
start

Review of attachment 596296 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +1777,5 @@
>  
>    // Remove collapsed ranges
>    PRUint32 numRanges = aRanges->Length();
>    for (PRUint32 count = 0; count < numRanges; count ++) {
> +    if ((*aRanges)[count]->Collapsed()) {

while you are here, could you rename count to idx please?

@@ +1829,4 @@
>  
>    // Get end point
> +  nsINode* endNode = range->GetEndParent();
> +  PRInt32 endOffset = range->EndOffset();

you can keep these under common comment "Get start and end points"

@@ +1833,2 @@
>  
> +  // Make sure start is before end, by swapping offsets

by swapping DOM points, dot in the end

@@ +1833,3 @@
>  
> +  // Make sure start is before end, by swapping offsets
> +  // This occurs when the user selects backwards in the text

keep this on the same line as prev comment, dot in the end

@@ +2342,5 @@
>    for (PRUint32 index = 0; index < rangeCount; index++) {
>      nsRange* range = ranges[index];
>  
>      PRInt16 result;
> +    nsresult rv = range->ComparePoint(DOMNode, aNodeOffset, &result);

you could add dexcpomed version of ComparePoint to nsRange, up to you

@@ +2350,5 @@
>      // See also bug 460690.
>      if (result == 0) {
> +      nsINode* endNode = range->GetEndParent();
> +      PRInt32 endOffset = range->EndOffset();
> +      if (aNode == endNode && aNodeOffset == endOffset)

sounds like you don't need local variables for those
Attachment #596296 - Flags: review?(surkov.alexander) → review+

Comment 3

6 years ago
Trevor, it'd be great if you get a chance to finish and land it.
(Assignee)

Comment 4

6 years ago
Created attachment 603237 [details] [diff] [review]
patch 2

aAlex, iirc this fixes all of your nits accept the qi to nsIDOMNode for a nsRange method which I think I'm for leaving be for now.  I really doubt I'll have time to deal with try etc to land this before friday, so if you want this done before then would you mind landing it?
Attachment #596296 - Attachment is obsolete: true

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7d28b45ede
https://hg.mozilla.org/mozilla-central/rev/aa7d28b45ede
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.