Closed Bug 330305 Opened 19 years ago Closed 19 years ago

DeCOMtaminate nsIFrameSelection.h

Categories

(Core :: Layout, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: marcldl+mozbugs, Unassigned)

References

Details

Attachments

(3 files, 9 obsolete files)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1 nsIFrameSelection should be deCOMtaminated Reproducible: Always Steps to Reproduce:
Attached patch Change all methods and fix callers (obsolete) (deleted) — Splinter Review
This makes most methods return a sensible thing where possible. I also removed the return type entirely where it was ignored or always NS_OK. If there's anywhere where I can changes nsCOMPtr to using a raw pointer then I can do that and possibly some methods can be inlined eventually depending on what happens to nsIFrameSelection.
Attachment #214876 - Flags: review?(roc)
Status: UNCONFIRMED → NEW
Ever confirmed: true
It looks like nsSelection is the only implementation of nsIFrameSelection. Let's replace nsIFrameSelection with nsSelection and pull the class definition of nsSelection to nsSelection.h. You'll need to predeclare "class nsTypedSelection;" to get that to compile, and pull in headers like nsIDOMNode.h and a few others, but it shouldn't require anything new to be exported. In fact it looks like the only call through nsIFrameSelection outside of content/ and layout/ is one call from dom/, and we can eliminate that by creating a small new helper method in nsIPresShell. You can then devirtualize all the nsSelection methods and inline any very simple ones. Also, since nsSelection has a presshell, you can eliminate all prescontext parameters from its methods.
This patch just converts uses of nsIFrameSelection to nsSelection. I removed some includes of nsIFrameSelection/nsSelection where they didn't seem to be needed. If any of these are wrong then let me know. It compiles and doesn't seem to crash when I select random things in random ways.
Attachment #214876 - Attachment is obsolete: true
Attachment #216597 - Flags: review?(roc)
Attachment #214876 - Flags: review?(roc)
Attached file new nsSelection.h file (obsolete) (deleted) —
This is the new file. Probably needs review? too.
Attachment #216600 - Flags: review?(roc)
Attached file new nsSelection.h file (obsolete) (deleted) —
I forgot to transfer the comments from nsIFrameSelection.
Attachment #216600 - Attachment is obsolete: true
Attachment #216601 - Flags: review?(roc)
Attachment #216600 - Flags: review?(roc)
You can do "cvs add layout/base/nsSelection.h" and then "cvs diff -N" to get the new file in your diff.
virtual nsresult GetPrevNextBidiLevels( Comment that it's virtual because it's used outside of gklayout. NS_IMETHOD StartBatchChanges(); NS_IMETHOD EndBatchChanges(); NS_IMETHOD DeleteFromDocument(); Why do these need to be virtual? It looks kinda bad that nsSelection doesn't implement nsISelection. If it's not too much work, I think we should rename nsSelection to nsFrameSelection. One thing you could do (or could have done) to reduce the size of this patch is put in nsSelection.h typedef nsSelection nsIFrameSelection; Then we can either leave it like that or in a separate patch just rename nsIFrameSelection to nsSelection. Splitting the change up like that would make it easier for you and for me. /** ShutDown will be called when the owner of the frame selection is shutting down * this should be the time to release all member variable interfaces. all methods * called after ShutDown should return NS_ERROR_FAILURE */ void ShutDown() {}; Just remove it? void HandleDrag(nsIFrame *aFrame, nsPoint& aPoint); Make this const nsPoint& or just nsPoint. Same in StartAutoScrollTimer. /** * ScrollSelectionIntoView scrolls a region of the selection, * so that it is visible in the scrolled view. * * @param aType the selection to scroll into view. * @param aRegion the region inside the selection to scroll into view. * @param aIsSynchronous when PR_TRUE, scrolls the selection into view * at some point after the method returns.request which is processed */ void ClearTableCellSelection(){ mSelectingTableCellMode = 0; } Wrong comment. nsresult GetFrameForNodeOffset(nsIContent *aNode, PRInt32 aOffset, HINT aHint, nsIFrame **aReturnFrame, PRInt32 *aReturnOffset); I think just return the frame as the function result, and nsnull on error. It doesn't really matter what the error is. virtual nsresult GetPrevNextBidiLevels(nsIContent *aNode, PRUint32 aContentOffset, PRBool aJumpLines, nsIFrame **aPrevFrame, nsIFrame **aNextFrame, PRUint8 *aPrevLevel, PRUint8 *aNextLevel); Put these 4 results into a struct and return that? nsresult GetFrameFromLevel(nsIFrame *aFrameIn, nsDirection aDirection, PRUint8 aBidiLevel, nsIFrame **aFrameOut); Again I don't think we really care what the result is. Just return aFrameOut. + [noscript] nsSelection getFrameSelection(); It's a bit unfortunate that we have to use IDL for nsISelectionPrivate, otherwise we could deCOMtaminate it. Perhaps what we should do is create nsISelection2.idl which extends nsISelection, and contains (for now) just interlinePosition which is the only thing needed from scripts. Then rename nsTypedSelection to nsSelection (assuming we've renamed nsSelection to nsFrameSelection already), and remove nsISelectionPrivate in favour of using a deCOMtaminated nsSelection. For another bug... @@ -1313,22 +1313,21 @@ nsEventStateManager::FireContextClick() + nsCOMPtr<nsSelection> frameSel This doesn't need to be an nsCOMPtr because we don't do anything that could destroy frameSel before the last use of it. @@ -1470,22 +1450,20 @@ nsEventStateManager::GenerateDragGesture ditto @@ -4953,20 +4935,18 @@ nsEventStateManager::MoveCaretToFocus() + nsCOMPtr<nsISelection> domSelection ditto @@ -5014,29 +4994,30 @@ nsresult + nsCOMPtr<nsSelection> frameSelection; + nsCOMPtr<nsISelection> domSelection = docFrameSelection-> ditto Index: layout/base/Makefile.in - nsIFrameSelection.h \ Don't you need to export nsSelection.h here? + nsCOMPtr<nsISelection> domselection = mSelection-> + GetSelection(nsISelectionController::SELECTION_NORMAL); if (!domselection) - return NS_ERROR_UNEXPECTED; + return NS_ERROR_INVALID_ARG; Do we really need this? All it's doing is maybe returning an error code instead of NS_OK. (The call to Shutdown() afterward doesn't do anything so skipping it has no effect.) Take it out. @@ -3677,19 +3691,18 @@ PresShell :: + nsCOMPtr<nsISelection> domSelection = mSelection-> this can be a regular pointer + nsCOMPtr<nsISelection> sel = mSelection->GetSelection(nsISelectionController::SELECTION_NORMAL); ditto + if (!sel) {//get selection from this PresShell 'sel' is never assigned above, so it's always null here. Remove 'sel' and just set *outSelection directly. -nsresult NS_NewSelection(nsIFrameSelection** aResult); +nsresult NS_NewSelection(nsSelection** aResult); nsresult NS_NewDomSelection(nsISelection** aResult); nsresult NS_NewDocumentViewer(nsIDocumentViewer** aResult); nsresult NS_NewRange(nsIDOMRange** aResult); nsresult NS_NewRangeUtils(nsIRangeUtils** aResult); nsresult NS_NewContentIterator(nsIContentIterator** aResult); nsresult NS_NewPreContentIterator(nsIContentIterator** aResult); nsresult NS_NewGenRegularIterator(nsIContentIterator** aResult); nsresult NS_NewContentSubtreeIterator(nsIContentIterator** aResult); All of these could be deCOMtaminated in another bug :-) (making them functions returning nsAlready_Addrefed) if (mFrameSelection) - return mFrameSelection->SetDisplaySelection(aToggle); + { + mFrameSelection->SetDisplaySelection(aToggle); + return NS_OK; + } return NS_ERROR_NULL_POINTER; Rewrite this as if (!mFrameSelection) return NS_ERROR_NULL_POINTER; mFrameSelection->SetDisplaySelection(aToggle); return NS_OK; and similar below + if (!(*_retval)) + return NS_ERROR_INVALID_ARG; ERROR_FAILURE? - if (!mPresShellWeak) return NS_ERROR_NOT_INITIALIZED; - nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShellWeak); - if (presShell) - { - nsPresContext *context = presShell->GetPresContext(); - if (context) - { - return mFrameSelection->RepaintSelection(context, type); - } - } - return NS_ERROR_FAILURE; Why is it safe to get rid of this? (It probably is, I just want to make sure.) Maybe check mFrameSelection for null-ness instead? @@ -696,18 +699,19 @@ nsTextInputSelectionImpl::SetCaretReadOn + nsCOMPtr<nsISelection> domSel = mFrameSelection-> Regular pointer @@ -718,18 +722,19 @@ nsTextInputSelectionImpl::GetCaretEnable ditto @@ -740,18 +745,19 @@ nsTextInputSelectionImpl::SetCaretVisibi ditto + nsCOMPtr<nsIContent> parentDIV = mFrameSelection->GetLimiter(); regular pointer @@ -1607,23 +1605,24 @@ nsFrame::GetDataForTableSelection(nsIFra + nsCOMPtr<nsIContent> limiter = aFrameSelection->GetLimiter(); regular pointer + *aFrameSelection = (nsSelection *)rlist; cast not needed + nsCOMPtr<nsIContent> tLimiter = aFrameSel->GetLimiter(); Regular pointer + if (mFrameSelection->GetHint() == nsSelection::HINTRIGHT) *aHintRight = PR_TRUE; else *aHintRight = PR_FALSE; *aHintRight = mFrameSelection->GetHint() == nsSelection::HINTRIGHT; + if (!aContent || !mShell) Where did the mShell check come from? - if (mMouseDownState == aState) - return NS_OK; - mMouseDownState = aState; - if (!mMouseDownState) + if (mMouseDownState != aState) Doing the early exit is preferred to nesting everything. + return NS_REINTERPRET_CAST(nsISelection*, mDomSelections[index]); Can you make this an NS_STATIC_CAST? + nsCOMPtr<nsISelection> domSel = + aFrameSel->GetSelection(nsISelectionController::SELECTION_NORMAL); regular pointer In nsSelection::GetDelayedCaretData() + return 0; return nsnull;
A few lines above is |rv = selCon->GetSelection(nsISelectionController::SELECTION_NORMAL, getter_AddRefs(sel));| Did you mean you know it can't be assigned? RepaintSelection doesn't need the prescontext passed anymore so it doesn't need the shell here. The mShell check replaced the STATUS_CHECK_RETURN_MACRO(); cvs add seems to need write access. I can't seem to find an offline option.
Attachment #216597 - Attachment is obsolete: true
Attachment #216811 - Flags: review?(roc)
Attachment #216597 - Flags: review?(roc)
Attached file new nsFrameSelection.h (obsolete) (deleted) —
This is the new file.
Attachment #216601 - Attachment is obsolete: true
Attachment #216813 - Flags: review?(roc)
Attachment #216601 - Flags: review?(roc)
Up to date...
Attachment #216811 - Attachment is obsolete: true
Attachment #217782 - Flags: review?(roc)
Attachment #216811 - Flags: review?(roc)
Attached file new nsFrameSelection.h (obsolete) (deleted) —
Up to date...
Attachment #216813 - Attachment is obsolete: true
Attachment #217783 - Flags: review?(roc)
Attachment #216813 - Flags: review?(roc)
nsEventStateManager::SetContentCaretVisible + nsFrameSelection* frameSelection; should have "= nsnull;" (In reply to comment #8) > A few lines above is |rv = > selCon->GetSelection(nsISelectionController::SELECTION_NORMAL, > getter_AddRefs(sel));| > > Did you mean you know it can't be assigned? Not sure what this is referring to. > Put these 4 results into a struct and return that? > Again I don't think we really care what the result is. Just return aFrameOut. You didn't address these comments? - comboboxFrame, aStyleContext, PR_TRUE, aFrameItems); + comboboxFrame, listStyle, PR_TRUE, aFrameItems); Is this change part of this patch? nsTextInputSelectionImpl::PageMove + nsIScrollableView *scrollableView = mFrameSelection->GetScrollableView(); You might want to test scrollableView for null before proceeding
I also changed nsFrameSelection::CommonPageMove to not take a frameselection since both of the callers just used this frameselection.
Attachment #217782 - Attachment is obsolete: true
Attachment #217903 - Flags: review?(roc)
Attachment #217782 - Flags: review?(roc)
Attached file new nsFrameSelection.h (deleted) —
with CommonPageMove changes and new nsPrevNextBidiLevels struct
Attachment #217783 - Attachment is obsolete: true
Attachment #217906 - Flags: review?(roc)
Attachment #217783 - Flags: review?(roc)
okay, but could you merge to trunk so I can land this? some of this has changed over the last week or so, sorry :-(
Updated.
Attachment #217903 - Attachment is obsolete: true
Attachment #219102 - Flags: review?(roc)
checked in! thanks!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Hrm. I had a completely different class renaming plan in bug 298715 (feel free to take that if you want to do it).
This appears to have created the following AIX (laredo) bustage: xlC_r -qmkshrobj=1 -o libeditor.so -bE:/home/tbox/sb/tinderbox/AIX_5.1_Clobber/mozilla/build/unix/aix.exp -bnoexpall nsEditorRegistration.o -lpthreads -bh:5 -Wl,-brtl -blibpath:/usr/lib:/lib ../html/libhtmleditor_s.a ../../txtsvc/src/libtxtsvc_s.a ../text/libtexteditor_s.a ../base/libeditorbase_s.a -L../../../dist/bin -L../../../dist/lib -lgkgfx ../../../dist/lib/libunicharutil_s.a -L../../../dist/bin -lxpcom -lxpcom_core -liconv -L../../../dist/bin -L../../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthreads -ldl -L../../../dist/bin -lmozjs -lC_r -ldl -lm -lc_r ld: 0711-317 ERROR: Undefined symbol: nsFrameSelection::QueryInterface(const nsID&,void**) ld: 0711-317 ERROR: Undefined symbol: nsFrameSelection::GetPrevNextBidiLevels(nsIContent*,unsigned int,int) ld: 0711-317 ERROR: Undefined symbol: .nsFrameSelection::~nsFrameSelection() Btw, why is the three arg GetPrevNextBidiLevels(...) virtual?
Depends on: 336617
This reverts a single line of attachment 219102 [details] [diff] [review] to fix an uninitialized variable warning gcc gives me that looks like a legitimate warning to me.
Attachment #251712 - Flags: superreview?(roc)
Attachment #251712 - Flags: review?(roc)
Attachment #251712 - Flags: superreview?(roc)
Attachment #251712 - Flags: superreview+
Attachment #251712 - Flags: review?(roc)
Attachment #251712 - Flags: review+
Comment on attachment 251712 [details] [diff] [review] revert one line to fix uninitialized variable warning Checked in to trunk.
Blocks: 78557
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: