Currentlty, the editor creates 2 command controllers in nsEditorShell.cpp: First contains the basic editor commands shared with textfields (copy, paste etc.), the 2nd are the basic HTML formating commands. Each of these now automatically registers their command in their repective "Init()" methods. Another controller (an instance of nsComposerController) is created in UI code in JS that holds the file open/save and other top-level window commands. This currently is troublesome because the same HTML formatting commands are automatically registered and have to be "unregistered". So the controller code needs to be reorganized to make it more flexible. We need: 1. The option to create a singleton (non-global) controller to share among editors created in C++ code. nsIEditorController should be obtainable as a Service to support this. 2. Classes derived from nsIEditorController need to support instantiation so we can create controller objects in JS.

I believe this is for factoring the controllers; I hope the summary change is ok

Mike is working on this now.

patch will alter use of "editor controllers" to a single class. now the factories will produce a generic version of the controllers and add the properly registered singleton commandmanagers to them. Also renamed nsIEditorController to nsIControllerContext for more generic use outside of editor. new files to be added in more attachments

new file 1

new file2

new idl file

patch just for the idl changes. removed 1 idl from editor/idl and added one to embedding/components/commandhandler/public

unified patch with all code in it..

Comment on attachment 103075 [details] [diff] [review] patch from /mozilla comments sent to mjudge via irc

fix handles the issues brade specified

ok this fixes the left out changes to Makefile.in... oops.

forgot the layout makefile.in. also a _content in the .js file. all good now

Comment on attachment 103127 [details] [diff] [review] patch from /mozilla in nsEditingSession.cpp + nsCOMPtr<nsIControllerContext> editorController(do_QueryInterface(controller)); should use "=" (it's the preferred style) There are 3 places you are touching that should be corrected. There is another bug that fixes the rest. in ComposerCommands.js, define a const for the uninitialized controller id (something like): const kUninitializedControllerID = -1; and then use that for gComposerJSCommandControllerID Better yet, would be if that was defined in some idl file. in nsIControllerContext.idl, use javadoc comment style: /** * text here * more text here */ I don't see the changes for Macintosh builds for adding nsIControllerContext.idl I don't see any build changes for Macintosh for adding nsBaseCommandController.cpp in nsBaseCommandController.cpp, move this line to not be in the middle of the implementations (it's too hard to see/find): + nsCOMPtr<nsIControllerCommandManager> mCommandManager; // our reference to the command manager Fix this comment: + //dont let users get the command manager if its immutable. they may harm it in some way. Capitalize "Don't" and "They" and add the apostrophe to "Don't" Why do we need both of these: NS_REGISTER_ONE_COMMAND NS_REGISTER_FIRST_COMMAND can one be removed? Same questions for these two: NS_REGISTER_NEXT_COMMAND NS_REGISTER_LAST_COMMAND Also, check on mozilla.org for the latest versions of the licenses to be used in new files. The new files you are adding have different licenses.

Brade:, in reviews by Darin, he said that this pattern was more efficient: nsCOMPtr<nsIFoo> foo(do_QueryInterface(bar))

The difference is miniscule, and only applicable to certain compilers.

NS_REGISTER_ONE_COMMAND is used if you are registering a single object that handles just one command NS_REGISTER_FIRST_COMMAND/NS_REGISTER_NEXT_COMMAND/NS_REGISTER_LAST_COMMAND are for registering a single object that handles multiple commands

Note: I have tested the "patch from mozilla" and it works fine along with my code in bug 174439, bug 169029, and bug 133598, which completely remove editorShell from Composer.

I believe I hit all the required changes. updated idl, changed some spellings. moved some code around. I will still seek some help when checking this in from mac folks to make sure I make the proper changes to the project files.

Comment on attachment 103398 [details] [diff] [review] /mozilla patch r=cmanske

+// our reference to the editor command manager singleton +nsCOMPtr<nsIControllerCommandManager> gComposerCommandManager; Static nsCOMPtrs are discouraged, because some platforms have issues calling static ctors/dtors, I think. In addition, library unload is usually too late a time to release XPCOM objects (since XPCOM has been shut down long before then). It would be better to use a raw pointer, and release it via a ShutDown method (see nsLayoutModule for an example; there may be a better way to do it). In SetupFrameControllers: + nsCOMPtr<nsIControllerContext> composerController = do_QueryInterface(controller); Why this line? composerController is not used. +// our reference to the editor command manager singleton +nsCOMPtr<nsIControllerCommandManager> gEditorCommandManager; Same comments as above. In function GetComposerCommandManager() (in js) + controller = window._content.controllers.getControllerById(gComposerJSCommandControllerID); .. + window._content.controllers.appendController(controller); Need to be careful here, to make this work with multiple <editor> tags. You might want to use something other than _content to make sure that you're messing with the same <iframe> that the editor is living on. Failing that, this needs a big comment. In nsBaseCommandController.h: + //if editor is null then look to mContent. this is for dual use of window and content + //attached controller. + nsISupports *mCommandRefCon; This comment doesn't seem to make sense here. #define NS_REGISTER_ONE_COMMAND(_cmdClass, _cmdName) \ etc. I think I'd nuke these macros here. Anyone trying to register commands on one of these guys will only have included the interface header, not this file. This needs Mac build changes to add nsIControllerContext.idl to a MANIFEST_IDL and an IDL project, and to add nsBaseCommandController.cpp to the build.

