572618 - Make debugging the editor easier

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

It's been a long time that I wanted to do this.  We can use warning messages to make debugging the editor easier, by switching to using macros like NS_ENSURE_*.  This can be done using automatic search and replacement.  It won't change any behavior, but would make reading the code easier as an added bonus.

Comment 1

[(no longer active)]

Attachment: Part 1: Replace all if(NS_FAILED) checks with NS_ENSURE_SUCCESS

This is the result of this command:

find . -type f -name \*.cpp -or -name \*.h | xargs perl -pi -e 's/if\s+\(NS_FAILED\(([^)]+)\)\)\s+return\s+([^;]+)/NS_ENSURE_SUCCESS(\1, \2)/g'

Comment 2

[(no longer active)]

Attachment: Part 2: Replace all if(!foo) return NS_ERROR_bar checks with NS_ENSURE_TRUE(foo, NS_ERROR_bar)

This is the result of this command:

find . -type f -name \*.cpp -or -name \*.h | xargs perl -pi -e 's/if\s+\(!([^)]+)\)\s+return\s+([^;]+)/NS_ENSURE_TRUE(\1, \2)/g'

with two manual tweaks in nsHTMLDataTransfer.cpp because in two places in that file, the |if| in question had an |else|.

Comment 3

[(no longer active)]

Attachment: Part 3: multiline version of part 2

This is the result of this command:

find . -type f -name \*.cpp -or -name \*.h | xargs perl -pi -e 'undef $/; s/if\s+\(!([^)]+)\)\s*\n\s*return\s+([^;]+)/NS_ENSURE_TRUE(\1, \2)/gms'

with one manual fix in nsHTMLEditor.cpp of this pattern:

  if (foo)
// if (bar)
    return NS_ERROR_baz;

Comment 4

[(no longer active)]

Attachment: Part 4: multiline version of part 1

This is the result of this command:

find . -type f -name \*.cpp -or -name \*.h | xargs perl -pi -e 'undef $/; s/if\s+\(NS_FAILED\(([^)]+)\)\)\s*\n\s*return\s+([^;]+)/NS_ENSURE_SUCCESS(\1, \2)/g'

Comment 5

[(no longer active)]

Attachment: Part 2: Replace all if(!foo) return NS_ERROR_bar checks with NS_ENSURE_TRUE(foo, NS_ERROR_bar)

Handle compound conditions correctly.

For example, handle constructs such as the following correctly:

-  if (!foo || !bar) return baz;
+  NS_ENSURE_TRUE(foo && bar, baz);

Interdiff with the previous version of part 2: http://pastebin.mozilla.org/736447

Comment 6

[(no longer active)]

I had to update these patches when I removed the patch to bug 572513 from my queue.  I'll attach the new versions.

Comment 7

[(no longer active)]

Attachment: Part 1: Replace all if(NS_FAILED) checks with NS_ENSURE_SUCCESS

Comment 8

[(no longer active)]

Attachment: Part 2: Replace all if(!foo) return NS_ERROR_bar checks with NS_ENSURE_TRUE(foo, NS_ERROR_bar)

Comment 9

[(no longer active)]

Attachment: Part 3: multiline version of part 2

Comment 10

[(no longer active)]

Attachment: Part 4: multiline version of part 1

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8d5a112a3091
http://hg.mozilla.org/mozilla-central/rev/223b2b67edbd
http://hg.mozilla.org/mozilla-central/rev/bebdf3413522
http://hg.mozilla.org/mozilla-central/rev/45dd34892448

Comment 12

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

I backed out one of the changes:
http://hg.mozilla.org/mozilla-central/rev/5332eb29d243
because it was causing huge amounts of debug spew, causing some tinderbox builds to fail due to huge log size:
WARNING: NS_ENSURE_TRUE(mRedoStack) failed: file e:/builds/moz2_slave/mozilla-central-win32-debug/build/editor/txmgr/src/nsTransactionItem.cpp, line 337

Comment 13

[Neil Deakin]

diff --git a/editor/libeditor/base/nsEditorEventListener.cpp b/editor/libeditor/base/nsEditorEventListener.cpp
...
   nsCOMPtr<nsIDOMNode> sourceNode;
   dataTransferNS->GetMozSourceNode(getter_AddRefs(sourceNode));
-  if (!sourceNode)
-    return PR_TRUE;
+  NS_ENSURE_TRUE(sourceNode, PR_TRUE);

This case isn't an error condition. Was this change desired? It causes the warning to dump out continuously from an external location while dragging over a textbox.

Comment 14

[(no longer active)]

(In reply to comment #13)
> diff --git a/editor/libeditor/base/nsEditorEventListener.cpp
> b/editor/libeditor/base/nsEditorEventListener.cpp
> ...
>    nsCOMPtr<nsIDOMNode> sourceNode;
>    dataTransferNS->GetMozSourceNode(getter_AddRefs(sourceNode));
> -  if (!sourceNode)
> -    return PR_TRUE;
> +  NS_ENSURE_TRUE(sourceNode, PR_TRUE);
> 
> This case isn't an error condition. Was this change desired? It causes the
> warning to dump out continuously from an external location while dragging over
> a textbox.

This was a bogus check.  I removed it in http://hg.mozilla.org/mozilla-central/rev/5f38f9a55345.  Thanks for catching this.