Closed Bug 343763 Opened 16 years ago Closed 16 years ago

Clean up nsPeekOffsetStruct

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: uriber, Assigned: uriber)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #228302 - Flags: review?(roc)
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.
Attachment #228302 - Flags: superreview+
Attachment #228302 - Flags: review?(roc)
Attachment #228302 - Flags: review+
Attached patch updated patchSplinter Review
- 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).
Attachment #228932 - Flags: superreview?(roc)
Attachment #228932 - Flags: review?(roc)
Attachment #228302 - Attachment is obsolete: true
No longer blocks: 342596
Attachment #228932 - Flags: superreview?(roc)
Attachment #228932 - Flags: superreview+
Attachment #228932 - Flags: review?(roc)
Attachment #228932 - Flags: review+
The previous patch attached here included a fix for another bug. This is what I actually checked in.
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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?
Attachment #229053 - Flags: review?(aaronleventhal)
Attachment #229053 - Flags: review?(aaronleventhal) → review+
You need to log in before you can comment on or make changes to this bug.