Comment on attachment 103398 [details] [diff] [review] /mozilla patch woah. You should not be doing custom singleton work here to make sure there is only one gComposerCommandManager, not to mention that fact that you should never have global nsCOMPtrs. Not to mention the fact that you're leaking gComposerCommandManager! Instead, you should be using the singleton stuff that's already accessible in XPCOM: services. Basically in the Init() method of your composer controller, you should be calling do_GetService() to get a global nsControllerCommandManager, and attaching it there. Let XPCOM do the singleton management.

There are missing changes required in content/html/content/src/nsHTMLInputElement.cpp and nsHTMLTextAreaElement.cpp Delete calls to editorController->Init() after creating the controller. Mike will supply the patch. I made these changes in my tree and now Input and TextAreas work fine.

patch has all changes as well as the changes for mime.

added 2 files from /content. removed files from the event code for the nsHRFrame.cpp

This contains mjudge's previous controller work plus: 1. Spliting of "composercontroller" into "plaintexteditorcontroller" and "htmleditorcontroller" so plaintext commands can always be registered before an editor is created. "htmleditorcontroller" is created and commands registered only if mimetype is "text/html", which is determined after loading a page. 2. Mechanism for notifying when editor is created and document is loaded is done via a command: "obs_documentCreated" and the "cantEditReason" error code is available from the nsIEditingSession [this will probably be moved into a private interface later]. The proper way to get this error code is by using commandManager->GetCommandState(), which sets the error as the "state_data" value in an nsICommandParameters object. 3. If an error occures during startup, no editor is made. If we were creating a new window and loading a URL, we start a timer that will load "about:blank" and create an editor. Thus an error such as "file not found" or "cant edit this mimetype" leaves the user in an empty page. 4. The nsComposerCommands changes include both mjudge work for redesiging the controllers as well as cmanske work for adding new commands. These changes are inseparable and should be reviewed together anyway. Mike is adding a method to nsIEditingSession so the embedder can tell us what mimetype(s) are acceptable to edit, but we wanted to get this significantly large patch before some eyes ASAP.

Fixes per reviewer comments: 1. Changed names of editor startup feedback to "editorStatus" etc. 2. Moved "cmd_insertText" and "cmd_pasteAsQuotation" into nsEditorCommands 3. Renamed "plaintexteditorcontroller" to "editordocstatecontroller". 4. Fixed spaces, tabs, and long lines for most files.

