169029 - Remove editorshell from Composer window files

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Charles Manske]

Use this bug for changes needed in nsEditorShell.cpp, editor.js,
ComposerCommands.js etc. (The Composer Main window code) to remove editorShell.
Bug 158881 tracks this work in Composer dialogs.

Comment 1

[Charles Manske]

Attachment: patch v1

This patch adds member variables that currently exist in nsIEditorShell to
nsIEditor or nsIHTMLEditor interfaces as replacements:
1. contentsMIMEType
2. HTMLSourceMode is replaced by nsIHTMLEditor::canEditHTML
3. documentEditable is replaced by nsIEditor::canEditDocument
Other changes:
Composer controller code revisions to:
1. Use IDs to identify specific controllers instead of grovelling for 
controllers based on query interface.
2. Replace GetEditorController() with GetComposerCommandManager(), since that
is what it really does!
3. Add "unregisterCommand()" as a temporary suggestion to avoid exception
errors caused by duplication of composer command registration on the 
nsComposerControllers added to both the content window and the XUL window.

Comment 2

[Charles Manske]

Attachment: patch v2

Corrections from review. Also used "k" prefix instead of "g" for const

Comment 3

[Kathleen :Brade]

Comment on attachment 99527 [details] [diff] [review]
patch v2

r=brade
but I find "canEditDocument" particularly confusing and hope it will be changed
to something else (as discussed on irc)

Comment 4

[Charles Manske]

Attachment: patch v3

The "unregisterCommand()" has an exellent side effect: By not having the HTML
formating commands on the top XUL window (but only on the content window where
they should be), "canEditHTML" is not needed at all. When in HTML Source mode,
the HTML format commands are not accessible because that is not the focussed 
window.

Comment 5

[Kathleen :Brade]

Comment on attachment 99548 [details] [diff] [review]
patch v3

do we still need the changes in nsComposerCommands.cpp?  I don't think they
need to be checked in.

remove the PR_FALSE assignment in nsEditor::GetIsDocumentEditable ; it's not
needed

r=brade with the above

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 99548 [details] [diff] [review]
patch v3

>+    var controller = window._content.controllers.getControllerAt(1);

Is there anywhere other than _content this could be stored? What
if this object was hacked by the content itself?

http://grey/u/mstoltz/techtalk/slide17.xml

Or is this _content the page being edited and you can guarantee
scripts are never able to run in that context?

Comment 7

[Charles Manske]

Yes, "_content"is the content window being edited and JS is completely turned off
in the editor.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 99548 [details] [diff] [review]
patch v3

sr=dveditz

Comment 9

[Charles Manske]

"patch v3" checked in (but not nsComposerCommands.cpp, as requested)

Comment 10

[Charles Manske]

Attachment: Convert Get/SetDocumentTitle methods v1

This converts the Get/SetDocumentTitle methods in nsIEditorShell to JS
utilities.
This also allowed eliminating the "doAfterSave()" method in nsEditorShell.
While I was adjusting to those changes in ComposerCommands.js, I cleanup up
some variable names and used "GetCurrentEditor()" instead of "gEditor" in 
regions of the code I was touching. There are many more of those to fix, but
leaving that for another patch to make it easier to review.
Note that these changes will have to be checked in at the same time as 
patches to the Page Properties, Save As Charset, and Publishing dialogs in
bug 158881 (those dialogs used "editorShell.Get/SetDocumentTitle()" .)

Comment 11

[Charles Manske]

Attachment: Convert Get/SetDocumentTitle methods v2

Fixed some problems with v1. Decided to use "destinationLocation" and 
"relatedFilesLocation" since it can be either an nsIURI or nsILocalFile object.

Comment 12

[Kathleen :Brade]

Comment on attachment 100308 [details] [diff] [review]
Convert Get/SetDocumentTitle methods v2

I know this isn't a regression from the existing code but this seems wrong to
me:
+    if (window.newTitle != null)
+      SetDocumentTitle(window.newTitle);

If the document has a title, and then I delete the title in this dialog, won't
I be left with the original dialog rather than having "" as a title?

You need to fix this commented line:
 //	 window.editorShell.editor.resetModificationCount();


This line is wrong:
+  var editorType = window. .editorType;

