Closed Bug 174439 Opened 22 years ago Closed 22 years ago

nsIEditorSession cleanup and new commands needed to support removal of editorShell

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.3alpha

People

(Reporter: cmanske, Assigned: cmanske)

References

Details

Attachments

(1 file, 5 obsolete files)

Various issues were discovered as part of removal of editorShell from 
Composer application. They will be explained further in the patch.
Trying to separate remaining "remove editorShell" work into manageable chunks.
This work convers:
1. Add new commands to observe document creation, destruction, and modification.
2. Cleanup nsIEdittingSession to not create 2 editors on startup and to 
communication with commandupdate correctly.
3. Cleanup nsICommandUpdater to use notify the observers on the new commands.
Status: NEW → ASSIGNED
Keywords: nsbeta1
Summary: nsIEditorSessiong cleanup needed to support removal of editorShell → nsIEditorSessiong cleanup and new commands needed to support removal of editorShell
Target Milestone: --- → mozilla1.2beta
Attached patch New commands v1 (obsolete) — Splinter Review
Whiteboard: [FIX IN HAND]need r=,sr=
The following block should be in UpdateCommandGroup:
  // This hardcoded list of commands in temporary. 
  // This code should use nsIControllerCommandGroup.
  if (aCommandGroup.Equals(NS_LITERAL_STRING("undo")))
  {
    commandUpdater->CommandStatusChanged("cmd_undo");
    commandUpdater->CommandStatusChanged("cmd_redo");
  }
  else if (aCommandGroup.Equals(NS_LITERAL_STRING("select")) ||
           aCommandGroup.Equals(NS_LITERAL_STRING("style")))
  {
    commandUpdater->CommandStatusChanged("cmd_bold");
    commandUpdater->CommandStatusChanged("cmd_italic");
    commandUpdater->CommandStatusChanged("cmd_underline");
    commandUpdater->CommandStatusChanged("cmd_tt");

    commandUpdater->CommandStatusChanged("cmd_strikethrough");
    commandUpdater->CommandStatusChanged("cmd_superscript");
    commandUpdater->CommandStatusChanged("cmd_subscript");
    commandUpdater->CommandStatusChanged("cmd_nobreak");

    commandUpdater->CommandStatusChanged("cmd_em");
    commandUpdater->CommandStatusChanged("cmd_strong");
    commandUpdater->CommandStatusChanged("cmd_cite");
    commandUpdater->CommandStatusChanged("cmd_abbr");
    commandUpdater->CommandStatusChanged("cmd_acronym");
    commandUpdater->CommandStatusChanged("cmd_code");
    commandUpdater->CommandStatusChanged("cmd_samp");
    commandUpdater->CommandStatusChanged("cmd_var");
   
    commandUpdater->CommandStatusChanged("cmd_increaseFont");
    commandUpdater->CommandStatusChanged("cmd_decreaseFont");

    commandUpdater->CommandStatusChanged("cmd_paragraphState");
    commandUpdater->CommandStatusChanged("cmd_fontFace");
    commandUpdater->CommandStatusChanged("cmd_fontColor");
    commandUpdater->CommandStatusChanged("cmd_backgroundColor");
    commandUpdater->CommandStatusChanged("cmd_highlight");
  }  
  else if (aCommandGroup.Equals(NS_LITERAL_STRING("save")))
  {
    // save commands (none in C++)
  }
Attached patch New commands v2 (obsolete) — Splinter Review
Improvements per reviewer comments
Attachment #102948 - Attachment is obsolete: true
OS: Windows 2000 → All
Summary: nsIEditorSessiong cleanup and new commands needed to support removal of editorShell → nsIEditorSession cleanup and new commands needed to support removal of editorShell
Comment on attachment 103054 [details] [diff] [review]
New commands v2

This should begin with upper case as the others do:
+   1. get the ...

Shorten the lines (with comments) in step 2 (ideally less than 80 columns)

remove the tab or extra spaces from this line:
+  nsCOMPtr<nsICommandManager>	 commandManager ...