Comment on attachment 105595 [details] [diff] [review] Commands and Controllers rewrite v1 >Index: embedding/components/commandhandler/public/nsIControllerContext.idl ... >+ * Portions created by the Initial Developer are Copyright (C) 2000 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): Check the license here; is the above old? >+class nsCString; Is the above line needed? >+ /** Set the cookie that is passed to commands >+ */ Use JavaDoc style comments (and below as well). >+ void SetCommandRefCon( in nsISupports commandRefCon ); >+ void SetControllerCommandManager(in nsIControllerCommandManager aCommandManager); These are inconsistent in these ways: spacing inside parens first letter of parameter >Index: embedding/components/commandhandler/src/Makefile.in > CPPSRCS = \ >+ nsBaseCommandController.cpp \ I don't see the corresponding Mac project changes for adding this file (in the other patch or this patch). >Index: embedding/components/commandhandler/src/nsBaseCommandController.cpp ... >+ * Portions created by the Initial Developer are Copyright (C) 2001 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): Check the license here; is the above old? >+#if 0 >+ >+NS_IMETHODIMP nsBaseCommandController::Init(nsISupports *aCommandRefCon) >+{ >+ nsresult rv; ... >+ return NS_OK; >+} >+#endif //0 Can the above block be removed? >+NS_IMETHODIMP nsBaseCommandController::SetControllerCommandManager(nsIControllerCommandManager *aCommandManager) The above line is > 80 chars. >+ //dont let users get the command manager if its immutable. they may harm it in some way. Fix this comment. If you are going to use periods, use mixed case. Fix spelling of "Don't" >+ if (!mImmutableManager && mCommandManager && aIID.Equals(NS_GET_IID(nsIControllerCommandManager))) split this line onto 2 lines >+NS_IMETHODIMP nsBaseCommandController::DoCommandWithParams(const char *aCommand, nsICommandParams *aParams) >+NS_IMETHODIMP nsBaseCommandController::GetCommandStateWithParams(const char *aCommand, nsICommandParams *aParams) long lines >Index: embedding/components/commandhandler/src/nsBaseCommandController.h ... >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. Check the license here; is the above old? >+class nsIEditor; Can this line be deleted? >+// the base editor controller is used for both text widgets, and basic text editing >+// commands in composer. The refCon that gets passed to its commands is an nsIEditor. Capitalize the first "the" to be consistent with the rest of the comment. >+ //if editor is null then look to mContent. this is for dual use of window and content >+ //attached controller. Use mixed case (as above) or remove the periods and put on separate lines. >Index: editor/idl/MANIFEST >-nsIEditorController.idl Is this file being cvs removed? I don't see corresponding build changes for this. >Index: editor/libeditor/base/nsEditorController.cpp >+#include "nsIControllerCommandManager.h" I'm not sure why you need to add this here; you aren't adding any other lines. Was it being implicitly included elsewhere and it no longer is? Did I miss something in the diff? >Index: editor/libeditor/build/nsEditorRegistration.cpp >+ nsCOMPtr<nsIControllerContext> context = do_CreateInstance("@mozilla.org/embedcomp/base-command-controller;1",&rv); >+ nsCOMPtr<nsIControllerCommandManager> editorCommandManager(do_GetService(NS_CONTROLLERCOMMANDMANAGER_CONTRACTID, &rv)); split above lines >Index: editor/composer/public/nsIEditingSession.idl >+ * is placed in attribute cantEditReason >+ */ remove spaces after the closing comment >+ const long eCantEditNoReason = 0; >+ const long eCantEditStartupInProgress = 1; >+ const long eCantEditFramesets = 2; >+ const long eCantEditMimeType = 3; >+ const long eCantEditFileNotFound = 4; >+ const long eCantEditOther = 9; I'd like to see the frameset const be later in the list (since I hope it'll be removed some day). Change all of the above as we discussed on irc (remove 'Cant', change 'NoReason', etc) >+ * Reason why editor creation or document loading failed is place here The above line doesn't read well, should it be "placed" (if so, I prefer "stored"). > /* Init the editing session, passing the window that will be the root of >+ * the session (usually the content root frame.) reverse the last two characters: ). >+ void makeWindowEditable(in nsIDOMWindow aWindow, in string aEditorType, in boolean inDoAfterUriLoad); The above line is still too long, can you split it up since you're touching it anyways? >+ /** Setup editor and related support objects >+ */ and >+ /** Destroy editor and related support objects >+ */ use javadoc style comments as you did above in this file. >Index: editor/composer/src/nsComposerCommands.cpp >+#include "nsIPlaintextEditor.h" Be sure to remove this line when you move the insert text command. >+#include "nsIEditingSession.h" Why do we need the editing session here? >@@ -216,13 +218,13 @@ > nsCOMPtr<nsIHTMLEditor> htmlEditor(do_QueryInterface(refCon)); > if (!htmlEditor) >- return NS_OK; >+ return NS_ERROR_NOT_IMPLEMENTED; > > nsCOMPtr<nsIEditor> editor(do_QueryInterface(htmlEditor)); > if (!editor) >- return NS_ERROR_NOT_IMPLEMENTED; >+ return NS_ERROR_INVALID_ARG; Can't we just do 1 QI here (delete the first block and QI on refCon in 2nd block)? >+nsInsertPlaintextCommand::IsCommandEnabled(const char * aCommandName, nsISupports *refCon, PRBool *outCmdEnabled) >+nsInsertPlaintextCommand::DoCommandParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) >+nsInsertPlaintextCommand::GetCommandStateParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) These are going to move to another file, but make sure they aren't too long. >+ if (text.Length() > 0) I think the preferred thing is to check for IsEmpty() --> if (!text.IsEmpty()) >+nsInsertHTMLCommand::IsCommandEnabled(const char * aCommandName, nsISupports *refCon, PRBool *outCmdEnabled) >+nsInsertHTMLCommand::DoCommandParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) >+nsInsertHTMLCommand::GetCommandStateParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) long lines >+ if (html.Length() > 0) Use IsEmpty() as above. >+nsSetDocumentStateCommand::IsCommandEnabled(const char * aCommandName, nsISupports *refCon, PRBool *outCmdEnabled) >+nsSetDocumentStateCommand::DoCommandParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) long lines >+ NS_ENSURE_ARG_POINTER(aParams); I expect to see this before the QI or inside the if below it. >+ if (!nsCRT::strcmp(aCommandName,"cmd_setDocumentModified")) add a space after the ',' >+nsSetDocumentStateCommand::GetCommandStateParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) long line >+ NS_ENSURE_ARG_POINTER(refCon); I'm not sure you need to do this since you are going to QI it below. >+ if (!nsCRT::strcmp(aCommandName,"cmd_setDocumentModified")) add a space after ',' I find it somewhat confusing that the comment below is about state notification but there are document state commands above and below it. Which does this correspond to? Can you be sure to include all the commands that are covered by the comment? I especially don't understand "aData" parameter in Observe. >+/** >+ * Commands just for state notification >+ * How to use: >+ * 1. Get the nsICommandManager for the current editor >+ * 2. Implement an nsIObserve object, e.g: >+ * >+ * void Observe( in nsISupports aSubject, // The nsICommandManager calling this Observer >+ * in string aTopic, // Name of the command e.g.: "obs_documentCreated" >+ * // or "obs_documentWillBeDestroyed" >+ * in wstring aData ); // "command_status_changed" >+ * >+ * 3. Add the observer by: >+ * commandManager.addObserver(observeobject, obs_documentCreated); >+ * 4. In the appropriate location in editorSession, editor, or commands code, >+ * trigger the notification of this observer by something like: >+ * >+ * nsCOMPtr<nsICommandManager> commandManager = do_GetInterface(mDocShell); >+ * nsCOMPtr<nsPICommandUpdater> commandUpdater = do_QueryInterface(commandManager); >+ * if (!commandUpdater) return NS_ERROR_FAILURE; >+ * commandUpdater->CommandStatusChanged(obs_documentCreated); The above lines don't line up with each other. >+ * 5. Use GetCommandStateParams() to obtain state information >+ * e.g., any error codes when creating an editor are Do we want to use "error codes" or "state" or ? >+nsDocumentStateCommand::IsCommandEnabled(const char* aCommandName, nsISupports *refCon, PRBool *outCmdEnabled) long line >+ // Always return false to prevent callers from using DoCommand() this comment could be expanded or clarified; perhaps use "to discourage" instead of "to prevent" also, should include DoCommandParams: DoCommand*()? >+nsDocumentStateCommand::DoCommandParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) >+nsDocumentStateCommand::GetCommandStateParams(const char *aCommandName, nsICommandParams *aParams, nsISupports *refCon) long lines >+ if (!nsCRT::strcmp(aCommandName,"obs_documentCreated")) add space after ',' >Index: editor/composer/src/nsComposerCommands.h >+class nsIEditor; I don't understand why this is added here; can it be removed? >Index: editor/composer/src/nsComposerCommandsUpdater.cpp >+#include "nsIDOMWindow.h" >+: mDocShell(nsnull) Is there a tab on this line? >+ UpdateOneCommand("obs_documentCreated"); >+ UpdateOneCommand("obs_documentWillBeDestroyed"); Can we make a list (at the top of this file or in its header?) with these commands? I'm worried they will be lost in the code and hard to find. >+ const PRUint32 kUpdateTimerDelay = 250; //150; I don't like this change; 250 seems WAY too long. If we need to go with it; please clarify with a comment (not just "150;") >+ commandUpdater->CommandStatusChanged("cmd_setDocumentModified"); Do we have such a command? I don't see it in lxr.mozilla.org or in your patch except here. > nsComposerCommandsUpdater::SelectionIsCollapsed() > { >+ if (!mDOMWindow) return true; return PR_TRUE instead of true >Index: editor/composer/src/nsComposerController.cpp >+nsresult nsComposerController::RegisterPlaintextEditorCommands(nsIControllerCommandManager *inCommandManager) long line rename to RegisterDocumentListeners or something? >+nsresult nsComposerController::RegisterHTMLEditorCommands(nsIControllerCommandManager *inCommandManager) long line >+ nsresult rv; Why are you adding this? I don't see you use it. >Index: editor/composer/src/nsComposerController.h >+class nsIControllerCommandManager; >+// the plaintext editor controller is used for basic text editing and html editing >+// commands in composer. capitalize "the" (to be more consistent with the rest of the comment) >+// and after successfule editor creation it is changed to nsIEditor. "successful" >+#define NS_PLAINTEXTEDITORCONTROLLER_CID \ >+ { 0x50e95301, 0x17a8, 0x11d4, { 0x9f, 0x7e, 0xdd, 0x53, 0x0d, 0x5f, 0x05, 0x7c } } rename this for "document state controller"? >Index: editor/composer/src/nsComposerRegistration.cpp >+#include "nsIControllerContext.h" > NS_GENERIC_FACTORY_CONSTRUCTOR(nsEditorShell) Can this line be removed? >+ nsCOMPtr<nsIControllerContext> context = do_CreateInstance("@mozilla.org/embedcomp/base-command-controller;1", &rv); >+ nsCOMPtr<nsIControllerCommandManager> composerCommandManager(do_GetService(NS_COMPOSERSCONTROLLERCOMMANDMANAGER_CONTRACTID, &rv)); >+ nsCOMPtr<nsIControllerContext> context = do_CreateInstance("@mozilla.org/embedcomp/base-command-controller;1", &rv); >+ nsCOMPtr<nsIControllerCommandManager> composerCommandManager(do_GetService(NS_COMPOSERSCONTROLLERCOMMANDMANAGER_CONTRACTID, &rv)); long lines > { "Editor Shell Component", NS_EDITORSHELL_CID, > "@mozilla.org/editor/editorshell;1", nsEditorShellConstructor, }, > { "Editor Shell Spell Checker", NS_EDITORSHELL_CID, > "@mozilla.org/editor/editorspellcheck;1", nsEditorShellConstructor, }, Can these lines be removed? >Index: editor/composer/src/nsEditingSession.cpp >+#include "nsIDOMNSDocument.h" >+ >+#include "nsEditorParserObserver.h" > > #if DEBUG >+#include "nsIURI.h" > #define NOISY_DOC_LOADING 1 > #endif > > /*--------------------------------------------------------------------------- >@@ -89,12 +101,12 @@ > > ----------------------------------------------------------------------------*/ > nsEditingSession::~nsEditingSession() > { >- NS_IF_RELEASE(mStateMaintainer); > } > >+ mEditorType = strdup(aEditorType); Does this leak? >+ rv = SetRefConOnControllerById(controllers, NS_STATIC_CAST(nsIEditingSession*,this), mBaseCommandControllerId); >+ rv = SetRefConOnControllerById(controllers, NS_STATIC_CAST(nsIEditingSession*,this), mPlaintextCommandControllerId); long lines; add space after second ',' >+#ifndef FULL_EDITOR_HTML_SUPPORT >+static eHTMLTags gWatchTags[] = Why is this #ifdef'd? >+ nsresult rv; if possible, move this down (closer to where it'll be used) >+ if (NS_SUCCEEDED(aWindow->GetDocument(getter_AddRefs(doc))) && doc) //then lets check the mime type long line; remove "then lets"? >+ mimeCType = strdup("text/plain"); Is this a memory leak? >+ else if (!mimeType.Equals(NS_LITERAL_STRING("text/html"))) Are mimetypes guaranteed to be lower-case? >+ PRBool htmlController = PR_FALSE; rename so it's clearer when reading code that this is a boolean (isHTMLController, bHTMLController) >+ mEditorFlags = nsIPlaintextEditor::eEditorPlaintextMask | nsIPlaintextEditor::eEditorEnableWrapHackMask | nsIPlaintextEditor::eEditorMailMask; >+ mEditorFlags = nsIPlaintextEditor::eEditorPlaintextMask | nsIPlaintextEditor::eEditorEnableWrapHackMask; >+ mEditorFlags = nsIPlaintextEditor::eEditorPlaintextMask | nsIPlaintextEditor::eEditorEnableWrapHackMask; long lines >+ nsCOMPtr<nsIController> controller = do_CreateInstance("@mozilla.org/editor/htmleditorcontroller;1", &rv); long line >+ nsCOMPtr<nsIDOMWindowInternal> domWindowInt = do_QueryInterface(aWindow, &rv); >+ if (NS_FAILED(rv)) return rv; >+ >+ nsCOMPtr<nsIControllers> controllers; >+ rv = domWindowInt->GetControllers(getter_AddRefs(controllers)); >+ if (NS_FAILED(rv)) return rv; >+ >+ rv = controllers->AppendController(controller); >+ if (NS_FAILED(rv)) return rv; >+ >+ // Remember the ID for this controller >+ rv = controllers->GetControllerId(controller, &mHTMLCommandControllerId); >+ if (NS_FAILED(rv)) return rv; I've seen code like the above before; does it make sense to factor it? > if (!contentViewer) return NS_ERROR_FAILURE; >- >+ You removed some of the spaces from this line but not all of them. >+ rv = editor->AddDocumentStateListener(NS_STATIC_CAST(nsIDocumentStateListener*, stateMaintainer)); long line >+ nsCOMPtr<nsISelection> selection; >+ editor->GetSelection(getter_AddRefs(selection)); >+ nsCOMPtr<nsISelectionPrivate> selPriv = do_QueryInterface(selection); >+ if (!selPriv) return NS_ERROR_FAILURE; are there tabs on some of those lines? >+ nsCOMPtr<nsITransactionListener> transactionListener = do_QueryInterface(mStateMaintainer); long line >+ // Set the error state -- we will create an editor anyway and load empty doc later This is so cool!!!!!! :-) Does loading about:blank overwrite the error state? > if (makeEditable) >+ rv = SetupEditorOnWindow(domWindow); >+ >+ if (NS_FAILED(rv)) This seems a little weird to me; I was expecting this to all be inside the if (makeEditable) block. >+ nsAutoString url(NS_LITERAL_STRING("about:blank")); >+ nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(docShell)); >+ if (webNav) >+ webNav->LoadURI(url.get(), 0, nsnull, nsnull, nsnull); can we use NS_NAMED_LITERAL or avoid "url" altogether and just use it in LoadURI? >+nsEditingSession::SetRefConOnControllerById(nsIControllers* inControllers, nsISupports* inRefCon, PRUint32 inID) >+ nsCOMPtr<nsIControllerContext> editorController = do_QueryInterface(controller); // ok with nil controller long lines >Index: editor/composer/src/nsEditingSession.h >+ //THESE MEMBER VARIABLES WILL BECOME A SET WHEN WE EDIT MORE THAN ONE EDITOR PER EDITING SESSION >+ nsCOMPtr<nsISupports> mStateMaintainer; // we hold the owning ref to this. >+ nsCString mEditorType; //we need this to hold onto the type for invoking editor after loading uri long lines

