In order for editor embedding to be complete, we need to complete this task: * All of the commands that embedders expect to use should be moved to C++ * Each command needs to be documented (as implemented?) for nsICommandParams

-->brade Midas needs some commands in C++ (exact list to come)

* clean up some QIs and return values * move editor QIs near top of methods * add inserttag (insert image and insert link) and fontsize commands * rename "cmd_insertHTML" to "cmd_insertHTMLNoUI" since it currently conflicts with a JS command with the same name * turn on image insertion, link insertion and font size commands in midas

It would be useful to have a bit more context in this patch; something like cvs diff -u12. nsCOMPtr<nsIEditor> editor = do_QueryInterface(refCon); - if (!editor) return NS_ERROR_NOT_INITIALIZED; + if (!editor) return NS_ERROR_INVALID_ARG; NS_ERROR_NOT_INITIALIZED is more appropriate. The controller will have a null refcon before the editor has been created (several instances of this in the patch). + nsCOMPtr<nsIAtom> fontAtom = getter_AddRefs(NS_NewAtom("font")); Use do_GetAtom(). (Several places). nsFontSizeStateCommand is imposing a behavior that embedders might not want. The GetCurrentState() throws away information (just returning 'mixed'), when the embedder might want to know the 'all, 'any' 'first' flags. The intention here was that we should provide all the information back to the caller, via command params. If information is hard to compute. we can only provide information that the embedder asks for (via empty entries in the command params). + char *tempAttrib = nsnull; + nsresult rv = aParams->GetCStringValue(STATE_ATTRIBUTE, &tempAttrib); Use nsXPIDLCString? Maybe we should fix nsICommandParams to use nsA{C}String to avoid allocations. Also, I think the 'STATE_ATTRIBUTE' should be a wide string, since it can contain unicode characters. Your 'attrib.AssignWithConversion' will nuke non-ascii (even utf-8). More generally, *any* command param values that go into content need to be wide strings. + nsCOMPtr<nsIDOMElement> domElem; Move this down to just before it's use. + if (0 == nsCRT::strcmp(mTagName, "a")) Does this need to be case insensitive? +#define NS_REGISTER_TAG_COMMAND(_cmdClass, _cmdName, _tagName) \ + { \ + _cmdClass* theCmd = new _cmdClass(_tagName); \ + if (!theCmd) return NS_ERROR_OUT_OF_MEMORY; \ + rv = inCommandManager->RegisterCommand(_cmdName, \ + NS_STATIC_CAST(nsIControllerCommand *, theCmd)); \ + } + Minor spacing issues (tabs?).

Comment on attachment 106365 [details] [diff] [review] patch I'd prefer if you use "cmd_insertHTML" instead of "cmd_insertHTMLNoUI", since I'm fixing that problem separately in bug 180303 by using "cmd_insertHTMLDlg" for the JS command.

Please use understandable names. ""cmd_insertHTMLWithDialog" indicates much more clearly what the command does.

I removed all of the changes for error return codes except one or two. I disagree on the usage of NS_ERROR_NOT_INITIALIZED in some of the commands but this bug isn't the place to argue this. The exceptions (where I do change them) is because the editor was already checked for null and returned NS_ERROR_NOT_INITIALIZED so subsequent QI's that fail should have invalid argument. I cleaned up the entire file to use do_GetAtom() (not just the code I was adding). For nsFontSizeStateCommand, I don't consider this command finished nor are most of the commands in this file finished. We need to have many more attributes returned to embedders. I expect this to be covered in a different bug. What this command provides now is consistent with other commands in this file. Use of nsXPIDLCString consistently in file rather than using char* >Maybe we should fix nsICommandParams to use nsA{C}String to avoid allocations. I agree that we should consider doing this. Possibly as part of the bug on defining the string types used? >Also, I think the 'STATE_ATTRIBUTE' should be a wide string... I agree with this also but I haven't addressed this in this patch. I'd like to see this as part of a different patch which addresses STATE_ATTRIBUTE for all commands which should be considered. >>+ if (0 == nsCRT::strcmp(mTagName, "a")) > >Does this need to be case insensitive? No, since the command is created with lower case tags. I don't see much point to checking case in this case. This patch doesn't address the bug about having two commands named "cmd_insertHTML"; that issue should also be covered in other bug(s).

+ nsXPIDLCString s; + aParams->GetCStringValue(STATE_ATTRIBUTE, getter_Copies(s)); nsAutoString tString; - tString.AssignWithConversion(tValue); - nsMemory::Free(tValue); + tString.AssignWithConversion(s); This still worries me. It will clobber a non-ASCII attribute value. nsFontSizeStateCommand::GetCurrentState is still throwing a way information that the embedder might want (allHas, anyHas).

>nsFontSizeStateCommand::GetCurrentState is still throwing a way information that > the embedder might want (allHas, anyHas). OK, read your comment above on this. Can we file another bug to clean up exisiting commands?

Comment on attachment 106663 [details] [diff] [review] clean up of previous patch Just be sure to file a bug to change "cmd_insertLinkNoUI" to "cmd_insertLink" and change "cmd_insertImageNoUI" to "cmd_insertImage" once we decide how we are going to change all the commands that require dialogs.

bug 180745 submitted for anyHas/allHas issues bug 166465 covers editor embedding string issues (may need to split that apart?)

Comment on attachment 106663 [details] [diff] [review] clean up of previous patch obsoleting patch which was checked in

Discussed in topembed bug triage. Can we get an updated milestone, please?

topembed- per saari (at this time, no known missing commands for embedders)