when refCon doesn't QI to the editor type we need, I prefer this error: 
NS_ERROR_INVALID_ARG

nsDocumentModified::GetCommandStateParams seems different from other commands
in that it doesn't QI for editor right away but instead sets aParams.  Should
it be the other way?

This seems wrong:
-  CallUpdateCommands(NS_LITERAL_STRING("undo"));
+  UpdateOneCommand("undo");
It seems like it should be calling UpdateCommandGroup instead of
UpdateOneCommand (3 places)

I'm not sure if this is correct or not:
+  UpdateOneCommand("obs_document_created");

This seems wrong:
-    CallUpdateCommands(NS_LITERAL_STRING("save"));
-
+    UpdateOneCommand("cmd_document_modified");
Why aren't we calling UpdateCommandgroup for "save" commands?

remove the extra spaces or tabs from these lines:
+  nsCOMPtr<nsIDocShell>	docShell;
+	  mDocShell = docShell.get();		
+  nsCOMPtr<nsICommandManager>		commandManager =
do_GetInterface(mDocShell);

Fix this comment to line up with the comment below it (and remove the space at
the end of the line):
+ // This hardcoded list of commands in temporary. 

Do we really want the param to be a char*?
+  nsresult	 UpdateOneCommand(const char* aCommand);

I think I'd prefer to see this block formatted differently:
+  nsresult rv = NS_ERROR_FAILURE;

+  nsCOMPtr<nsIEditorController>
editorController(do_QueryInterface(controller));    // ok with nil controller
+  if (editorController)
+    rv = editorController->SetCommandRefCon(inEditor);

as:
+  nsCOMPtr<nsIEditorController>
editorController(do_QueryInterface(controller));    // ok with nil controller
+  if (!editorController) return NS_ERROR_FAILURE;
+
+  return editorController->SetCommandRefCon(inEditor);

I'd like to see this changed too:
+  if (mBaseCommandControllerId >= 0)
shouldn't it really be != -1?
and if so, shouldn't we #define something for that to make it more obvious?
(same with the html command controller id)
  #define kUndefinedControllerID -1

it'd be nice if you could  add a blank line above this comment and expand on
the "weirdness":
+  // Used to prevent double creation of editor because of
nsIWebProgressListener weirdness

are the EditorShell changes needed here?
Attachment #103054 - Flags: needs-work+
Attached patch New commands v3 (obsolete) — Splinter Review
All comments by Brade were addressed.
Re:
  Do we really want the param to be a char*?
  +  nsresult	 UpdateOneCommand(const char* aCommand);
commandUpdater->CommandStatusChanged uses "char*" so I did as well to avoid
extra string class manipulation.
Attachment #103054 - Attachment is obsolete: true
I forgot to mention that nsEditingSession.cpp also contains the changes by
mjudge for bug 170353. It was impossible to separate and it's important that my 
patch works with that new controller code anyway (it does.)
nsComposerCommands.cpp formatting: any chance you could keep the lines
(especially those function definitions) under 80 columns?

Kathy asked:
I'm not sure if this is correct or not:
+  UpdateOneCommand("obs_document_created");

It's still UpdateOneCommand -- should it be?

+  nsCOMPtr<nsIDocShell> docShell;
+  rv = scriptGlobalObject->GetDocShell(getter_AddRefs(docShell));
+  if (docShell)
+	  mDocShell = docShell.get();		

The last line still has lots of extra spaces at the end of the line.

+  // This hardcoded list of commands in temporary.

You fixed the indentation and spacing, but should that be "is temporary"?

+  if (docShell)
+	  mDocShell = docShell.get();		

Please remove the tab in that second line.
I know it's not new in this patch, but is there any chance we might need
a weak reference on the mDocShell?

+#ifdef NOISY_DOC_LOADING
+{
[ ... ]
+}
+#endif