I think it's confusing to rename "parentDir" but not curParentString; can we
just save that variable renaming for another patch?

Comment 13

[Charles Manske]

Attachment: Convert Get/SetDocumentTitle methods v3

Fixed items requested by reviewer.
I changed "curParentString" to "recentFilesLocationStr" and would like to
include
that change. (next patch on this file is going to be very big!)
Concerning setting of title to "untitled" when title is empty: this is exactly
the behavior we have now, so there's nothing new there.

Comment 14

[Charles Manske]

Note that  "v3" patch includes some of the fix to bug 107522, which will have to
be checked in at the same time as this.

Comment 15

[Akkana Peck]

I don't understand this change:

> -    BuildRecentMenu(true);
> +    BuildRecentMenu();

But the definition of BuildRecentMenu still takes a SavePrefs argument.

I'm okay with the "untitled" thing in the publish dialog (Kathy asked about
that) as long as we also show the filename; it may help encourage users to
notice that documents have titles.

Comment 16

[Kathleen :Brade]

Comment on attachment 100627 [details] [diff] [review]
Convert Get/SetDocumentTitle methods v3

I don't like "relatedFilesLocation" because it doesn't indicate that it is a
directory.  If "parentDir" is unacceptable, how about "relatedFilesDir"?  I'd
also prefer "relatedFilesDirStr" over relatedFilesLocationStr.	(I always think
of "parentDir" as the parent directory for the related files, that's where the
name comes from)

This comment should reflect what the actual code should call; currently it is
missing an "if":
 // Don't forget to do these things after calling OutputFileWithPersistAPI:
-//    window.editorShell.doAfterSave(doUpdateURLOnDocument, urlstring);  // we
need to update the url before notifying listeners
+//    SetDocumentURL(newUrlspec);
+//    UpdateWindowTitle();

This comment needs to be put somewhere (you are removing it but it is important
to leave in):
// we need to update the url before notifying listeners

probably in this block:
+	     // Get the new docUri from the "browse location" in case "publish
location" was FTP
+	     SetDocumentURL(GetDocUrlFromPublishData(gPublishData));
+	     UpdateWindowTitle();

The same goes for this change also:
-      window.editorShell.doAfterSave(doUpdateURL, urlstring);	// we need to
update the url before notifying listeners
+      if (doUpdateURL)
+	 SetDocumentURL(urlstring);
+      UpdateWindowTitle();


Also, in the above block, it's not clear to me that we should always be calling
UpdateWindowTitle.  If you look at the C++ version that is being removed,
you'll see a comment in there explaining that.	I don't think we want to lose
that comment.


SetDocumentURL should be named SetDocumentURI and its parameter should be a uri
(which we already have in some cases).

Don't we have a utility function for creating a uri?  Can't we use that in the
one or two places where we don't already have a uri?


Do GetDocumentTitle and SetDocumentTitle really belong in EditorUtilities.js?

Comment 17

[Charles Manske]

Re: Akkana's comment #15: The editor.js changes in the patch don't reflect the 
changes to fix bug 107522 (I wasn't sure if I should include that fix here; I
guess I should have!) The SavePrefs param is removed in the final code.

Re: Brade's comment #16
1. I'll use "relatedFilesDirStr"
2. I'll fix the comments as requested.
3. about "it's not clear to me that we should always be calling
UpdateWindowTitle.":
I'll copy the comment from nsEditorShell.cpp. Here is the issue in the last part
of that comment: After changing the title from Page Properties dialog and then
using undo, the previous title shows up in the window caption, but the 
[file:/...] appendage isn't updated. That is why I favor calling 
"UpdateWindowTitle()" with every save -- that will rebuild the full window 
caption string, including the filename.

4. I *did* make SetDocumentUri() with nsIURI param for the first version, but it
didn't work for local files. But the 2 "location" params in 
nsIWebBrowserPersist::saveDocument() are nsISupports* because they can be either
nsIURI* or nsILocalFile*. But we always need nsIURI* for the docshell. So the 
simplest solution was to use the url strings we already have and build a new 
nsIURI object.
Another solution would be to do SetDocumentUri() and do a QI to test if it's
nsILocalFile, and build a new nsIURI only in that case. But that causes us to
add more code to get nsIFileProtocolHanlder and use getURLSpecFromFile 
to get the url string we already have. In the context of bug 168788, concerning
unescaping of file urls, this could be even more complicated.

