330305 - DeCOMtaminate nsIFrameSelection.h

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Marc Liddell]

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:

Comment 1

[Marc Liddell]

Attachment: Change all methods and fix callers

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.

Comment 2

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

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.

Comment 3

[Marc Liddell]

Attachment: Change all uses of nsIFrameSelection to nsSelection

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.

Comment 4

[Marc Liddell]

Attachment: new nsSelection.h file

This is the new file. Probably needs review? too.

Comment 5

[Marc Liddell]

Attachment: new nsSelection.h file

I forgot to transfer the comments from nsIFrameSelection.

Comment 6

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

You can do "cvs add layout/base/nsSelection.h" and then "cvs diff -N" to get the new file in your diff.

Comment 7

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

  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;

Comment 8

[Marc Liddell]

Attachment: Change all uses of nsIFrameSelection/nsSelection to nsFrameSelection

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.

Comment 9

[Marc Liddell]

Attachment: new nsFrameSelection.h

This is the new file.

Comment 10

[Marc Liddell]

Attachment: Change all uses of nsIFrameSelection/nsSelection to nsFrameSelection

Up to date...

Comment 11

[Marc Liddell]

Attachment: new nsFrameSelection.h

Up to date...

Comment 12

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

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

Comment 13

[Marc Liddell]

Attachment: Change all uses of nsIFrameSelection/nsSelection to nsFrameSelection

I also changed nsFrameSelection::CommonPageMove to not take a frameselection since both of the callers just used this frameselection.

Comment 14

[Marc Liddell]

Attachment: new nsFrameSelection.h

with CommonPageMove changes and new nsPrevNextBidiLevels struct

Comment 15

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

okay, but could you merge to trunk so I can land this? some of this has changed over the last week or so, sorry :-(

Comment 16

[Marc Liddell]

Attachment: Change all uses of nsIFrameSelection/nsSelection to nsFrameSelection

Updated.

Comment 17

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

checked in! thanks!

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm.  I had a completely different class renaming plan in bug 298715 (feel free to take that if you want to do it).

Comment 19

[jag (Peter Annema)]

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?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: revert one line to fix uninitialized variable warning

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 251712 [details] [diff] [review]
revert one line to fix uninitialized variable warning

Checked in to trunk.