398595 - PRBool misuse bugs in editor/

Status: RESOLVED WORKSFORME

(Core :: DOM: Editor, defect)

Description

[(dormant account)]

These issues were found by prcheck.

I'm not sure about
rv = mDocStateListeners[i]->NotifyDocumentStateChanged(0 != (mDocDirtyState));

I'm not sure if NotifyDocumentStateChanged receiver always expects a prbool. Signature might be wrong.

PRBool nsHTMLEditor::GetNumberOfCellsInRow(nsIDOMElement* aTable, PRInt32 rowIndex) is wrong. The signature is a prbool, it returns a cell count OR an error in the same field. I'm leaving the patch as the tool produced there.

Comment 1

[(dormant account)]

Attachment: This isn't meant to be applied. See comments in the bug

Comment 2

[(dormant account)]

Attachment: This isn't meant to be applied. See comments in the bug

Changed code to use !! instead of 0 != to be consistent with other modules.

Comment 3

[(dormant account)]

Daniel, 
Could you review this please?

Comment 4

[(dormant account)]

Comment on attachment 283785 [details] [diff] [review]
This isn't meant to be applied. See comments in the bug

roc, can you shed some light on the concerns mentioned int the bug?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #0)
> These issues were found by prcheck.
> 
> I'm not sure about
> rv = mDocStateListeners[i]->NotifyDocumentStateChanged(0 != (mDocDirtyState));
> 
> I'm not sure if NotifyDocumentStateChanged receiver always expects a prbool.
> Signature might be wrong.

It does, your change is correct.

> PRBool nsHTMLEditor::GetNumberOfCellsInRow(nsIDOMElement* aTable, PRInt32
> rowIndex) is wrong. The signature is a prbool, it returns a cell count OR an
> error in the same field. I'm leaving the patch as the tool produced there.

I'd change the result to PRInt32 and return -1 on failure.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 283785 [details] [diff] [review]
This isn't meant to be applied. See comments in the bug

Patch needs to be updated, see comment #5

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 283785 [details] [diff] [review]
This isn't meant to be applied. See comments in the bug

Clearing review since these issues have been resolved through other bugs.

Comment 8

[Michael Wu [:mwu]]

Issues resolved by bug 671417 and bug 671185 .