I assume the zero-indentation on this and similar blocks is intentional?
(All right with me if it is, just looked strange.)
Attached patch New commands v3 (obsolete) — Splinter Review
Removed more tabs and spaces
Methods in nsComposerCommandUpdater.cpp reformated to be under 80 chars/line.
"UpdateOneCommand" was approved by Brade and SFraser.
Attachment #103238 - Attachment is obsolete: true
Comment on attachment 103387 [details] [diff] [review]
New commands v3

Two lines were reformatted but are still over 80 columns:

+		    nsIDocumentStateListener, nsITransactionListener,
nsITimerCallback);

+				   nsITransaction *aTransaction, PRBool
*aInterrupt)

No need to make a new patch for that, though. Looks good to me, r=akkana
Attachment #103387 - Flags: review+
Whiteboard: [FIX IN HAND]need r=,sr= → [FIX IN HAND]need sr=
-    return NS_ERROR_NOT_IMPLEMENTED;
+    return NS_ERROR_INVALID_ARG;

(various places).

The idea behind returning NS_ERROR_NOT_IMPLEMENTED is that you're calling an
HTML command on a non-HTML editor, so returning NS_ERROR_NOT_IMPLEMENTED makes
sense (and is more indicative of the cause of the error than
NS_ERROR_INVALID_ARG), taken in the context of the consumer, rather than just
the current method.

You created new commands nsDocumentCreatedCommand, and nsDocumentWillBeDestroyed
that are dummy 'state notification' commands (which is a good thing). I think
IsCommandEnabled() on these should always return PR_FALSE, since you never want
someone to call DoCommand() on one. Also, GetCommandStateParams() should
probably just return an error for them.

You could also have a single C++ class for these two commands, and just register
it under 2 different names. It's just a placeholder.

nsDocumentModified should be renamed nsSetDocumentStateCommand, I think.

+NS_IMETHODIMP
+nsDocumentModified::DoCommand(const char *aCommandName, nsISupports *refCon)
+{
+  return NS_OK;
+}

This should return an error.

+  {
+    // save commands (most are not in C++)
+    commandUpdater->CommandStatusChanged("cmd_document_modified");
+  }

Even if  they are not in C++, we still need to dirty the commands (since these
notifications propagate out to JS).


+#define kUndefinedControllerID -1

+, mBaseCommandControllerId(kUndefinedControllerID)
+, mHTMLCommandControllerId(kUndefinedControllerID)

+  PRUint32        mBaseCommandControllerId;

Won't the compiler warn about initting unsigned variables with -1? Is this intended?

Rest of the patch looks good, but I think I need to see a new patch.
Attached patch New commands v4 (obsolete) — Splinter Review
Fixed issues addressed by Simon:
1. A single command class, nsDocumentStateCommand, was implemented to use with 

"obs_documentCreated" and "obs_documentWillBeDestroyed". This class should be
used
for state changes the can't be set, just observed. DoCommand() and 
GetCommandStateParams() should not be used (returns error)
2. A single command class was, nsSetDocumentStateCommand, was implement to use
with state info that can be set by editor embeddors. The getting and setting
should check the aCommand param (we'll change to a switch statement once other
state commands are implemented.)
3. nsXULControllers::AppendController was modified to assign controll ID
numbers starting at 1 so we can avoid the -1 for kUndefinedControllerID
Attachment #103387 - Attachment is obsolete: true
*** Bug 79875 has been marked as a duplicate of this bug. ***
Comment on attachment 103735 [details] [diff] [review]
New commands v4

the state_doc_modified could be using a more generic state_ name since it
willprobably not be coupled with any other command. talked to charlie about
this. r=mjudge based on what we talked about
Attachment #103735 - Flags: review+
In looking over everything that happens during nsIWebProgressListener callbacks
in nsEditorshell.cpp, I noticed that we did not have the disable JavaScript
code in nsIEditingSession. This should happen during the "StartDocument()" 
call (when network aState = STATE_START & STATE_DOCUMENT). This is the patch
fragments to add the same code in the same place in nsIEditingSession:

@@ -65,11 +65,13 @@
 
 #include "nsIContentViewer.h"
 #include "nsISelectionController.h"
 #include "nsIPlaintextEditor.h"
