Closed Bug 170353 Opened 22 years ago Closed 22 years ago

embedding: factor nsIEditorController / nsComposerController code

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: cmanske, Assigned: mjudge)

References

Details

(Keywords: embed)

Attachments

(19 obsolete files)

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.
Status: NEW → ASSIGNED
Depends on: edembed
Target Milestone: --- → mozilla1.2beta
I believe this is for factoring the controllers; I hope the summary change is ok
OS: Windows 2000 → All
Hardware: PC → All
Summary: Reorganize nsIEditorController / nsComposerController code → embed: factor nsIEditorController / nsComposerController code
Summary: embed: factor nsIEditorController / nsComposerController code → embedding: factor nsIEditorController / nsComposerController code
Mike is working on this now.
Assignee: cmanske → mjudge
Status: ASSIGNED → NEW
Keywords: embed, nsbeta1
Attached patch patch from /mozilla (obsolete) — Splinter Review
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
Attached patch patch from /mozilla (obsolete) — Splinter Review
unified patch with all code in it..
Attachment #103064 - Attachment is obsolete: true
Attachment #103065 - Attachment is obsolete: true
Attachment #103066 - Attachment is obsolete: true
Attachment #103067 - Attachment is obsolete: true
Attachment #103068 - Attachment is obsolete: true
Comment on attachment 103075 [details] [diff] [review]
patch from /mozilla

comments sent to mjudge via irc
Attachment #103075 - Flags: needs-work+
Attached patch patch from /mozilla (obsolete) — Splinter Review
fix handles the issues brade specified
Attachment #103075 - Attachment is obsolete: true
Attached patch patch from /mozilla (obsolete) — Splinter Review
ok this fixes the left out changes to Makefile.in... oops.
Attachment #103094 - Attachment is obsolete: true
Attached patch patch from /mozilla (obsolete) — Splinter Review
forgot the layout makefile.in. also a _content in the .js file. all good now
Attachment #103117 - Attachment is obsolete: true
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.
Attachment #103127 - Flags: needs-work+
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.
Attached patch /mozilla patch (obsolete) — Splinter Review
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.
Attachment #103127 - Attachment is obsolete: true
Comment on attachment 103398 [details] [diff] [review]
/mozilla patch

r=cmanske
Attachment #103398 - Flags: review+
+// 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.
Attachment #103398 - Flags: needs-work+
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.
Attached patch dif from /mozilla (obsolete) — Splinter Review
patch has all changes as well as the changes for mime.
Attachment #103398 - Attachment is obsolete: true
Attached patch /mozilla (obsolete) — Splinter Review
added 2 files from /content. removed files from the event code for the
nsHRFrame.cpp
Attachment #104547 - Attachment is obsolete: true
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.
Attachment #104564 - Attachment is obsolete: true
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.
Attachment #105595 - Attachment is obsolete: true
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
Attachment #105595 - Attachment is obsolete: false
Added missing entries for nsBaseCommandController.cpp
Attachment #105595 - Attachment is obsolete: true
Attachment #105605 - Attachment is obsolete: true
This includes all build files in embedding and editor dirs.
It includes removing editorShell from editor build files.
Attachment #105722 - Attachment is obsolete: true
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
Attachment #105642 - Attachment is obsolete: true
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
Attachment #105769 - Flags: superreview+
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.
Attachment #105771 - Flags: superreview+
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
Attachment #105769 - Flags: review+
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
Attachment #105771 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Attached patch Fix for the Plaintext-only build (obsolete) — Splinter Review
This fixes the problem found in comment #20
Attachment #105769 - Attachment is obsolete: true
Attachment #105771 - Attachment is obsolete: true
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
Attachment #106118 - Attachment is obsolete: true
verified via code inspection
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: