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.

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.

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

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)

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 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 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?

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

Comment on attachment 99548 [details] [diff] [review] patch v3 sr=dveditz

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

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()" .)

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

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?

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.

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.

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 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?

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.

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

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 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

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 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 on attachment 100781 [details] [diff] [review] Convert Get/SetDocumentTitle methods v5 ugh, that lookupMethod stuff is pretty nasty. ah well. sr=alecf

Comment on attachment 100781 [details] [diff] [review] Convert Get/SetDocumentTitle methods v5 This patch was checked into mozilla 1.2beta trunk

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.

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

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 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 on attachment 102198 [details] [diff] [review] Remove editorShell from ComposerCommands.js v1 sr=kin@netscape.com 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 on attachment 102198 [details] [diff] [review] Remove editorShell from ComposerCommands.js v1 checked into 1.2 trunk

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.

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.)

Changes per review, including: GetCurrentEditor() is QI'd to nsIEditor, nsIPlaintextEditor, and nsIHTMLEditor. For table editing, use GetCurrentTableEditor().

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

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 on attachment 103234 [details] [diff] [review] Final editor shell removal and cleanup v3 sr=sfraser

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

This also includes fix to dom inspector to remove editorshell

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

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

> 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.

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

verified via lxr