-
+#include "nsIScriptGlobalObject.h"
+#include "nsIScriptContext.h"
 
@@ -483,13 +495,27 @@
 
   if (domWindow)
   {
     nsresult rv = TearDownEditorOnWindow(domWindow);
+    if (NS_FAILED(rv)) return rv;
+
+    // Disable JavaScript in any editor window
+    nsCOMPtr<nsIScriptGlobalObject> sgo (do_QueryInterface(domWindow));
+    if (sgo)
+    {
+      nsCOMPtr<nsIScriptContext> scriptContext;
+      sgo->GetContext(getter_AddRefs(scriptContext));
+      if (scriptContext)
+        scriptContext->SetScriptsEnabled(PR_FALSE, PR_TRUE);
+    }
   }
     
   return NS_OK;
 }
 
Is it ok if I include that code in this fix?
That last attachment looks just like the previous one. Is it the correct patch?
Attached patch New commands v5Splinter Review
Foo. My script failed me :(
This is the correct patch.
Note that it includes the "disable JS" code as I discussed in comment #15,
as well a few lines from a patch by mjudge (do_QueryInterface pattern changes;
he will checkin that controller patch before I checkin this.)
Attachment #103735 - Attachment is obsolete: true
Comment on attachment 103871 [details] [diff] [review]
New commands v5

+NS_IMETHODIMP
+nsSetDocumentStateCommand::DoCommand(const char *aCommandName, nsISupports
*refCon)
+{
+  return NS_OK; // or NS_ERROR_NOT_IMPLEMENTED ?
+}

This should return NS_ERROR_NOT_IMPLEMENTED.

+  if (aCommandName == "cmd_documentModified")

I think we should call this cmd_setDocumentModified. Then it's move obvious
when it would be disbaled (if the doc is already modified).

+    // Disable JavaScript in any editor window

I don't really think that it's nsEditingSessions's job to do this (since it is
supposed to be content agnostic). We should fix that later.

Fix those and sr=sfraser
Attachment #103871 - Flags: superreview+
This should return NS_ERROR_NOT_IMPLEMENTED.
  Ok.
-----
+  if (aCommandName == "cmd_documentModified")

I think we should call this cmd_setDocumentModified. Then it's move obvious
when it would be disbaled (if the doc is already modified).

   But the command isn't disabled if doc is currently modified.
   doCommand can be called with state_attribute=false to reset the 
   document's dirty state. That feature is required because a caller may
   setup some styles or load a template as part of document initialization
   using editor commands and they need to be able to clear the dirty flag.
-----
+    // Disable JavaScript in any editor window

I don't really think that it's nsEditingSessions's job to do this (since it is
supposed to be content agnostic). We should fix that later.

   Ok, but then who is going to do this? I think this is an important 
   security issue, not to mention that it is a real pain to do in JS!
Comment on attachment 103871 [details] [diff] [review]
New commands v5

r=mjudge
Attachment #103871 - Flags: review+
Concerning "cmd_documentModified", I guess what I really mean to say is that
included "set" seems superfluous -- lots of other commands could use that
pattern, e.g., "cmd_setBold", but the fact that you use the command to set or get 
state is implied. 
After all, you get the documentModified state by calling 
GetCommandParams("cmd_documentModified"...

Whiteboard: [FIX IN HAND]need sr= → [FIX IN HAND]need a=
Target Milestone: mozilla1.2beta → mozilla1.2final
The reason I object to "cmd_documentModified" is that it doesn't imply action.
"cmd_bold" implies "make bold" but "cmd_documentModified" is more descriptive.

How about "cmd_setDocumentModificationState" or "cmd_setDocumentDirtyState"?
I disagree. "cmd_bold" doesn't seem to imply "make bold" any more or less than
cmd_documentModified imples "set documentModified"
*** Bug 156756 has been marked as a duplicate of this bug. ***
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND]need a=
verified by code inspection
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.2final → mozilla1.3alpha
*** Bug 178560 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: