The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
Editor
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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)

Updated

5 years ago
Assignee: nobody → acelists
(Assignee)

Comment 1

5 years ago
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.
Attachment #602627 - Flags: review?(ehsan)
Attachment #602627 - Flags: feedback?(mcow)
if (expr) {
  stmt
}
(Assignee)

Comment 3

5 years ago
Also if stmt is just return?
Yes. See <https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_structures>.
(Assignee)

Comment 5

5 years ago
That's ugly :) So I better leave the 'if (expr) return' untouched.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

5 years ago
But thanks for the doc, there are some more style fixes I could do.
(Assignee)

Comment 7

5 years ago
Created attachment 602640 [details] [diff] [review]
patch v2
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+

Comment 9

5 years ago
@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.
(Assignee)

Comment 10

5 years ago
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?
(Assignee)

Comment 11

5 years ago
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?
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.
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Created attachment 608994 [details] [diff] [review]
patch v4

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!
(Assignee)

Comment 18

5 years ago
The point of the cleanup were whitespace changes too :)
I'll attach it but there is already bitrot.
(Assignee)

Comment 19

5 years ago
Created attachment 609481 [details] [diff] [review]
patch v5

Unbitrotted.
Attachment #608994 - Attachment is obsolete: true
Attachment #609481 - Flags: review?(ehsan)
Attachment #608994 - Flags: review?(ehsan)
(Assignee)

Comment 20

5 years ago
Created attachment 609485 [details] [diff] [review]
patch v5 - diff -w

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+

Updated

5 years ago
Attachment #609485 - Flags: review?(ehsan)
(Assignee)

Comment 22

5 years ago
Thanks, something like in bug 654933?
Keywords: checkin-needed
Yes!
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7a23be3310
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/2e7a23be3310
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.