Added missing entries for nsBaseCommandController.cpp

This includes all build files in embedding and editor dirs. It includes removing editorShell from editor build files.

This includes LOTS of new changes to simply reduce lines to 80chars. It doesn't include build files (see patch 105642) All comments by brade and sfraser are addressed as suggested. Other explanations: >@@ -216,13 +218,13 @@ > nsCOMPtr<nsIHTMLEditor> htmlEditor(do_QueryInterface(refCon)); > if (!htmlEditor) >- return NS_OK; >+ return NS_ERROR_NOT_IMPLEMENTED; > > nsCOMPtr<nsIEditor> editor(do_QueryInterface(htmlEditor)); > if (!editor) >- return NS_ERROR_NOT_IMPLEMENTED; >+ return NS_ERROR_INVALID_ARG; brade: Can't we just do 1 QI here (delete the first block and QI on refCon in 2nd block)? cmanske: because the command itself is only in an nsIHTMLEditor, so we need to be sure we have that interface. But we're using the nsIEditor::CanPaste to test if there's stuff on the clipboard, so I check for nsIEditor is for safety; it obviously isn't really needed. >+ nsresult rv; brade: Why are you adding this? I don't see you use it. cm: the command REGISTER macros requre it

Comment on attachment 105771 [details] [diff] [review] Commands and Controllers rewrite v4 >Index: embedding/components/commandhandler/public/nsIControllerContext.idl >=================================================================== >> * Portions created by the Initial Developer are Copyright (C) 2000 Why Copyright 2000? Shouldn't it be 2002? >> /** >> * Set the cookie that is passed to commands >> */ line up the closing up (add a space before '*'); 2 places >Index: embedding/components/commandhandler/src/nsBaseCommandController.cpp >=================================================================== >+ * Portions created by the Initial Developer are Copyright (C) 2001 Why 2001 and not 2002? >+ * Contributor(s): Add your name here? >+ // Don't let users get the command manager if its >+ // immutable. they may harm it in some way its --> it's they --> They way --> way. >Index: embedding/components/commandhandler/src/nsBaseCommandController.h >=================================================================== >+ * Portions created by the Initial Developer are Copyright (C) 1998 Why 1998 and not 2002? >+ * Contributor(s): Add your name here? >+#include "nsIControllerCommand.h" >+#include "nsIInterfaceRequestorUtils.h" >+#include "nsHashtable.h" >+#include "nsString.h" >+#include "nsWeakPtr.h" Are all of the above includes really needed for this header file or are they misplaced (belong in C++ file?)?? >+// the base editor controller is used for both text widgets, and basic text editing the --> The >Index: editor/composer/src/nsComposerCommands.cpp >=================================================================== >+nsPasteNoFormattingCommand::IsCommandEnabled(const char * aCommandName, ... > nsCOMPtr<nsIHTMLEditor> htmlEditor(do_QueryInterface(refCon)); > if (!htmlEditor) >- return NS_OK; >+ return NS_ERROR_NOT_IMPLEMENTED; Add a comment here explaining that this only works for html editors so we need to ensure we have an html editor. >+ aParams->SetBooleanValue(STATE_MIXED,anyOfSelectionHasProp Add space after ',' >+ nsCOMPtr<nsICommandParams> params = >+ do_CreateInstance(NS_COMMAND_PARAMS_CONTRACTID,&rv); Add space after ',' >@@ -382,7 +351,8 @@ >+ outInList = (0 == nsCRT::strcmp(tagStr, >+ NS_ConvertASCIItoUCS2(mTagName).get())); I wonder if instead we should do mTagName.Equals(NS_LITERAL_STRING(tagStr)) or similar? >+ PRBool inList = (0 == nsCRT::strcmp(tagStr, >+ NS_ConvertASCIItoUCS2(mTagName).get())); see above >+ nsCOMPtr<nsICommandParams> params = >+ do_CreateInstance(NS_COMMAND_PARAMS_CONTRACTID,&rv); add space after ',' >+ outInList = (0 == nsCRT::strcmp(tagStr, >+ NS_ConvertASCIItoUCS2(mTagName).get())); and >+ PRBool inList = >+ (0 == nsCRT::strcmp(tagStr, NS_ConvertASCIItoUCS2(mTagName).get())); mTagName.Equals? >+ nsCOMPtr<nsICommandParams> params = >+ do_CreateInstance(NS_COMMAND_PARAMS_CONTRACTID,&rv); space after ',' >+/** >+ * Commands for document state that may be changed via doCommandParams >+ */ would like to see a list of the actual command strings used here >+ if (!nsCRT::strcmp(aCommandName,"cmd_setDocumentModified")) add space after ',' >+/** >+ * Commands just for state notification >+ * How to use: would like to see the actual command strings listed in this comment >Index: editor/composer/src/nsComposerCommands.h >=================================================================== will continue here after lunch :-)

