125146 - Editor embedding landing needs super-review

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Simon Fraser [no longer active]]

The editor embedding changes landed by mjudge between 2002/02/02 and 2002/02/04 
landed without super-review. Here's the checkin comment:

  115922 embedding work. no changes should be observed in
  current product this will only affect the embedded effort.
  This moves the command handler stuff from content xul to
  embedding/components/commandhandler. a=judd r=saari
  built on mac/windows/gmake with this we will be able to
  finish removing editorshell from the app and have a true
  embedded editor.akkana will pick this up after me and finish
  the embedding work.

See the url field for bonsai information.

I think we need to go over these changes and do an sr sweep. In particular, I 
think the following issues need addresing:

New docshell dependencies on composer and editor:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&
subdir=mozilla/docshell/build&command=DIFF_FRAMESET&file=makefile.win&rev1=1.15&
rev2=1.16&root=/cvsroot

The form of the to-be-public command handling API:
http://lxr.mozilla.org/mozilla/source/content/xul/document/public/
nsIController.idl#52

String usage issues:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&
root=/cvsroot&subdir=mozilla/editor/composer/src&command=DIFF_FRAMESET&file=
nsComposerCommands.cpp&rev2=1.43&rev1=1.42

I'm sure careful SR will find other issues too.

Note that parts of this landing _did_ get SR via other bugs (e.g. the dochsell 
source changes).

In addition, I did some work in layout (nsFrameFrame.cpp) that still hasn't 
landed. This work allowed <editor>'s in XUL to become editable without any JS, 
and is an important part of the story.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Diff for the checkin

Comment 2

[Simon Fraser [no longer active]]

Attachment: Diffs for checkin, second try

Comment 3

[Kathleen :Brade]

I fixed the string stuff.
What is the current state of this bug?

Comment 4

[mjudge]

Attachment: patch to change API.

this is not the fix for the docshell issuem, just the API change.

Comment 5

[Simon Fraser [no longer active]]

mjudge: can you attach a unified diff please? cvs diff -u

Comment 6

[mjudge]

Attachment: unified. wrong patch before

UnifieD this time

Comment 7

[mjudge]

i posted bigdif.dif instead of bigdif.txt oops
bigdif.txt is correct

Comment 8

[mjudge]

waiting for r= and sr= akkana will be up at the plate on monday.

Comment 9

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 10

[Brendan Eich [:brendan]]

This is ridiculous.

/be

Comment 11

[saari (gone)]

Yes, this should not have moved and is in progress. Thanks Brendan

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 71530 [details] [diff] [review]
unified. wrong patch before

Index: content/xbl/src/nsXBLPrototypeHandler.cpp

+    nsCString command;
+    command.AssignWithConversion(mHandlerText);

nsCAutoString would be good here

Index: content/xul/document/public/nsIController.idl

-	void onEvent(in DOMString eventName);
+	void onEvent(in string eventName);

I'm not sure about changing the event name type. hyatt should commment there.

Index: dom/src/base/nsGlobalWindow.cpp

