Last Comment Bug 732691 - Further cleanups in /mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp found in bug 368758
: Further cleanups in /mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp fou...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla14
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 368758
Blocks: 650787
  Show dependency treegraph
 
Reported: 2012-03-03 04:46 PST by :aceman
Modified: 2012-03-27 05:24 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (34.49 KB, patch)
2012-03-03 08:57 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (76.13 KB, patch)
2012-03-03 11:43 PST, :aceman
Ms2ger: feedback+
Details | Diff | Splinter Review
patch v3 (77.19 KB, patch)
2012-03-21 13:12 PDT, :aceman
Ms2ger: feedback+
Details | Diff | Splinter Review
patch v4 (77.39 KB, patch)
2012-03-24 07:40 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v5 (77.18 KB, patch)
2012-03-26 14:13 PDT, :aceman
ehsan: review+
Details | Diff | Splinter Review
patch v5 - diff -w (56.57 KB, patch)
2012-03-26 14:18 PDT, :aceman
no flags Details | Diff | Splinter Review

Description :aceman 2012-03-03 04:46:59 PST
+++ This bug was initially created as a clone of Bug #368758 +++

Further cleanups in /mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp found in bug 368758 comment 22, bug 368758 comment 18.
Comment 1 :aceman 2012-03-03 08:57:31 PST
Created attachment 602627 [details] [diff] [review]
patch

There are mostly whitespace changes and renaming of the preNode variable.
But also some early returns to save some indentation levels. Those caused some serious changes in nsHTMLEditor::Paste, but I hope I didn't change the behaviour. Please check.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:58:57 PST
if (expr) {
  stmt
}
Comment 3 :aceman 2012-03-03 09:06:46 PST
Also if stmt is just return?
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 09:36:59 PST
Yes. See <https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_structures>.
Comment 5 :aceman 2012-03-03 10:31:40 PST
That's ugly :) So I better leave the 'if (expr) return' untouched.
Comment 6 :aceman 2012-03-03 10:52:59 PST
But thanks for the doc, there are some more style fixes I could do.
Comment 7 :aceman 2012-03-03 11:43:45 PST
Created attachment 602640 [details] [diff] [review]
patch v2
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 12:02:39 PST
Comment on attachment 602640 [details] [diff] [review]
patch v2

Review of attachment 602640 [details] [diff] [review]:
-----------------------------------------------------------------

f=me once it builds

