343763 - Clean up nsPeekOffsetStruct

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Uri Bernstein]

Prompted by Aaron's comments in bug 342596, I did some clean-up of nsPeekOffsetStruct.

The main issues addressed by the patch:
- Clearly document for each member of nsPeekOffsetStruct whether it is an input parameter, an output parameter, or just for internal use.
- Reorder the data members more logically, placing input arguments before output arguments.
- Reorder the arguments to SetData() accordingly, and make sure they only include input arguments.
- Document for each argument with what values of mAmount it is actually used.
- Remove mContentOffsetEnd which was never used.
- Remove mShell which was never really used. 
- Rename mPreferLeft to mAttachForward.
- Clarify the status of mAttachForward as an output argument (remove places where it was assigned a value before calling PeekOffset).

I'll attach the patch soon.

Comment 1

[Uri Bernstein]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 228302 [details] [diff] [review]
patch

+  //                 PR_FALSE means "the frame before", PR_TRUE means "the frame after".

This should say whether this is "after" visually, or logically, or whatever.

Comment 3

[Uri Bernstein]

Attachment: updated patch

- Updated for bug 342596 (added mWordMovementType).
- Updated for bug 16203.
- Clarified the mAttachForward comment per roc's suggestion.
- I realized that mStartOffset is only actually used with eSelectCharacter and eSelectWord, so noted this fact in the comments, and moved this argument back to where it was (after mAmount and mDirection).

Comment 4

[Uri Bernstein]

Attachment: what I checked in

The previous patch attached here included a fix for another bug. This is what I actually checked in.

Comment 5

[Uri Bernstein]

Checking in layout/generic/nsFrameSelection.h;
/cvsroot/mozilla/layout/generic/nsFrameSelection.h,v  <--  nsFrameSelection.h
new revision: 1.5; previous revision: 1.4
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.246; previous revision: 3.245
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.659; previous revision: 3.658
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.582; previous revision: 1.581
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.926; previous revision: 3.925
done

Comment 6

[Uri Bernstein]

Attachment: Fix for nsHyperTextAccessible.cpp [checked in]

I was unaware that bug 312093 was already checked in, so my patch caused a tree bustage. This is what I checked in to fix it. Aaron - can you please have a look and make sure this is OK?