5. We don't have a utility to create a new URI. Since it's essentially:
GetIOService().newURI(urlspec, GetCurrentEditor().documentCharacterSet, null);
I'm not sure if we really need one. In most contexts used, we already have 
a the nsIIOServices object.

6. GetDocumentTitle() and SetDocumentTitle() are used by both main window and
dialog JS; that is the criterion to put it in editorUtilities.js.

Comment 18

[Charles Manske]

Attachment: Convert Get/SetDocumentTitle methods v4

Changed per reviewer comments. "SetDocumentURI()" method implemented instead of

"SetDocumentURL()."

Comment 19

[Charles Manske]

Attachment: Convert Get/SetDocumentTitle methods v4

Same as v4 except for removal of a try/catch that wasn't needed.
Has all changes requested by reviewer.
This contains all of the fix for bug 170522 since they are intermingled.

Comment 20

[Kathleen :Brade]

Comment on attachment 100748 [details] [diff] [review]
Convert Get/SetDocumentTitle methods v4

ComposerCommands.js does not appear to be up-to-date; you may have a conflict
from Aaron in there (around line 122).

This code seems wrong to me:
+    if (window.newTitle != null)
+      SetDocumentTitle(window.newTitle);

in this situation:  document has title, save as charset, user remove existing
title
The new title doesn't get set so the original title remains.  Can't we always
just call SetDocumentTitle or do the other compare I've seen in other places: 
if (oldTitle != newTitle) ...
I see a similar problem when prompting for the document title.


for this line can't we remove the editorShell?
+  if (!aMimeType || aMimeType == "" || !editor || !window.editorShell)


In editor.js, in the DocumentStateListener, why don't we need to save the JS
prefs there?  Do we not have the uri being opened yet?	How would the file just
opened get recorded in prefs if I never saved it?


I still don't like this for the window title since I think it is confusing but
I won't let that block this patch:
  untitled [file:/.../foo.html] - Composer

Comment 21

[Charles Manske]

Attachment: Convert Get/SetDocumentTitle methods v5

Addressing last set of comments by reviewer:
1. Used same pattern of saving "oldTitle" and setting only if changed for the 
SaveAsCharset dialog.
2. Fixed not saving an empty title when prompting for title during file save.
3. I'll remove "!window.editorShell" when I remove all instances of 
"editorShell" in next round of fixes.
4. In editor.js, we call UpdateWindowTitle()" in the DocumentStateListener code

and this always calls "SaveRecentFilesPrefs()"
5. I simplified GetSuggestedFileName(), PromptForSaveLocation(), and 
PromptAndSetTitleIfNone() to not require the "aHTMLDoc" param since all it was
used for is getting the current document title. This eliminated redundant code
by using new "GetDocumentTitle()" method.

Comment 22

[Kathleen :Brade]

Comment on attachment 100781 [details] [diff] [review]
Convert Get/SetDocumentTitle methods v5

