209548 - code cleanup in mozilla/editor

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Kathleen :Brade]

Comment 1

[Kathleen :Brade]

Attachment: patch 1 (-u8)

Comment 2

[Kathleen :Brade]

Attachment: patch 1 (-w -u8)

Comment 3

[Joe Francis]

joe chugs through review...

First comment is for this part of patch:
@@ -849,22 +821,19 @@
       PRInt32 rangeCount;
       rv = selection->GetRangeCount(&rangeCount);
       if (NS_FAILED(rv)) return PR_FALSE;
 
       for (PRInt32 i = 0; i < rangeCount; i++)
       {
         nsCOMPtr<nsIDOMRange> range;
         rv = selection->GetRangeAt(i, getter_AddRefs(range));
-        if (NS_FAILED(rv) || !range) 
-        continue; //dont bail yet, iterate through them all
-
         nsCOMPtr<nsIDOMNSRange> nsrange(do_QueryInterface(range));
         if (NS_FAILED(rv) || !nsrange) 
-        continue; //dont bail yet, iterate through them all
+          continue; //don't bail yet, iterate through them all
 
         PRBool inRange = PR_TRUE;
         (void)nsrange->IsPointInRange(parent, offset, &inRange);
         if (inRange)
           return PR_FALSE;  //okay, now you can bail, we are over the orginal
selection
       }
     }
   }


I think you need to check for an error at least from GetRangeAt.  You don't
check for error, and yuo don't initialize range, and then you try to qi it.  If
it's junk, you will bomb.

Comment 4

[Kathleen :Brade]

jfrancis--I need your review on the patch in this bug.

Comment 5

[Kathleen :Brade]

Attachment: changes for libeditor/base (independent of patch 1)

Comment 6

[Kathleen :Brade]

Attachment: changes for libeditor/base (-w)

Comment 7

[Kathleen :Brade]

Attachment: changes v2 for libeditor/base

Comment 8

[Kathleen :Brade]

Attachment: changes v2 for libeditor/base (-w)

Comment 9

[Joe Francis]

In place where you make this kind of change:
-  rv = nsComponentManager::CreateInstance(kCTransferableCID, nsnull, 
+  rv = nsComponentManager::CreateInstance("@mozilla.org/widget/transferable;1",
nsnull, 
go ahead and make the switch to do_CreateInstance().

Comment 10

[Joe Francis]

-  PRInt32 nodeCount,j;
[...]
-  nodeCount = arrayOfNodes.Count();
+  PRInt32 nodeCount = arrayOfNodes.Count();
+  PRInt32 j;

Do we really have to do this?  I don't mind moving the declarations so much, but
splitting a loop counter and the total count onto two seperate lines for
declarations seems like an unneeded addition of lines.  

Comment 11

[Joe Francis]

Comment on attachment 125846 [details] [diff] [review]
patch 1 (-u8)

r=jfrancis modulo my 3 comments in bug report.

Comment 12

[Joe Francis]

the 3 comments are #3, #9, and #10

Comment 13

[Joe Francis]

nm about coment 3, i didn't notice it was a comptr and hence inited to null.

Comment 14

[Kathleen :Brade]

Attachment: cleanup of NS_IMPL macros v1

Comment 15

[kinmoz]

Comment on attachment 126488 [details] [diff] [review]
cleanup of NS_IMPL macros v1

==== Why not leave this all on one line like the rest of  your changes?


+NS_IMPL_ISUPPORTS2(EditTxn,
+		   nsITransaction,
+		   nsPIEditorTransaction)



==== If you feel like it, in nsEditorTxnLog.cpp, nsTSDNotifier.cpp, and
nsTextServicesDocument.cpp, you can get rid of the debug refcount code so you
can actually use the NS_IMPL_ISUPPORTS1 macros instead.

sr=[e-mail]

Comment 16

[Kathleen :Brade]

Comment on attachment 126488 [details] [diff] [review]
cleanup of NS_IMPL macros v1

obsoleting patch; this has landed

Comment 17

[kinmoz]

Comment on attachment 126376 [details] [diff] [review]
changes v2 for libeditor/base

sr=[e-mail] just address these issues:


==== You moved the declaration of |multiple|, but you didn't move the comment
that goes with it:


   // does this property accept more than 1 value ?
   // we need to know that because of bug 62682
-  PRBool multiple = AcceptsMoreThanOneValue(mProperty);
-  
-  nsAutoString styleAttr(NS_LITERAL_STRING("style"));
-
+  NS_NAMED_LITERAL_STRING(styleAttr, "style");
   result = mElement->HasAttribute(styleAttr, &mUndoAttributeWasSet);
   if (NS_FAILED(result)) return result;

   nsAutoString values;
   result = cssDecl->GetPropertyValue(propertyNameString, values);
   if (NS_FAILED(result)) return result;     
   mUndoValue.Assign(values);

+  PRBool multiple = AcceptsMoreThanOneValue(mProperty);
+  



==== Can you fix the indentation of the |if| and |else| here too?


-  nsresult result = NS_OK;
+  nsresult result;
   if (mReplaceLength==0) {
       result = mElement->InsertData(mOffset,mStringToInsert);
   } else {
       result = mElement->ReplaceData(mOffset,mReplaceLength,mStringToInsert);
   }
   if (NS_SUCCEEDED(result)) {
     result = CollapseTextSelection();
   }


==== Since you're  touching this stuff, what about switching to COMPtr and
using 

do_QueryInterface()?


   IMETextTxn*	otherTxn = nsnull;
-  result =
aTransaction->QueryInterface(IMETextTxn::GetCID(),(void**)&otherTxn);
+  nsresult result =
aTransaction->QueryInterface(IMETextTxn::GetCID(),(void**)&otherTxn);
   if (otherTxn && NS_SUCCEEDED(result))


==== Do we really need to assign into |length| or can we just pass in the
result of 

mStringToInsert.Length()?


-  nsresult result;
   PRUint32 length = mStringToInsert.Length();
-  result = mElement->DeleteData(mOffset, length);
-  return result;
+  return mElement->DeleteData(mOffset, length);



==== Hmmm, the original code here looks like a bug. We should be basing the
value of 

|*aDocumentIsEmpty| on the value of |firstChild|, not just |res|. Also note
that GetFirstChild() can 

return null for |firstChild| and NS_OK.


   res = rootNode->GetFirstChild(getter_AddRefs(firstChild));

-  if(NS_SUCCEEDED(res))
-      *aDocumentIsEmpty = PR_TRUE;
-  else
-      *aDocumentIsEmpty = PR_FALSE;
-
+  *aDocumentIsEmpty = NS_SUCCEEDED(res);
   return res;
 }



==== In GetRootElement() there shouldn't be a need to do a QI to
|resultElement| and then do another 

QI to |mBodyElement| ... you should just be able to QI directly off of
|bodyElement|.


==== In GetRootElement() I see you are switching from  using a DOMDoc to  an
HTMLDOMDoc, I'm just 

concerned that could prevent us from supporting XML docs ... though I see the
old code looks for a 

"body" tag in the DOMDoc. Perhaps  we should add a comment or something that
says if  the QI to 

domDoc fails, we assume the root element is  the  root of the doc tree?


==== I don't think .get() is necessary anymore. I see this in a couple of
places:

-  if (node.get() != aRoot) *outFirstNode = node;
+  if (node.get() != aRoot)



==== You're not going to return the real error if GetNewTransaction() fails?


-  nsresult result=NS_ERROR_NULL_POINTER;
-  if (nsnull != aNode)
+  if (aNode)
   {
-    result = TransactionFactory::GetNewTransaction(SplitElementTxn::GetCID(),
(EditTxn **)aTxn);
+    nsresult result =
TransactionFactory::GetNewTransaction(SplitElementTxn::GetCID(), (EditTxn 

**)aTxn);
     if (NS_SUCCEEDED(result))	{
       result = (*aTxn)->Init(this, aNode, aOffset);
     }
   }
-  return result;
+  return NS_ERROR_NULL_POINTER;


==== I don't think this is a copy paste error, there are a couple of places in
the code below what 

you changed that will only be entered if |selStartNode| is not null, and that
code operates on both 

|selStartNode| and |selEndNode|. I would undo this change.


     result = GetStartNodeAndOffset(selection, address_of(selStartNode),
&selStartOffset);
     if (NS_FAILED(result)) selStartNode = nsnull;
     result = GetEndNodeAndOffset(selection, address_of(selEndNode),
&selEndOffset);
-    if (NS_FAILED(result)) selStartNode = nsnull;
+    if (NS_FAILED(result)) selEndNode = nsnull; // Joe or Kin: is this
copy/paste error?



==== I probably would've combined these into one |if| expression with ||:


+  if (!*aResultNode) return NS_OK;
+  if (!aEditableNode) return NS_OK;
+  if (IsEditable(*aResultNode)) return NS_OK;
+


==== Same here:


+    if (!aEditableNode) return NS_OK;
+    if (IsEditable(*aResultNode)) return NS_OK;


==== We shouldn't need .get():


+  return (atom1.get() == atom2.get());


==== In nsPasteCommand::DoCommand() we used to	return NS_OK if nothing was
done, but your changes 

make it an error. I'm assuming that was intentional right?


==== I would put the comment before the assignment:

+  *outCmdEnabled = (editor != nsnull);
+  // you can select all if there is an editor (and potentially no contents)

   return NS_OK;


==== Most of the IsCommandEnabled() methods never check for null out params
before setting them.


==== Also, why is it an error if the editor doesn't exist in most of the
IsCommandEnabled() 

implementations, but in some it isn't an error? 
nsInsertPlaintextCommand::IsCommandEnabled() and 

nsPasteQuotationCommand::IsCommandEnabled()  are 2 examples that don't throw an
error.


==== Also some DoCommand() implementations return NS_ERROR_FAILURE when nothing
happens, and others 

return NS_ERROR_NOT_IMPLEMENTED. Doesn't have to be addressed as part of this
patch, but I thought I'd bring it up.


==== I would init |enabled| especially since we aren't checking the return
value of |CanPaste()|  

note that only the text and html versions of CanPaste() init |enabled|, the
base version  just 

throws an error without init'ing.



 NS_IMETHODIMP
 nsPasteQuotationCommand::GetCommandStateParams(const char *aCommandName,
						nsICommandParams *aParams,
						nsISupports *refCon)
 {
   nsCOMPtr<nsIEditor> editor = do_QueryInterface(refCon);
-  PRBool enabled = PR_FALSE;
   if (editor)
   {
+    PRBool enabled;
     editor->CanPaste(nsIClipboard::kGlobalClipboard, &enabled);
     aParams->SetBooleanValue(STATE_ENABLED, enabled);
   }

   return NS_OK;
 }


==== In nsSelectionState::SaveSelection() we never check |res| within the |for|
loop, we just return 

the last value it had during the last iteration. That seems bit hokey to me.

Comment 18

[Kathleen :Brade]

Attachment: remove CID usage in editor (except in 2 cases)

obsoleting patches that have already landed

Comment 19

[kinmoz]

Comment on attachment 126943 [details] [diff] [review]
remove CID usage in editor (except in 2 cases)

sr=[e-mail], just fix the following:

==== In nsPlaintextDataTransfer.cpp did you mean to use the contract id for the
transferable instead of the clipboard?


-  nsresult rv = nsComponentManager::CreateInstance(kCTransferableCID, nsnull, 
+  nsresult rv =
nsComponentManager::CreateInstance("@mozilla.org/widget/clipboard;1", nsnull, 
					   NS_GET_IID(nsITransferable), 
					   (void**)transferable);



==== Same here:


-  nsCOMPtr<nsITransferable> trans = do_CreateInstance(kCTransferableCID, &rv);
+  nsCOMPtr<nsITransferable> trans =
do_CreateInstance("@mozilla.org/widget/clipboard;1", &rv);

Comment 20

[Kathleen :Brade]

Attachment: cleanup NodeIsType; reduce NS_LITERAL_STRINGS; v1

obsolete attachment 126943 [details] [diff] [review] since it has already landed

Comment 21

[Kai Engert (:KaiE:)]

Comment on attachment 127365 [details] [diff] [review]
cleanup NodeIsType; reduce NS_LITERAL_STRINGS; v1

r=kaie

However, please consider the following change, the r=kaie still applies if you
do:

+nsEditor::NodeIsType(nsIDOMNode *aNode, const char *aTagStr)
+{
+  if (aNode)

What about using
   if (aNode && aTagStr)
?

I'm not sure if the string comparison function will do a null check on its own.

Comment 22

[Darin Fisher]

Comment on attachment 127365 [details] [diff] [review]
cleanup NodeIsType; reduce NS_LITERAL_STRINGS; v1

>Index: libeditor/base/nsEditor.cpp

>+nsEditor::NodeIsType(nsIDOMNode *aNode, const char *aTagStr)
>+{
>+  if (aNode)
>+  {
>+    nsAutoString tag;
>+    nsEditor::GetTagString(aNode, tag);
>+    if (tag.EqualsIgnoreCase(aTagStr))
>+      return PR_TRUE;

hmm... are all tags ASCII?  if so, then how about doing this
instead:

    nsCOMPtr<nsIAtom> atom = nsEditor::GetTag(aNode);
    if (atom)
    {
      const char *atomVal;
      if (NS_SUCCEEDED(atom->getUTF8String(&atomVal)) &&
	  nsCRT::strcasecmp(atomVal, aTagStr) == 0)
      {
	return PR_TRUE;
      }
    }
    return PR_FALSE;

this avoids a string copy and string length computation.

if all tags are not ASCII, then you have a bug because
nsString::EqualsIgnoreCase(const char *rhs) assumes |rhs|
is ASCII.


>Index: libeditor/html/nsHTMLCSSUtils.cpp

>+  if (nsEditor::NodeIsType(aNode, "span")) {

you know some compilers/linkers don't coalesce multiple instances
of the same string literal.  so it might make sense to declare these
string constants someplace in libeditor.

actually, i don't understand why you don't want to just reference
the static atoms directly.  then you could just use GetTag and do
a straight pointer comparison, right?

Comment 23

[Kathleen :Brade]

Attachment: cleanup NodeIsType; use atoms; v2

Darin is right; we should dump the strings altogether and just use atoms where
we can.  This patch does that and renames the NodeIsType string version so that
it can more easily be removed at a future time.

Comment 24

[Darin Fisher]

Comment on attachment 127525 [details] [diff] [review]
cleanup NodeIsType; use atoms; v2

>Index: libeditor/base/nsEditor.cpp

> PRBool 
> nsEditor::NodeIsType(nsIDOMNode *aNode, nsIAtom *aTag)
> {
>+  if (!aTag) return PR_FALSE;
>+
>+  nsCOMPtr<nsIAtom> nodeAtom = nsEditor::GetTag(aNode);
>+  return (nodeAtom == aTag);
> }

oooh... i just thought of something nasty.  since content registers
its own set of atoms, don't some of those atoms overlap with this
set of atoms?  and if so, then if any of the atoms (like "b") are 
already registered, then won't your "b" not get registered?  iow,
could it be that GetTag returns a pointer to an atom for "b" that
is not equal to nsEditProperty::b?  doesn't that horribly screw up
this comparison? :-(

would be good to consult alecf or dbaron on this.

Comment 25

[Darin Fisher]

alecf, dbaron: can you guys comment on the above.  thx!

Comment 26

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

I don't see the problem.  Initialization of all those static atom variables is
done after a lookup in the atom table.

Comment 27

[kinmoz]

Comment on attachment 127525 [details] [diff] [review]
cleanup NodeIsType; use atoms; v2

r=[e-mail]

==== Since you're doing a bunch of NodeIsTYpe() calls already, I probably
wouldn't have bothered 

changing the checks for "big" and "small" to the Is*() calls since it just adds
an extra function call that does the same thing, but it's up to you. Since this
same list appears in a couple of places, what we probably need is an
IsHTMLInlineStyle() utility method, but that's not required for this checkin.



-    if (nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("b"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("i"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("u"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("tt"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("s"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("strike")) ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("big"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("small"))  ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("blink"))  ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("sub"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("sup"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("font"))	 ||
-	 nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("span"))) {
+    if (nsEditor::NodeIsType(child, nsEditProperty::b)      ||
+	 nsEditor::NodeIsType(child, nsEditProperty::i)      ||
+	 nsEditor::NodeIsType(child, nsEditProperty::u)      ||
+	 nsEditor::NodeIsType(child, nsEditProperty::tt)     ||
+	 nsEditor::NodeIsType(child, nsEditProperty::s)      ||
+	 nsEditor::NodeIsType(child, nsEditProperty::strike) ||
+	 nsHTMLEditUtils::IsBig(child)			     ||
+	 nsHTMLEditUtils::IsSmall(child)		     ||
+	 nsEditor::NodeIsType(child, nsEditProperty::blink)  ||
+	 nsEditor::NodeIsType(child, nsEditProperty::sub)    ||
+	 nsEditor::NodeIsType(child, nsEditProperty::sup)    ||
+	 nsEditor::NodeIsType(child, nsEditProperty::font)   ||
+	 nsEditor::NodeIsType(child, nsEditProperty::span)) {



==== I'd make your comment an XXX: comment so it stands out, and I'd probably
move it above the  

|if| statement, but it's up to you.


-	   if (nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("body")) ||
-	       nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("td")) ||
-	       nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("th")) ||
-	       nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("caption"))
||
-	       nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("tr")) ||
-	       nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("table")))
+	   if (nsEditor::NodeIsType(node, nsEditProperty::body) ||
+// brade: should we call nsHTMLEditUtils::IsTableElement here?
+// that also checks for thead, tbody, tfoot
+	       nsEditor::NodeIsType(node, nsEditProperty::td) ||
+	       nsEditor::NodeIsType(node, nsEditProperty::th) ||
+	       nsEditor::NodeIsType(node, nsEditProperty::caption) ||
+	       nsEditor::NodeIsType(node, nsEditProperty::tr) ||
+	       nsEditor::NodeIsType(node, nsEditProperty::table))


==== I tend to dislike performing the same checks more than once, but I see
that there was some pre-existing code that did these same types of checks:


-  if (sibling && NodeIsType(sibling, nodeType))
+  if (sibling && NodeIsType(sibling, (aSizeChange==1) ? nsEditProperty::big :
nsEditProperty::small))

Comment 28

[Darin Fisher]

dbaron: thanks for clearing up my confusion.

Comment 29

[Darin Fisher]

Comment on attachment 127525 [details] [diff] [review]
cleanup NodeIsType; use atoms; v2

>Index: libeditor/base/nsEditor.cpp

> PRBool 
> nsEditor::NodeIsType(nsIDOMNode *aNode, nsIAtom *aTag)
> {
>+  if (!aTag) return PR_FALSE;
>+
>+  nsCOMPtr<nsIAtom> nodeAtom = nsEditor::GetTag(aNode);
>+  return (nodeAtom == aTag);
> }

is |aTag == NULL| really a common case?  seems like you could
do away with that branch altogether and get the same result.
also would it make sense for NodeIsType to be defined in the
header file so it could possibly be inlined by the compiler?


> PRBool 
>+nsEditor::NodeIsTypeString(nsIDOMNode *aNode, const nsAString &aTagStr)
> {
>   nsCOMPtr<nsIDOMElement>element = do_QueryInterface(aNode);
>   if (element)
>   {
>+    nsAutoString tag;
>     element->GetTagName(tag);
>+    if (tag.Equals(aTagStr, nsCaseInsensitiveStringComparator()))
>       return PR_TRUE;
>   }
>   return PR_FALSE;
> }

nit: if you use GetTag again to retrieve the nsIAtom, then you can
call nsIAtom::Equals to avoid having to copy out the string value
of the atom.

Comment 30

[Kathleen :Brade]

Attachment: cleanup NodeIsType; use atom; v3 [CHECKED IN]

Comment 31

[Darin Fisher]

Comment on attachment 127754 [details] [diff] [review]
cleanup NodeIsType; use atom; v3 [CHECKED IN]

sr=darin

Comment 32

[kinmoz]

Comment on attachment 127754 [details] [diff] [review]
cleanup NodeIsType; use atom; v3 [CHECKED IN]

r=[e-mail]

Comment 33

[Kathleen :Brade]

Comment on attachment 127754 [details] [diff] [review]
cleanup NodeIsType; use atom; v3 [CHECKED IN]

obsoleting patch; it was just checked in

Comment 34

[tor]

This broke --enable-plaintext-editor-only builds, as nsEditProperty
exists in the html libeditor subdir and is only built for the full
editor.  nsEditProperty::body and nsEditProperty::br are refered
to from nsTextEditUtils.cpp.

Comment 35

[tor]

Attachment: patch from brade to fix plaintext-only build

Comment 36

[Brian Ryner (not reading)]

Comment on attachment 127908 [details] [diff] [review]
patch from brade to fix plaintext-only build

r/sr=bryner

Comment 37

[Brian Ryner (not reading)]

checked in fix for --enable-plaintext-editor-only bustage.

Comment 38

[Kathleen :Brade]

Attachment: easy cleanup/optimizations in nsHTMLEditRules.cpp

Comment 39

[Charles Manske]

Comment on attachment 127969 [details] [diff] [review]
easy cleanup/optimizations in nsHTMLEditRules.cpp

hurray! reduce code size!

Comment 40

[Kathleen :Brade]

Attachment: Truncate() instead of SetLength(0) [CHECKED IN]

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127982 [details] [diff] [review]
Truncate() instead of SetLength(0) [CHECKED IN]

sr=bzbarsky

Comment 42

[Kathleen :Brade]

Attachment: more recent version of "patch 1" (1st part) which already has r=jfrancis [CHECKED IN]

Comment 43

[Christopher Blizzard (:blizzard)]

Comment on attachment 128011 [details] [diff] [review]
more recent version of "patch 1" (1st part) which already has r=jfrancis [CHECKED IN]

sr=blizzard

Comment 44

[Kathleen :Brade]

Attachment: current leftovers from initial "patch 1"

Comment 45

[Kathleen :Brade]

Comment on attachment 128013 [details] [diff] [review]
current leftovers from initial "patch 1"

already has r=jfrancis; seeking sr=

Comment 46

[Kathleen :Brade]

resolving this bug as fixed
There is more cleanup to do but it will happen in a new bug since this one
already tracks so many changes.

Comment 47

[Aleksey Nogin]

This commit have added a new "might be used uninitialized" warning to brad TBox:

+editor/libeditor/text/nsPlaintextEditor.cpp:1905
+ `nsresult result' might be used uninitialized in this function

Indeed, if aNode is non-null, but nodeAsContent is null (e.g. do_QueryInterface
failed), nsPlaintextEditor::GetLayoutObject will return an uninitialized nsresult:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/text/nsPlaintextEditor.cpp&rev=1.73&cvsroot=/cvsroot&mark=1905-1909,1915,1919-1920#1900