Closed Bug 732691 Opened 8 years ago Closed 8 years ago

Further cleanups in /mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp found in bug 368758

Categories

(Core :: Editor, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: aceman, Assigned: aceman)

References

()

Details

Attachments

(2 files, 4 obsolete files)

+++ 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.
Assignee: nobody → acelists
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #602627 - Flags: review?(ehsan)
Attachment #602627 - Flags: feedback?(mcow)
if (expr) {
  stmt
}
Also if stmt is just return?
That's ugly :) So I better leave the 'if (expr) return' untouched.
Status: NEW → ASSIGNED
But thanks for the doc, there are some more style fixes I could do.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #602627 - Attachment is obsolete: true
Attachment #602640 - Flags: feedback?(Ms2ger)
Attachment #602627 - Flags: review?(ehsan)
Attachment #602627 - Flags: feedback?(mcow)
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];

{}
Attachment #602640 - Flags: feedback?(Ms2ger) → feedback+
@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.
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?
Attached patch patch v3 (obsolete) — Splinter Review
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?
Attachment #602640 - Attachment is obsolete: true
Attachment #608069 - Flags: feedback?(Ms2ger)
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.
Attachment #608069 - Flags: feedback?(Ms2ger) → feedback+
If this is ready for review, please feel free to set the review? flag to me.
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.
Nothing seems to be happening there, so go ahead.
Attached patch patch v4 (obsolete) — Splinter Review
Here it is. But comment 11 is still not resolved. Can you help me?
Attachment #608069 - Attachment is obsolete: true
Attachment #608994 - Flags: review?(ehsan)
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!
The point of the cleanup were whitespace changes too :)
I'll attach it but there is already bitrot.
Attached patch patch v5Splinter Review
Unbitrotted.
Attachment #608994 - Attachment is obsolete: true
Attachment #609481 - Flags: review?(ehsan)
Attachment #608994 - Flags: review?(ehsan)
Diff -w version.
Attachment #609485 - Flags: review?(ehsan)
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!
Attachment #609481 - Flags: review?(ehsan) → review+
Attachment #609485 - Flags: review?(ehsan)
Thanks, something like in bug 654933?
Keywords: checkin-needed
Yes!
https://hg.mozilla.org/mozilla-central/rev/2e7a23be3310
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.