Comment on attachment 105769 [details] [diff] [review] Just build file changes -- All platforms v3 sr=sfraser

Comment on attachment 105771 [details] [diff] [review] Commands and Controllers rewrite v4 Comments: Index: embedding/components/commandhandler/public/nsIControllerContext.idl =================================================================== nsIControllerContext.idl needs some comments to explain the weird immutability thing with setting the nsIControllerCommandManager, and to explain the refCon. > void setCommandRefCon(in nsISupports commandRefCon); This needs a comment to say that the commandRefCon is *not* addreffed, and thus needs to outlive the controller. Index: embedding/components/commandhandler/src/nsBaseCommandController.cpp =================================================================== Need a comment in this file that explains the immutability stuff. + NS_INIT_ISUPPORTS(); + nsresult rv; + mCommandManager = + do_CreateInstance(NS_CONTROLLERCOMMANDMANAGER_CONTRACTID, &rv); Maybe assert if this failed (since rv is never passed back). + // Don't let users get the command manager if its + // immutable. they may harm it in some way + if (!mImmutableManager && mCommandManager && + aIID.Equals(NS_GET_IID(nsIControllerCommandManager))) + return mCommandManager->QueryInterface(aIID, result); I don't like this at all. I don't think the behaviour of GetInterface should be conditional on how it's created. I think the nsIControllerCommandManager itself should store the immutability flag, and it should return errors if you try to add/remove commands. Index: embedding/components/commandhandler/src/nsBaseCommandController.h =================================================================== +// the base editor controller is used for both text widgets, and basic text editing +// commands in composer. The refCon that gets passed to its commands is an nsIEditor. + This comment is not appropriate here. // If editor is null then look to mContent + // This is for dual use of controllers for window and content Nor is this one. Index: editor/composer/public/nsIEditingSession.idl =================================================================== When are we going to split this into public and private APIs? + /** + * Error codes when we fail to create an editor + * is placed in attribute editorStatus + */ + const long eEditorOK = 0; + const long eEditorCreationInProgress = 1; + const long eEditorErrorCantEditMimeType = 2; + const long eEditorErrorFileNotFound = 3; + const long eEditorErrorCantEditFramesets = 8; + const long eEditorErrorUnknown = 9; + + /** + * Status after editor creation and document loading + * Value is one of the above error codes + */ + readonly attribute unsigned long editorStatus; This stuff needs a comment to say that's it's temporary. aParams->SetBooleanValue(STATE_ALL,allOfSelectionHasProp); aParams->SetBooleanValue(STATE_BEGIN,firstOfSelectionHasProp); aParams->SetBooleanValue(STATE_END,allOfSelectionHasProp);//not completely accurate - aParams->SetBooleanValue(STATE_MIXED,anyOfSelectionHasProp && !allOfSelectionHasProp); + aParams->SetBooleanValue(STATE_MIXED,anyOfSelectionHasProp Need spaces after commas here. +nsSetDocumentStateCommand::DoCommandParams(const char *aCommandName, + nsICommandParams *aParams, + nsISupports *refCon) +{ + nsCOMPtr<nsIEditor> editor = do_QueryInterface(refCon); + if (!editor) + return NS_ERROR_INVALID_ARG; + + if (!nsCRT::strcmp(aCommandName,"cmd_setDocumentModified")) Need a comment here to say why you are comparing command name strings. + if (!nsCRT::strcmp(aCommandName, "obs_documentCreated")) ditto. + nsCOMPtr<nsIAtom> styleAtom = getter_AddRefs(NS_NewAtom(aProp)); use do_GetAtom (i think that's the correct name). + mEditorStatus = eEditorCreationInProgress; + mLoadBlankDocTimer->InitWithFuncCallback(nsEditingSession::TimerCallback, + (void*)docShell, + 10, nsITimer::TYPE_ONE_SHOT); How did you determine this timeout? Why does it need to happen on a timer anyway? Index: editor/idl/nsIEditorController.idl =================================================================== Isn't this file dead with this patch? sr=sfraser with those changes.

