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)
Core
DOM: Editor
Tracking
()
VERIFIED
FIXED
mozilla1.3alpha
People
(Reporter: cmanske, Assigned: cmanske)
References
Details
Attachments
(1 file, 5 obsolete files)
33.49 KB,
patch
|
mjudge
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
Various issues were discovered as part of removal of editorShell from Composer application. They will be explained further in the patch.
Assignee | ||
Comment 1•22 years ago
|
||
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
Assignee | ||
Comment 2•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Blocks: editorshell, 169029
Whiteboard: [FIX IN HAND]need r=,sr=
Comment 3•22 years ago
|
||
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++) }
Assignee | ||
Comment 4•22 years ago
|
||
Improvements per reviewer comments
Attachment #102948 -
Attachment is obsolete: true
Updated•22 years ago
|
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 5•22 years ago
|
||
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+
Assignee | ||
Comment 6•22 years ago
|
||
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
Assignee | ||
Comment 7•22 years ago
|
||
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.)
Comment 8•22 years ago
|
||
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.)
Assignee | ||
Comment 9•22 years ago
|
||
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 10•22 years ago
|
||
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+
Assignee | ||
Updated•22 years ago
|
Whiteboard: [FIX IN HAND]need r=,sr= → [FIX IN HAND]need sr=
Comment 11•22 years ago
|
||
- 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.
Assignee | ||
Comment 12•22 years ago
|
||
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
Comment 13•22 years ago
|
||
*** Bug 79875 has been marked as a duplicate of this bug. ***
Comment 14•22 years ago
|
||
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+
Assignee | ||
Comment 15•22 years ago
|
||
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?
Comment 16•22 years ago
|
||
That last attachment looks just like the previous one. Is it the correct patch?
Assignee | ||
Comment 17•22 years ago
|
||
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 18•22 years ago
|
||
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+
Assignee | ||
Comment 19•22 years ago
|
||
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 20•22 years ago
|
||
Comment on attachment 103871 [details] [diff] [review] New commands v5 r=mjudge
Attachment #103871 -
Flags: review+
Assignee | ||
Comment 21•22 years ago
|
||
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
Comment 22•22 years ago
|
||
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"?
Assignee | ||
Comment 23•22 years ago
|
||
I disagree. "cmd_bold" doesn't seem to imply "make bold" any more or less than cmd_documentModified imples "set documentModified"
Assignee | ||
Comment 24•22 years ago
|
||
*** Bug 156756 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 25•22 years ago
|
||
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND]need a=
Comment 26•22 years ago
|
||
verified by code inspection
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.2final → mozilla1.3alpha
Assignee | ||
Comment 27•22 years ago
|
||
*** 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.
Description
•