I think this file uses this format:
+function SaveRecentFilesPrefs()
+{

Also in this function, move these lines closer to the top:
+  // Nothing will change, so don't bother doing the work
+  if(IsUrlAboutBlank(curUrl))
+    return;
Does the above check need to be for data urls also?

r=brade with the above fixes

Comment 23

[Alec Flett]

Comment on attachment 100781 [details] [diff] [review]
Convert Get/SetDocumentTitle methods v5

ugh, that lookupMethod stuff is pretty nasty. 

ah well. 
sr=alecf

Comment 24

[Charles Manske]

Comment on attachment 100781 [details] [diff] [review]
Convert Get/SetDocumentTitle methods v5

This patch was checked into mozilla 1.2beta trunk

Comment 25

[Charles Manske]

Getting very close! I have already tested creating an editor without creating
editorShell and I can type and do simple text formatting.
I'm now working out various problems and side effects.

Comment 26

[Charles Manske]

Attachment: Remove editorShell from ComposerCommands.js v1

also removes editorShell in a few other places.
This uses editorShell ONLY for loading a URL and printing.

Comment 27

[Charles Manske]

Oops. I also left in changes for "editorShell.editorType". If this is fixed 
as well today, I'll update that to use new method of getting editorType,
else I'll leave the old editorShell code.

Comment 28

[Kathleen :Brade]

Comment on attachment 102198 [details] [diff] [review]
Remove editorShell from ComposerCommands.js v1

There are problems with having the name change; be sure to use the correct name
here:
+      var xulWin = document.getElementById("WebComposerWindow");

Should this be a call to EditorSetTextProperty instead?
+      editor.setTextProperty(tagName, "", "");

Should this line use "GetCurrentEditor":
+      editor.insertHTML("<br clear='all'>");

This line is the wrong case for beginTransaction (2 places):
+      editor.BeginTransaction();

Do we want to use GetCurrentEditor here (instaed of gEditor):
+	 var element = gEditor.getSelectedElement("");

r=brade with the above changes

Comment 29

[kinmoz]

Comment on attachment 102198 [details] [diff] [review]
Remove editorShell from ComposerCommands.js v1

sr=[e-mail] with the following addressed, as well as brade's concerns.

==== Why is this "TextTypes" instead of "MimeTypes"?


+const kSupportedTextTypes =
+[
+  "text/plain",
+  "text/css",


==== Same question for the function name that uses it?


+function IsSupportedTextType(aMimeType)


==== Don't forget to remove references to "WebComposerWindow" before checkin so
we don't collide with Neil's checkin that changed things back to
"editorWindow".


==== I actually like the comparison against null since you don't know if the
caller is actually looking for specific "true" or "false" values. There are
actually a couple of |isCommandEnabled()| functions that do this:


   isCommandEnabled: function(aCommand, dummy)
   {
-    return (window.editorShell != null);
+    return (GetCurrentEditor());
   },


==== Should be lower case "begin" in a couple of places:

+      editor.BeginTransaction();


==== Shouldn't this be |GetCurrentEditor().joinTableCells(false);|


@@ -3769,7 +3821,9 @@
   doCommand: function(aCommand)
   {
     // Param: Don't merge non-contiguous cells
-    window.editorShell.JoinTableCells(false);
+    try {
+      editor.joinTableCells(false);
+    } catch (e) {}
     window._content.focus();
   }
 };



==== Same here:

   doCommand: function(aCommand)
   {
-    window.editorShell.SplitTableCell();
+    try {
+      editor.splitTableCell();
+    } catch (e) {}
     window._content.focus();
   }
 };


==== |editor| isn't defined before it's used here:


@@ -3903,31 +3963,34 @@
 {
   isCommandEnabled: function(aCommand, dummy)
   {
-    if (window.editorShell && window.editorShell.documentEditable &&
IsEditingRenderedHTML())
+    if (IsDocumentEditable() && IsEditingRenderedHTML())
     {
-    var selection = window.editorShell.editorSelection;
+      var selection;
+      try {
+	 selection = editor.selection;
+      } catch (e) {}


==== After you define it above, you may want to use |editor| here instead of
|gEditor|:


+	 var element = gEditor.getSelectedElement("");

Comment 30

[Charles Manske]

Comment on attachment 102198 [details] [diff] [review]
Remove editorShell from ComposerCommands.js v1

checked into 1.2 trunk

Comment 31

[Charles Manske]

Attachment: Fix document state listeners v1

Work in progress, but given the amount of change, I want to get some feedback.
This uses new "observer" commands (convention is to prefix with "obs_")
whose purpose is to notify the UI when document is created, modified, or about
to be destroyed.
Some cleanup in nsEditingSession and nsComposerCommandUpdater also done.

Comment 32

[Charles Manske]

Attachment: Final editor shell removal and cleanup v1

This patch:
1. removes remaining editorShell references
2. Converts globals such as gEditor to use the 'current editor',
editingSession,
commandManager, etc. from the main content tag (the <editor> element). 
This facilitates any future changes that support multiple editors per Composer
app or when editing a frameset. Utility methods GetCurrentEditor(), 
GetCurrentEditorElement(), GetCurrentCommandManager(), and
GetCurrentEditorType()
should always be used to obtain those objects. To test for various state info, 

these utilities should be used: IsHTMLEditor(), PageIsEmptyAndUntouched(), 
IsInHTMLSourceMode(), IsEditingRenderedHTML(), IsWebComposer(), 
IsDocumentEditable(), IsDocumentEmpty(), IsDocumentModified(), and 
IsHTMLEditor(). [For consistency, I started all methods with "Is", so this
required a simple text change in ColorPicker and InsertTable dialog code.]
3. OnDocumentCreation now works using the new "obs_document_creation" command
and observer mechanism (new command is in patch for bug 174439.)

Comment 33

[Charles Manske]

Attachment: Final editor shell removal and cleanup v2

Changes per review, including:
GetCurrentEditor() is QI'd to nsIEditor, nsIPlaintextEditor, and nsIHTMLEditor.

For table editing, use GetCurrentTableEditor().

Comment 34

[Charles Manske]

Attachment: Final editor shell removal and cleanup v3

Minor changes relative to v2. Leverage "instanceof" to avoid having to use
try/catch around frequently-called QueryInterface calls.

Comment 35

[Akkana Peck]

Comment on attachment 103234 [details] [diff] [review]
Final editor shell removal and cleanup v3

This has a lot of long lines (over 80 columns), mostly comments, which makes it
harder to read (especially when comparing diffs).  I won't insist on their all
being fixed, but I'd sure appreciate any that you did fix.  Especially with
comments where it's easy.

I don't understand the capitalization of isHTMLEditor to IsHTMLEditor -- aren't
JS functions supposed to start with lowercase?	Though I guess that's more
consistent with other local functions like IsDocumentEditable().

Re GetCurrentEditor QI'ing (or actually doing instanceof) to other editor
types: are we sure that isn't going to be a performance hit?  Maybe we should
also have a GetComplexEditor or something ...

Removing references to editorOverlay.js: don't forget to remove the file once
you're sure nobody else is referring to it!  I guess mail or other customers
might still need it now?

+	 // Just for convenience
+	 gContentWindow = window._content;

Are we going to have to get rid of gContentWindow later?  If it's going away,
this might be a good time to add a comment saying that.

In TextEditorAppShell you add editortype= to the <editor> tag, but you don't
remove src="about:blank".  But in editor.xul, you add editortype and remove
src.  Why the difference?  They should probably be made consistent: either
remove src= for both, or for neither (unless there's a reason for it).

Fix those issues and r=akkana.

Comment 36

[Simon Fraser [no longer active]]

Comment on attachment 103234 [details] [diff] [review]
Final editor shell removal and cleanup v3

sr=sfraser

Comment 37

[Charles Manske]

*** Bug 169530 has been marked as a duplicate of this bug. ***

Comment 38

[Charles Manske]

Attachment: Final purging of editorShell and cvs remove of files

This also includes fix to dom inspector to remove editorshell

Comment 39

[Charles Manske]

Attachment: Remove nsInterfaceState from build and cvs remove

nsEditorShell was the only user of this class, so it dies too.

Comment 40

[Akkana Peck]

Comment on attachment 106144 [details] [diff] [review]
Final purging of editorShell and cvs remove of files

Woohoo!

Comment 41

[[e-mail]]

> Leverage "instanceof" to avoid having to use
> try/catch around frequently-called QueryInterface calls.

try {
  var editorElement = GetCurrentEditorElement();
  editor = editorElement.getEditor(editorElement.contentWindow);

  // Do QIs now so editor users won't have to figure out which interface to use
  // Using "instanceof" does the QI for us.
  editor instanceof Components.interfaces.nsIPlaintextEditor;
  editor instanceof Components.interfaces.nsIHTMLEditor;
} catch (e) { dump (e)+"\n"; }

Well, the try/catch is still there, so QueryInterface would work just as well.
Also, don't you have consts for nsI...Editor

function GetCurrentTableEditor()
{
  var editor = GetCurrentEditor();
  return (editor && (editor instanceof nsITableEditor)) ? editor : null;
}

As it happens, !editor implies !(editor instanceof nsITableEditor), so
return (editor instanceof nsITableEditor) ? editor : null; works.

Comment 42

[Charles Manske]

Last two patches checked in. CVS removed files no longer used.

Comment 43

[Kathleen :Brade]

verified via lxr