Last Comment Bug 726287 - misc decomtamination in nsHypertextAccessible
: misc decomtamination in nsHypertextAccessible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-02-11 02:56 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-03-07 02:22 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
start (8.44 KB, patch)
2012-02-11 02:59 PST, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch 2 (8.68 KB, patch)
2012-03-06 05:02 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-02-11 02:56:04 PST

    
Comment 1 Trevor Saunders (:tbsaunde) 2012-02-11 02:59:04 PST
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)
Comment 2 alexander :surkov 2012-02-12 19:42:30 PST
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
Comment 3 alexander :surkov 2012-02-29 20:29:02 PST
Trevor, it'd be great if you get a chance to finish and land it.
Comment 4 Trevor Saunders (:tbsaunde) 2012-03-06 05:02:10 PST
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?
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-07 02:22:33 PST
https://hg.mozilla.org/mozilla-central/rev/aa7d28b45ede

Note You need to log in before you can comment on or make changes to this bug.