128903 - nsIEditor.idl and nsIHTMLEditor.idl methods need to return out params correctly

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Charles Manske]

During the IDLizing of editor interfaces, the IDL methods return out params 
that make it much harder to use in JavaScript:
Methods that have just one return param should be attributes: E.g.:
  void GetSelection(out nsISelection selection);
should be:
readonly attribute nsISelection selection;

Methods with in params or multiple out params should return the last out param:
  void CreateNode(in DOMString tag,
                        in nsIDOMNode parent,
                        in long         position,
                        out nsIDOMNode newNode);
should be:
  nsIDOMNode newNode CreateNode(in DOMString tag,
                        in nsIDOMNode parent,
                        in long position);

Comment 1

[Charles Manske]

In last example, it should be:
nsIDOMNode CreateNode(in DOMString tag,
                      in nsIDOMNode parent,
                      in long position);

Comment 2

[Akkana Peck]

Attachment: Changes to nsIEditor.idl and nsIHTMLEditor.idl

Comment 3

[Akkana Peck]

Patch attached.  I changed all the nsIEditor and nsIHTMLEditor methods which
returned one out parameter, except for one case, SplitNode, where it wouldn't be
obvious what the return value was without a named out param.

It would be nice to make CSSEnabled at the end of nsIHTMLEditor a read/write
parameter, but I was trying to stick to changes that wouldn't affect any code,
for safety, and making it a parameter would change the name of IsCSSEnabled().

Seeking review (ideally from both cmanske and brade).  Also cc'ing Simon and Kin
in case they have any comments (and so they know what's going on when I hit one
of them up for an sr).

Comment 4

[Charles Manske]

I would suggest converting these methods in nsIHTMLEditor.idl:

  void  GetParagraphState(out boolean aMixed,out DOMString  outState);
  void  GetFontFaceState(out boolean aMixed,out DOMString  outFont);
  void  GetFontColorState(out boolean aMixed,out DOMString  outColor);
  void  GetBackgroundColorState(out boolean aMixed,out DOMString  outColor);
  void  GetAlignment(out boolean aMixed, out short aAlign);
so they all return the last out param.
Doesn't seem like we need both of these:
  void  GetHighlightColorState(out boolean aMixed,out DOMString  outColor);
  wstring GetHighlightColor(out boolean mixed);
GetHighlightColor() is used in editor.js. Can we eliminate it?

Comment 5

[Kathleen :Brade]

Comment on attachment 72464 [details] [diff] [review]
Changes to nsIEditor.idl and nsIHTMLEditor.idl