::: editor/libeditor/html/nsHTMLDataTransfer.cpp
@@ +314,5 @@
>    {
>      // if caller didn't provide the destination/target node,
>      // fetch the paste insertion point from our selection
> +    rv = GetStartNodeAndOffset(selection, getter_AddRefs(targetNode), &targetOffset);
> +    if (!targetNode) rv = NS_ERROR_FAILURE;

Might as well add {}s here

@@ +413,5 @@
>      // delete whole cells: we will replace with new table content
> +    // Save current selection since DeleteTableCell perturbs it
> +    nsAutoSelectionReset selectionResetter(selection, this);
> +    rv = DeleteTableCell(1);
> +    NS_ENSURE_SUCCESS(rv, rv);

You need to keep the nsAutoSelectionReset in a scope, so just remove the 'if (1)' line and leave the {}s

@@ +432,5 @@
>    {
>      // The rules code (WillDoAction above) might have changed the selection.  
>      // refresh our memory...
> +    rv = GetStartNodeAndOffset(selection, getter_AddRefs(parentNode), &offsetOfNewNode);
> +    if (!parentNode) rv = NS_ERROR_FAILURE;

As before

@@ +983,5 @@
>      if (parent)
>      {
>        if (!aListOnly || nsHTMLEditUtils::IsList(parent))
> +        rv = parent->RemoveChild(aNode, getter_AddRefs(ignored));
> +      return rv;

I'd change this to

if (!aListOnly || nsHTMLEditUtils::IsList(parent)) {
  return parent->RemoveChild(aNode, getter_AddRefs(ignored));
}
return NS_OK;

and move the declaration of rv down.

@@ +1002,4 @@
>        child = tmp;
>      }
>    }
> +  return rv;

This should just return NS_OK

@@ +1123,5 @@
>    //   http://msdn.microsoft.com/en-us/library/aa767917(VS.85).aspx#unknown_854
>    if (startHTML == -1) {
>      startHTML = aCfhtml.Find("<!--StartFragment-->");
>      if (startHTML == -1)
> +      return NS_OK;

Nice.

@@ +1549,5 @@
> +  // Get the Data from the clipboard
> +  rv = clipboard->GetData(trans, aSelectionType);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!IsModifiable()) {
> +    retrun NS_OK;

Ahem

@@ +1790,4 @@
>    {
> +    printf("Couldn't collapse");
> +    // XXX: error result:  should rv be returned here?
> +  }

Just NS_ENSURE_SUCCESS here.

@@ +1823,5 @@
> +  nsCOMPtr<nsISupports> genericDataObj;
> +  PRUint32 len = 0;
> +  char* flav = 0;
> +  rv = trans->GetAnyTransferData(&flav, getter_AddRefs(genericDataObj),
> +                                   &len);

Indentation

@@ +1835,2 @@
>  
> +  if (flav && 0 == nsCRT::strcmp((flav), kUnicodeMime))

Remove the parens around flav too

@@ +1997,4 @@
>    {
> +    // Add an attribute on the pre node so we'll know it's a quotation.
> +    // Do this after the insertion, so that
> +    nsCOMPtr<nsIDOMElement> preElement = do_QueryInterface(newNode);

You want to change this name too

@@ +2068,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
> +
> +  nsAutoEditBatch beginBatching(this);
> +  nsAutoRules beginRulesSniffing(this, kOpInsertQuotation, nsIEditor::eNext);

These may need scoping too

@@ +2262,5 @@
>      RemoveBodyAndHead(contextAsNode);
>  
> +    rv = FindTargetNode(contextAsNode, contextLeaf);
> +    if (rv == NS_FOUND_TARGET)
> +      rv = NS_OK;

{}

@@ +2335,5 @@
>      }
>    }
>  
>    GetLengthOfDOMNode(*outEndNode, (PRUint32&)*outEndOffset);
> +  return rv;

NS_OK

@@ +2355,5 @@
>                                           aContextLocalName ?
>                                             aContextLocalName : nsGkAtoms::body,
>                                          kNameSpaceID_XHTML,
>                                          false,
>                                          true);

Indentation

@@ +2577,1 @@
>      return nsnull;

{}

@@ +2580,1 @@
>      return aNodeArray[listCount-1];

{}
Comment 9 Joe Sabash [:JoeS1] 2012-03-03 12:04:31 PST
@aceman
I know this is only cleanup, but you _might_ be touching code that is regressing paste image see bug 732585
Just might not be the best time to do this in case there are new regressions.
Comment 10 :aceman 2012-03-03 12:25:09 PST
Thanks for great catches.

(In reply to Ms2ger from comment #8)
> @@ +2068,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
> > +
> > +  nsAutoEditBatch beginBatching(this);
> > +  nsAutoRules beginRulesSniffing(this, kOpInsertQuotation, nsIEditor::eNext);
> 
> These may need scoping too

Why and how to decide this?
Comment 11 :aceman 2012-03-21 13:12:58 PDT
Created attachment 608069 [details] [diff] [review]
patch v3

Fixes done. Can you help me with those nsAutoEditBatch and nsAutoSelectionReset scopes? Should I just keep the blocks as they were? Or can they be optimized somehow?
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-03-21 13:55:38 PDT
Comment on attachment 608069 [details] [diff] [review]
patch v3

Review of attachment 608069 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good enough; a few more nits below

::: editor/libeditor/html/nsHTMLDataTransfer.cpp
@@ +377,2 @@
>  
> +  // are there any table elements in the list?

are -> Are

@@ +455,3 @@
>      }
> +
> +    // remeber if we are in a link.