+  if (!nsCRT::strcmp(sCopyString,aCommand)) {
...

Prefer to space after the comma.

+  if (!nsCRT::strcmp(sCopyString,aCommand) || 

Ditto. Lots of other occurrences of this.

Not your fault, but nsDOMWindowController::DoCommand() sucks; it does
lots of string compares to decide whether to call DoCommandWithEditInterface()
or DoCommandWithSelectionController(), which each then do a bunch more
string compares. Yuck!

Index: editor/composer/src/nsComposerCommands.cpp

+nsMultiStateCommand::DoCommandParams(const char *aCommandName,
nsICommandParams *aParams, nsISupports *refCon)
...
+      char *tValue;
       nsresult rv;
-      aParams->GetStringValue(STATE_ATTRIBUTE,tValue);
-      rv = SetState(editor, tValue);
+      aParams->GetCStringValue(STATE_ATTRIBUTE,&tValue);
+      nsString tStringValue;
+      tStringValue.AssignWithConversion(tValue);
+      if (tValue)
+	 delete tValue;

You should use nsMemory::Free() for disposing tValue.

+nsMultiStateCommand::GetCommandState(const char *aCommandName,
nsICommandParams *aParams, nsISupports *refCon)
...
+    nsCString tOutStateString;
+    tOutStateString.AssignWithConversion(outStateString);

Better to use nsCAutoString (several occurrences).

Index: embedding/components/commandhandler/public/nsICommandManager.idl
+   * aCommandName is te name of the command that needs the state
Typo

   const short eWStringType		   = 4;
   const short eISupportsType		   = 5;
+  const short eStringType		   = 6;

Put eStringType before eWStringType and renumber.

Index: embedding/components/commandhandler/src/nsCommandGroup.cpp
+  char*  commandString = nsCRT::strdup(aCommand);;	// we store allocated
PRUnichar* in the array

Fix the comment.

+    char*  commandString = (char*)commandList->ElementAt(i);

Better as a const char*.

-	 itemObserver->Observe((nsICommandManager *)this,
"command_status_changed", flatCommand.get());
+	 itemObserver->Observe((nsICommandManager *)this,
aCommandName,NS_LITERAL_STRING("command_status_changed").get());

OK, I suck. I think the second param here should always be the reason for the
notification
("command_status_changed"). So we'll have to copy the command name to unicode,
and pass that as the
3rd param:

  itemObserver->Observe((nsICommandManager *)this, "command_status_changed",
NS_ConvertASCIItoUCS2(aCommandName).get());

Index: embedding/components/commandhandler/src/nsCommandParams.cpp
...
+/* AString getStringValue (in AString name); */
+NS_IMETHODIMP nsCommandParams::GetCStringValue(const char * name, char
**_retval)

Fix the comment.


Index: embedding/tests/mfcembed/EditorFrm.cpp
 NS_METHOD
-CEditorFrame::ExecuteStyleCommand(const nsAString &aCommand)
+CEditorFrame::ExecuteStyleCommand(const char *aCommand)
 {
-	nsCOMPtr<nsICommandParams> params;
-	nsresult rv = MakeCommandParams(aCommand,getter_AddRefs(params));
-	if (NS_FAILED(rv))
-		return rv;
-	if (!params)
-		return NS_ERROR_FAILURE;
-    params->SetBooleanValue(STATE_ALL, true);
-
-    return DoCommand(params);
+  return DoCommand(aCommand,0);
 }

You lost the params->SetBooleanValue(STATE_ALL, true); part. Was that
deliberate?


+    char *tchar;
+    rv = params->GetCStringValue(STATE_ATTRIBUTE,&tchar);
+    aValue = tchar;
+    if (tchar)
+      delete []tchar;

Need to use nsMemory::Free();

Index: security/makefile.win
Probably don't want to check this in!

Comment 13

[Simon Fraser [no longer active]]

Comment on attachment 69352 [details] [diff] [review]
Diffs for checkin, second try

RCS file: /cvsroot/mozilla/docshell/base/Makefile.in,v

+		    composer \
+		    commandhandler \
+		    editor \

This is where we add dependencies in docShell on editor/composer. The
composer dependency should be removable easily. If docshell folks find
the dependency on editor unnacceptable, then we'd need to do more work
to remove it.

RCS file: /cvsroot/mozilla/embedding/components/build/Makefile.in,v
+		    commandhandler \
+		    content_xul \

Why do we need the content_xul dependency here?

+ NS_METHOD
+ CEditorImpl::MakeEditable()
...
+   rv= editingSession->MakeWindowEditable(domWindow, PR_TRUE);
+   // this can fail for the root (if it's a frameset), but we still want
+   // to make children editable
+   
+   nsCOMPtr<nsISimpleEnumerator> docShellEnumerator;
+   docShell->GetDocShellEnumerator( nsIDocShellTreeItem::typeContent,
+				      nsIDocShell::ENUMERATE_FORWARDS,
+				      getter_AddRefs(docShellEnumerator));  
+   if (docShellEnumerator)
+   {
+     PRBool hasMore;
+     while (NS_SUCCEEDED(docShellEnumerator->HasMoreElements(&hasMore)) &&
hasMore)
+     {
+	  nsCOMPtr<nsISupports> curSupports;
+	  rv = docShellEnumerator->GetNext(getter_AddRefs(curSupports));
+	  if (NS_FAILED(rv)) break;
+ 
+	  nsCOMPtr<nsIDocShell> curShell = do_QueryInterface(curSupports, &rv);
+	  if (NS_FAILED(rv)) break;
+ 
+	  nsCOMPtr<nsIDOMWindow> childWindow = do_GetInterface(curShell,&rv);
+	  if (childWindow)
+	    editingSession->MakeWindowEditable(childWindow, PR_FALSE);
+     }
+   }
+   return NS_OK;  
+ }

I think we need to add a param to 'MakeWindowEditable' that does what this
iteration
loop does; makes every subframe editable, since embedders should not have to
know
anything about nsIDocShell.

Comment 14

[mjudge]

We can make that makewindow editable change in another patch once this one goes 
in i think.  

we need the xul dependancy because the embedding code for the new command 
structure needs to see nsIController inside the xul directory.

the docshell dependancy is "under the hood" from any public view.  If we need 
to remove references to nsIEditor and instead put in nsISupports (with the wink 
wink we know its an editor really), that wont be a problem.   

Comment 15

[mjudge]

Attachment: final patch with changes. reset-r and sr then we apply for a=

patch that contains files about to be checked in. Re-Did it to make sure all
last minute changes were in.

Comment 16

[mjudge]

also to note. sujay the editor QA person tested this patch for 45min+ just to 
make sure no regressions happened.

Comment 17

[Akkana Peck]

Comment on attachment 74193 [details] [diff] [review]
final patch with changes. reset-r and sr then we apply for a=

r=akkana

Comment 18

[mjudge]

Attachment: YAP yet another patch

changed some nsString to nsAutoString.	Removed bogus selection file from dif.

Comment 19

[Simon Fraser [no longer active]]

OK, this patch still doesn't address issues that I raised before (comment #12), 
and there are still uses of ns[C]Strings on the stack that should be 
ns[C]AutoStrings:

Index: content/xbl/src/nsXBLPrototypeHandler.cpp
+    nsCString command;
+    command.AssignWithConversion(mHandlerText);

nsCAutoString.

Index: editor/composer/src/nsComposerCommands.cpp

+      aParams->GetCStringValue(STATE_ATTRIBUTE,&tValue);
+      nsAutoString tStringValue;
+      tStringValue.AssignWithConversion(tValue);
+      if (tValue)
+        delete tValue;

Like I said before, you can't use |delete| on tValue. Use nsMemory::Free().

+    nsCString tOutStateString;
+    tOutStateString.AssignWithConversion(outStateString);

nsCAutoString. 6 places.

+// why can't these two members be inside the union also?
+    nsCString*                mCString;
+    nsString*                 mString;
+    nsCOMPtr<nsISupports>     mISupports;    

The nsCOMPtr has to be outside the union, otherwise its ctor will clobber the 
initial values set for the other fields of the union.

The string pointers could go inside the union. I think I left them outside to 
ensure that it was always safe to call |delete| on that field.

I filed bug 132700 on allowing MakeEditable() to deal with nested <iframe>s.

Comment 20

[mjudge]

Attachment: big patch start at y:\mozilla

fixed conflicts with char * and nsAString & that occured some time ago.

Comment 21

[mjudge]

Attachment: new uber diff

new uber diff from mozilla directory. should be all problems fixed i hope.

Comment 22

[mjudge]

Comment on attachment 84353 [details] [diff] [review]
big patch start at y:\mozilla

obsolete

Comment 23

[Simon Fraser [no longer active]]

Index: embedding/components/commandhandler/src/nsCommandParams.h
===================================================================
+        case eStringType:
+          NS_ASSERTION(inRHS.mString, "Source entry has no string");
+          mCString = new nsCString(*inRHS.mCString);

The assertion is wrong here. It needs to check mCString.

Fix that, and sr=sfraser

Comment 24

[Simon Fraser [no longer active]]

Comment on attachment 89325 [details] [diff] [review]
new uber diff

sr=sfraser (with fix), bringing akkana's r= forward.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 89325 [details] [diff] [review]
new uber diff

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 26

[mjudge]

checked it into 1.1 trunk

Comment 27

[David Epstein]

checked patch against Mozilla 1.1b Gecko 20020715:
* /mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp
* /mozilla/content/xul/document/public/nsIController.idl
* /mozilla/content/xul/document/public/nsIControllerCommand.idl
* /mozilla/content/xul/document/public/nsIControllers.idl
* /mozilla/content/xul/document/src/nsXULCommandDispatcher.cpp
* /mozilla/content/xul/document/src/nsXULControllers.cpp
* /mozilla/docshell/base/nsWebShell.cpp
* /mozilla/docshell/base/nsWebShell.h
* /mozilla/dom/public/base/nsIFocusController.h
* /mozilla/dom/public/idl/xul/nsIDOMXULCommandDispatcher.idl
* /mozilla/dom/src/base/nsFocusController.cpp
* /mozilla/dom/src/base/nsFocusController.h
* /mozilla/dom/src/base/nsGlobalWindow.cpp
* /mozilla/dom/src/base/nsGlobalWindow.h
will check the rest later

Comment 28

[David Epstein]

checked the rest of the files. Everything looks ok. Minor spelling error in the
comments of nsICommandManager.idl if you wanted to change that, under
isCommandEnabled(in string, ...) : "aCommandName is te name ...", should be
"aCommandName is the name".

files checked:
* /mozilla/editor/composer/src/nsComposerCommands.cpp
* /mozilla/editor/composer/src/nsComposerCommandsUpdater.cpp
* /mozilla/editor/composer/src/nsComposerController.cpp
* /mozilla/editor/composer/src/nsEditorShell.cpp & .h
* /mozilla/editor/idl/nsIEditorController.idl
* /mozilla/editor/libeditor/base/nsEditorCommands.cpp & .h
* /mozilla/editor/libeditor/base/neEditorController.cpp & .h
* /mozilla/embedding/components/commandhandler/public/nsICommandManager.idl
* /mozilla/embedding/components/commandhandler/public/nsICommandParams.idl
* /mozilla/embedding/components/commandhandler/public/nsIControllerCommand.idl
*
/mozilla/embedding/components/commandhandler/public/nsIControllerCommandManager.idl
* /mozilla/embedding/components/commandhandler/public/nsPICommandUpdater.idl
* /mozilla/embedding/components/commandhandler/src/nsCommandGroup.cpp
* /mozilla/embedding/components/commandhandler/src/nsCommandManager.cpp & .h
* /mozilla/embedding/components/commandhandler/src/nsCommandParams.cpp & .h
* /mozilla/embedding/components/commandhandler/src/nsControllerCommandManager.cpp
* /mozilla/embedding/tests/mfcembed/EditorFrm.cpp & .h
* /mozilla/embedding/tests/mfcembed/mfcEmbed.rc
* /mozilla/embedding/tests/mfcembed/resource.h

Comment 29

[David Epstein]

verified