+  boolean ShouldTxnSetSelection();
+  boolean CanCut();
+  boolean CanCopy();
+  boolean CanPaste(in long aSelectionType);
+  boolean CanDrag(in nsIDOMEvent aEvent);
+  nsIDOMNode CreateNode(in DOMString tag,
...

iirc, these should all begin with lowercase letters:
  canCopy, canPaste, canDrag, createNode, ...

It'd be great if we could all jump in and help Akkana get some real comments in
these files (explain each parameter, return values, etc.).  However, I don't
think that should hold up this patch.

Comment 6

[Akkana Peck]

Attachment: Better patch

Comment 7

[Charles Manske]

Comment on attachment 72487 [details] [diff] [review]
Better patch

Don't include 
void Shutdown()
for this fix. It depends
on other remove editorShell work.
I wonder if you should return boolean for nsIEditor::GetAttributeValue()

Comment 8

[Charles Manske]

Comment on attachment 72487 [details] [diff] [review]
Better patch

r=cmanske with suggested changes.

Comment 9

[Simon Fraser [no longer active]]

Ideally, all the methods should have leading lower case names. Now would be a
good time to fix that, before they start being used in JS.

Comment 10

[Akkana Peck]

Attachment: big patch: fix all the problems in our idl files

At Simon's request, I have renamed all the editor IDL methods in the files I
was touching to be lower-case, and have changed the JS cases I found which were
using those methods in their upper-cased incarnations.	And as long as we're
trying to make the editor IDL right, it turns out that nsITableEditor,
nsIEditorMailSupport and nsIEditorStyleSheets were never IDL-ified; two of them
were put in the idl directory but they were only partially converted and were
not being built.  We need all those interfaces if we're going to make people
stop using the editor shell.  So I've converted those three, and made the
necessary changes to editor files.  Unfortunately, that results in a huge
patch, primarily because most of the table editor methods were nonscriptable
(passing references) and had to be changed to pass pointers.  While I was in
there I fixed some warnings, cleaned up or improved some of the javadoc
comments (we still have quite a few methods that aren't documented, though),
and a lot of formatting problems (lines too long, random spacing).

Charley: need review.
Simon: need super-review.

Comment 11

[Akkana Peck]

I didn't mean to include EdReplace.js and EdReplace.xul in that patch; they are
part of bug 80805, dependant on this bug and are also awaiting review and sr.

Comment 12

[Akkana Peck]

Kathy, I'd appreciate it if you could also look this over and see if I missed
anything or if there's something you think should be done differently.

Comment 13

[Kathleen :Brade]

Comment on attachment 72878 [details] [diff] [review]
big patch: fix all the problems in our idl files

extra space after 'attribute' on this line:
+  readonly attribute  nsIDOMElement rootElement;

I'd remove this line altogether (we can always add it later if we want to
implement that):
+  //readonly attribute nsIPresShell presShell;

in the following block, you *could* (but not really necessary) rearrange code:
-nsHTMLEditor::GetCellIndexes(nsIDOMElement *aCell, PRInt32 &aRowIndex, PRInt32
&aColIndex)
+nsHTMLEditor::GetCellIndexes(nsIDOMElement *aCell,
+			      PRInt32 *aRowIndex, PRInt32 *aColIndex)
 {
+  NS_ENSURE_ARG_POINTER(aRowIndex);
+  NS_ENSURE_ARG_POINTER(aColIndex);
   nsresult res=NS_ERROR_NOT_INITIALIZED;
-  aColIndex=0; // initialize out params
-  aRowIndex=0;
+  *aColIndex=0; // initialize out params
+  *aRowIndex=0;

NS_ENSURE_ARG_POINTER(aRowIndex);
*aRowIndex = 0;
NS_ENSURE_ARG_POINTER(aColIndex);
*aColIndex = 0;

This line could be moved up with the other NS_ENSURE_ARG_POINTER lines in
GetCellDataAt:
 if (!aCell) return NS_ERROR_NULL_POINTER;

publish.js should be cvs removed; no one should be using it

looks great!

I'll let Charley check the checkbox but you have  my r=brade :-)

Comment 14

[Akkana Peck]

Attachment: new MsgComposeCommands.js diff

Charley pointed out that I missed changing composer's use of flags (now an
editor attribute rather than methods).	Here's a new patch just for
MsgComposeCommands.js.	It overrides MsgComposeCommands.js in the big patch.

Comment 15

[Charles Manske]

Comment on attachment 72878 [details] [diff] [review]
big patch: fix all the problems in our idl files

Wow! 
Ignoring Replace dialog stuff and msgComposer,
r=cmanske

Comment 16

[Charles Manske]

Comment on attachment 73014 [details] [diff] [review]
new MsgComposeCommands.js diff

needs work.

Comment 17

[Kathleen :Brade]

do we need to make any changes to Netscape's commercial tree?  If so, what is
the corresponding bugscape bug #?

Comment 18

[Akkana Peck]

Cc ducarroz for the MsgComposeCommands.js change.  J-F, can you review this
small change?

Kathy, good point!  It turns out there are a couple of lines in that have to
change in the commercial build: bugscape bug 12419 now tracks that.

Comment 19

[Akkana Peck]

Attachment: Improved MsgComposeCommands.js

Cleaner way of handling the flags in MsgComposeCommands.js, suggested by
Charley.

Comment 20

[Jean-Francois Ducarroz]

Comment on attachment 73019 [details] [diff] [review]
Improved MsgComposeCommands.js

R=ducarroz

Comment 21

[Simon Fraser [no longer active]]

Index: editor/idl/nsIHTMLEditor.idl
+  void  Align(in AString aAlign);

You missed a lowercasing.

-  void   SetCSSEnabled(in boolean aIsCSSPrefChecked);
+  void setCSSEnabled(in boolean aIsCSSPrefChecked);
 
-  /** IsCSSEnabled answers a boolean which is true is the HTMLEditor has been
+  /** isCSSEnabled answers a boolean which is true is the HTMLEditor has been
     * instantiated with CSS knowledge and if the CSS pref is currently checked
     *
-    * @param aIsCSSEnabled [OUT] true if CSS handled and enabled
+    * @return    true if CSS handled and enabled
     */
-  void   IsCSSEnabled(out boolean aIsCSSEnabled);
+  boolean isCSSEnabled();

Change to a read/write attribute?

-  nsIDOMCSSStyleRule ParseStyleAttrIntoCSSRule(in wstring aString);
+  nsIDOMCSSStyleRule parseStyleAttrIntoCSSRule(in wstring aString);

wstring -> AString?

+    * Returns NS_EDITOR_ELEMENT_NOT_FOUND if an element is not found
+    * (passes NS_SUCCEEDED macro)
     *   You can scan for all cells in a row or column
     *   by iterating through the appropriate indexes
     *   until the returned aCell is null
     */
-  void GetCellAt(in nsIDOMElement aTable, in long  aRowIndex, in long 
aColIndex, out nsIDOMElement aCell);
+  nsIDOMElement getCellAt(in nsIDOMElement aTable,
+                          in long  aRowIndex, in long aColIndex);

This return error will be inaccessible from JS. Do we need to fix that? Same for
other methods that do this.

+  void getFirstRow(in nsIDOMElement aTableElement, out nsIDOMNode aRow);
+  void getNextRow(in nsIDOMNode aTableElement, out nsIDOMNode aRow);

Return the row?

+  void GetFirstSelectedCell(out nsIDOMElement aCell, out nsIDOMRange aRange);

lowercase.

Index: editor/ui/dialogs/content/EdReplace.js
Some changes in here that look unrelated.

Fix those, and sr=sfraser

Comment 22

[Akkana Peck]

Attachment: Big patch incorporating Simon's suggestions

Comment 23

[Simon Fraser [no longer active]]

Comment on attachment 73751 [details] [diff] [review]
Big patch incorporating Simon's suggestions

sr=sfraser

Comment 24

[Charles Manske]

Comment on attachment 73751 [details] [diff] [review]
Big patch incorporating Simon's suggestions

r=cmanske

Comment 25

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

The final "big patch" doesn't seem to incorporate the second to last
patch.  Is that needed as well?  Doesn't it need super-review?

How extensively has this been tested?  Did you search the whole tree in
some way (LXR?) looking for the case changes that you needed to make?
It seems there's a lot of potential for JS errors resulting from missed
case changes in JS for this type of change.

(I also noticed a bunch of style nits I'd pick if I were reviewing the
patch.  I've been trying to restrain myself from reviewing as part of
the approval process, so feel free to ignore these or fix them sometime
in the future, but:
 * There are bunch of what I consider to be bogus NS_ENSURE_ARG_POINTER
   statements -- I think callers deserve to crash if they pass null for
   something declared in IDL as |out| or as a return value, although I
   don't object to assertions.
 * I *think* the [const] on |in nsAString| is extraneous -- the |in|
   should get you the C++ |const|.
 * In nsIEditorStyleSheets.idl:
   * I think you should be using |[ptr] native| and a forward declaration
     in a C++ block for nsICSSStyleSheet, since nsICSSStyleSheet isn't in
     IDL (although it might be that it should be based on whether it's
     XPCOM).
   * There are a bunch of final |out nsICSSStyleSheet| that should be
     return types.
And, FWIW (in response to comment 21), there is a way of accessing
success return values from JS, although I forget what it is.)

Comment 26

[Simon Fraser [no longer active]]

Comment on attachment 73019 [details] [diff] [review]
Improved MsgComposeCommands.js

sr=sfraser

Comment 27

[Akkana Peck]

Simon has now explicitly sr'ed the mail and IM files.

Re NS_ENSURE_ARG_POINTER: I've gotten burned before when a reviewer told me "It
should never happen that this is called with a null argument," and then for the
next year I got blamed every time there was a crash in that code.  Please allow
maintainers the option of checking arguments so their components can be robust
no matter what callers do.

Re searching: I searched using both lxr and recursive grep for patterns like
"hell.ed (i.e. catching editorShell.editor but with flexibility in
capitalization).  Previously the only way to get the nsIEditor interface from JS
was via the "editor" member of the editor shell.  I also searched for
"editor.getflags" to see if that would turn up anything additional, but it didn't.

New patch addressing the other concerns coming.

Comment 28

[Akkana Peck]

Attachment: Three files changed re dbaron's comments

1. Removes all the [const]s from in AString declarations.
2. Makes a C++ block as requested for nsICSSStyleSheet.
3. Makes those out nsICSSStyleSheet arguments into return parameters.
This small patch only includes files changed vs. the previous patch:
nsIEditorStyleSheets.idl, nsIEditorMailSupport.idl, nsIEditor.idl.

Comment 29

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

Comment on attachment 74158 [details] [diff] [review]
Three files changed re dbaron's comments

I pinged sfraser on IRC and he seemed happy with these changes, so a=dbaron.

Comment 30

[Akkana Peck]

Fix is in!

Comment 31

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Akkana : you removed GetHighlightColor() but did not change the calls in
editor.js 

Comment 32

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: new patch for getHighlightColorState()

Comment 33

[Kathleen :Brade]

Comment on attachment 74332 [details] [diff] [review]
new patch for getHighlightColorState()

r=brade

Comment 34

[kinmoz]

Comment on attachment 74332 [details] [diff] [review]
new patch for getHighlightColorState()

sr=[e-mail]

Comment 35

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

Comment on attachment 74332 [details] [diff] [review]
new patch for getHighlightColorState()

If getHighlightColor doesn't exist anymore, why does the second of the two
changes in this patch still use it?

Comment 36

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

baahh, glazou gets a knock-knock on his head because he did not attach the right
file...

Comment 37

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: new patch for getHighlightColorState() v2.0

Comment 38

[Kathleen :Brade]

Comment on attachment 74347 [details] [diff] [review]
new patch for getHighlightColorState() v2.0

r=brade

Comment 39

[kinmoz]

Comment on attachment 74347 [details] [diff] [review]
new patch for getHighlightColorState() v2.0

sr=[e-mail]

Comment 40

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

Comment on attachment 74347 [details] [diff] [review]
new patch for getHighlightColorState() v2.0

a=dbaron for trunk checkin

Comment 41

[Akkana Peck]

The tree didn't open at a reasonable time for Daniel or Kathy, so I've checked
in the fix for them.  Thanks all!

Comment 42

[[e-mail]]

Would someone mind explaining why some code in EdFieldSetProps.js was changed?

Comment 43

[Akkana Peck]

Neil: The editor API IDL files were cleaned up (that's what this bug was about)
to follow the JS convention that function names should begin with lower-case. 
EdFieldSetProps.js had references to editor.DeleteNode and editor.InsertNode,
which needed to be lowercased.

Comment 44

[[e-mail]]

Sorry, I confused this patch with another change.

Comment 45

[sujay]

Charley/Akkana, can one of you please verify this and mark verified-fixed..?

thanks.

Comment 46

[Akkana Peck]

Verifying.