Remember

@@ +608,2 @@
>        }
> +      // check for pre's going into pre's.

Capitalize this one too

@@ +985,5 @@
>      aNode->GetParentNode(getter_AddRefs(parent));
>      if (parent)
>      {
>        if (!aListOnly || nsHTMLEditUtils::IsList(parent))
> +        return parent->RemoveChild(aNode, getter_AddRefs(ignored));

{}

@@ +1794,4 @@
>    {
> +    printf("Couldn't collapse");
> +  }
> +#endif

I don't think Akkana will find this terribly useful if she ever comes back. Just remove it?

@@ +1834,5 @@
> +#ifdef DEBUG_akkana
> +    printf("PasteAsPlaintextQuotation: GetAnyTransferData failed, %d\n", rv);
> +#endif
> +    return rv;
> +  }

Same here; NS_ENSURE_SUCCESS will do

@@ +2401,5 @@
>    nsRefPtr<nsRange> docFragRange = new nsRange();
> +  rv = docFragRange->SetStart(aStartNode, aStartOffset);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = docFragRange->SetEnd(aEndNode, aEndOffset);
> +  NS_ENSURE_SUCCESS(rv, rv);

There's nsRange::CreateRange() you could use

@@ +2581,5 @@
>                                             nsCOMArray<nsIDOMNode>& aNodeArray)
>  {
>    PRInt32 listCount = aNodeArray.Count();
> +  if (listCount <= 0)
> +  {

{ on the previous line if you're adding it.
Comment 13 :Ehsan Akhgari 2012-03-22 16:33:11 PDT
If this is ready for review, please feel free to set the review? flag to me.
Comment 14 :aceman 2012-03-23 01:02:10 PDT
I'll address the last comment of ms2ger and give it to you for review soon. I just don't know if/how long to wait due to comment 9.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-03-23 01:17:48 PDT
Nothing seems to be happening there, so go ahead.
Comment 16 :aceman 2012-03-24 07:40:41 PDT
Created attachment 608994 [details] [diff] [review]
patch v4

Here it is. But comment 11 is still not resolved. Can you help me?
Comment 17 :Ehsan Akhgari 2012-03-26 13:40:54 PDT
Comment on attachment 608994 [details] [diff] [review]
patch v4

Review of attachment 608994 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please submit a version of this patch generated with hg diff -w as well?  This is very hard to review because of whitespace changes.  Thanks!
Comment 18 :aceman 2012-03-26 13:48:36 PDT
The point of the cleanup were whitespace changes too :)
I'll attach it but there is already bitrot.
Comment 19 :aceman 2012-03-26 14:13:44 PDT
Created attachment 609481 [details] [diff] [review]
patch v5

Unbitrotted.
Comment 20 :aceman 2012-03-26 14:18:13 PDT
Created attachment 609485 [details] [diff] [review]
patch v5 - diff -w

Diff -w version.
Comment 21 :Ehsan Akhgari 2012-03-26 14:20:43 PDT
Comment on attachment 609481 [details] [diff] [review]
patch v5

r=me.

In the future please split this type of cleanup info multiple patches and focus on one kind of cleanup in each patch (for example: patch 1 would do s/res/rv/, patch 2 would eliminate this if block, etc.)

Thanks!
Comment 22 :aceman 2012-03-26 14:41:31 PDT
Thanks, something like in bug 654933?
Comment 23 :Ehsan Akhgari 2012-03-26 14:59:00 PDT
Yes!
Comment 25 Ed Morley [:emorley] 2012-03-27 05:24:34 PDT
https://hg.mozilla.org/mozilla-central/rev/2e7a23be3310

Note You need to log in before you can comment on or make changes to this bug.