Comment on attachment 105771 [details] [diff] [review] Commands and Controllers rewrite v4 >Index: editor/composer/src/nsComposerCommands.h >=================================================================== >+class nsIEditor; It's not clear to me why this needs to be added here; can it be removed? >Index: editor/composer/src/nsComposerCommandsUpdater.cpp >=================================================================== > * Contributor(s): >- * Simon Fraser <sfraser@netscape.com> >+ * Michael Judge <mjudge@netscape.com> >+ * Simon Fraser <sfraser@netscape.com> >+ * Charles Manske <cmanske@netscape.com> You should probably leave Simon as the first contributor in this list since he did the initial checkin. >+#include "nsIDOMWindow.h" > #include "nsComposerCommandsUpdater.h" > #include "nsIServiceManager.h" > #include "nsIDOMDocument.h" Do we still need all of these includes? In particular I'm wondering about nsIDOMDocument.h and others we may no longer be using in this file. >Index: editor/composer/src/nsComposerCommandsUpdater.h >=================================================================== delete this line: class nsIEditor; >Index: editor/composer/src/nsComposerController.cpp >=================================================================== >+nsresult nsComposerController::RegisterEditorDocStateCommands(nsIControllerCommandManager *inCommandManager) long line >+nsresult nsComposerController::RegisterHTMLEditorCommands(nsIControllerCommandManager *inCommandManager) long line; does it help to move nsresult to a separate line? >Index: editor/composer/src/nsComposerRegistration.cpp >=================================================================== >+ static PRBool htmlCommandsRegistered = PR_FALSE; Can we rename this so it's more easily recognized as static? sHTMLCommandsRegistered? >Index: editor/composer/src/nsEditingSession.cpp >=================================================================== >RCS file: /cvsroot/mozilla/editor/composer/src/nsEditingSession.cpp,v >retrieving revision 1.4 >diff -u -r1.4 nsEditingSession.cpp >--- editor/composer/src/nsEditingSession.cpp 9 Oct 2002 02:10:20 -0000 1.4 >+++ editor/composer/src/nsEditingSession.cpp 10 Nov 2002 21:32:38 -0000 >@@ -48,15 +48,15 @@ > > #include "nsIChannel.h" > #include "nsIWebProgress.h" >+#include "nsIWebNavigation.h" > > #include "nsIControllers.h" > #include "nsIController.h" >-#include "nsIEditorController.h" >+#include "nsIControllerContext.h" > > #include "nsIPresShell.h" > > #include "nsComposerCommandsUpdater.h" >- > #include "nsEditingSession.h" > > #include "nsIComponentManager.h" >@@ -67,9 +67,13 @@ > #include "nsISelectionController.h" > #include "nsIPlaintextEditor.h" > >+#include "nsIDOMNSDocument.h" >+ >+#include "nsEditorParserObserver.h" >+, mEditorStatus(eEditorOK) Do we really want to initialize to eEditorOK?? >+#define DEFAULT_TYPE "html" Could we clarify this define (default what?)? editor type? mime type? output type? >+// is this a MIME type that we support the editing of, in plain text mode? >+const char* const gSupportedTextTypes[] = { Did this list come from somewhere or did you just make it? If you copied it from somewhere, I think you should add a reference so others could (in theory) track down the person(s) who originally assembled the list (and bug numbers, etc). >+#ifndef FULL_EDITOR_HTML_SUPPORT >+static eHTMLTags gWatchTags[] = add a comment here explaining that the editor can't handle it now but hopefully will soon >+ nsAutoString mimeType; >+ nsCAutoString mimeCType; What's the difference between these two? Are they intended to be kept in synch with each other? >+ // Remove all the listeners >+ nsCOMPtr<nsISelection> selection; >+ editor->GetSelection(getter_AddRefs(selection)); >+ nsCOMPtr<nsISelectionPrivate> selPriv = do_QueryInterface(selection); >+ if (!selPriv) return NS_ERROR_FAILURE; tabs? >+ >+ nsCOMPtr<nsITransactionManager> txnMgr; spaces on the blank line >+ mCanCreateEditor = PR_TRUE; Are there places where we need to set this to false (error conditions where we've already set it to true but should set it back to false)? >+ to get deterimine if editor was created and document deterimine --> determine >+NS_IMETHODIMP >+nsPasteQuotationCommand::IsCommandEnabled(const char * aCommandName, >+ nsISupports *refCon, >+ PRBool *outCmdEnabled) >+{ >+ nsCOMPtr<nsIEditor> editor = do_QueryInterface(refCon); shouldn't we QI here for nsIEditorMailSupport too? >+ aParams->SetBooleanValue(STATE_ENABLED,enabled); add space after ',' >Index: editor/libeditor/text/nsPlaintextEditor.cpp >=================================================================== >+ // Remove event listeners. Note that if we had an HTML editor, >+ // it installed it's own instead of these it's --> its

commenst by brade: >Index: editor/composer/src/nsEditingSession.cpp >=================================================================== >+// is this a MIME type that we support the editing of, in plain text mode? >+const char* const gSupportedTextTypes[] = { Did this list come from somewhere or did you just make it? If you copied it from somewhere, I think you should add a reference so others could (in theory) track down the person(s) who originally assembled the list (and bug numbers, etc). cmanske: it's from the old sfraser nsEditorShell code -- I added a comment about this. We actually never did change the mimetype on the channel as we thought we were. >+ nsAutoString mimeType; >+ nsCAutoString mimeCType; What's the difference between these two? Are they intended to be kept in synch with each other? Yes. I moved the nsAutoString to just above where we need it to get mimetype from doc. The rest of the code uses the mimeCType conversion >+ mCanCreateEditor = PR_TRUE; Are there places where we need to set this to false (error conditions where we've already set it to true but should set it back to false)? cmanske: No. We don't pay attention to it later. I suppose it really means "mCanAttemtpToCreateEditor", but that seemed silly -------------------------------------------------------------------------------- Comments from sfraser: Index: embedding/components/commandhandler/public/nsIControllerContext.idl =================================================================== nsIControllerContext.idl needs some comments to explain the weird immutability thing with setting the nsIControllerCommandManager, and to explain the refCon. Index: embedding/components/commandhandler/src/nsBaseCommandController.cpp =================================================================== Need a comment in this file that explains the immutability stuff. + // Don't let users get the command manager if its + // immutable. they may harm it in some way + if (!mImmutableManager && mCommandManager && + aIID.Equals(NS_GET_IID(nsIControllerCommandManager))) + return mCommandManager->QueryInterface(aIID, result); I don't like this at all. I don't think the behaviour of GetInterface should be conditional on how it's created. I think the nsIControllerCommandManager itself should store the immutability flag, and it should return errors if you try to add/remove commands. cmanske: mjudge will address these issue. I will test any changes. Index: editor/composer/public/nsIEditingSession.idl =================================================================== When are we going to split this into public and private APIs? cmanske: soon after this landing!!! + /** + * Error codes when we fail to create an editor + * is placed in attribute editorStatus + */ + const long eEditorOK = 0; + const long eEditorCreationInProgress = 1; + const long eEditorErrorCantEditMimeType = 2; + const long eEditorErrorFileNotFound = 3; + const long eEditorErrorCantEditFramesets = 8; + const long eEditorErrorUnknown = 9; + + /** + * Status after editor creation and document loading + * Value is one of the above error codes + */ + readonly attribute unsigned long editorStatus; This stuff needs a comment to say that's it's temporary. cmanske: The general "editorStatus" isn't temporary, is it? It's true we won't always need eEditorErrorCantEditFramesets and in the future, eEditorErrorCantEditMimeType. + mEditorStatus = eEditorCreationInProgress; + mLoadBlankDocTimer->InitWithFuncCallback(nsEditingSession::TimerCallback, + (void*)docShell, + 10, nsITimer::TYPE_ONE_SHOT); How did you determine this timeout? Why does it need to happen on a timer anyway? cmanske: We definitely need a timer because we must unwind the current network loop before trying to load a different url. I'm not sure about the timeout; it could probably be pretty short -- user will always get a message dialog if there is an error. The 10ms seemed to work fine. Index: editor/idl/nsIEditorController.idl =================================================================== Isn't this file dead with this patch? cmanske: yes

I file bug 179585 to cleanup string usage in nsComposerCommands instead of using "const char*" for "mTagName". This bug is not the place to do all the required changes.

Comment on attachment 105769 [details] [diff] [review] Just build file changes -- All platforms v3 remove the duplicate file from MANIFEST_IDL r=brade

Comment on attachment 105771 [details] [diff] [review] Commands and Controllers rewrite v4 sorry I forgot to mark the bug yesterday; r=brade with the above fixes

The latest patch broke --enable-plaintext-editor-only. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1037158740.14839.gz

v4 patch (attachment 105771 [details] [diff] [review]): > PRBool > nsComposerCommandsUpdater::SelectionIsCollapsed() > { ... > nsresult rv; > // we don't care too much about failures here. > - nsCOMPtr<nsIEditor> editor = do_QueryInterface(mEditor, &rv); > if (NS_SUCCEEDED(rv)) ... This change means that you now *always* check NS_SUCCEEDED on *uninitialized* nsresult. P.S. This have added a warning on brad TBox: +editor/composer/src/nsComposerCommandsUpdater.cpp:362 + `nsresult rv' might be used uninitialized in this function See bug 59652 and bug 179819 for more on these warnings.

I just checked in the fix for the compiler warning mentioned in comment 41. -->fixed

This fixes the problem found in comment #20

Comment on attachment 106118 [details] [diff] [review] Fix for the Plaintext-only build This fix is now superceeded by the much better fix to bug 177333 that simplifies building text-only editor

verified via code inspection