137629 - Remove editorshell dependencies from mailnews

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Akkana Peck]

As part of the ongoing editorshell removal, we need to remove editorshell
references from mailnews.

Comment 1

[Akkana Peck]

I have a preliminary patch.  It isn't complete -- doesn't address the creation
or window focus issues -- but it removes all editorshell dependency from
mailnews C++ files, and some from JS files.

Comment 2

[Akkana Peck]

Attachment: First patch: remove all edshell deps from C++, some from JS

Here's the patch.  It isn't ready for one reason: the call to LoadURL in
nsMsgCompose::SetEditor has no corresponding call.  This is part of the
initialization sequence problems which need to be worked out.

Comment 3

[Kathleen :Brade]

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

Comment 4

[Akkana Peck]

Here's an updated patch ... but it doesn't work yet.  For some reason, during
initialization, ComposeStartup gets called and sets
window.editorShell.editorType = "textmail", but then immediately after that
(according to venkman), just before we call EditorSharedStartup(),
window.editorShell.editorType is "text" and EditorSharedStartup doesn't do the
right things to set up a mail editor.  That's as far as I got ... I'll look at
it more Monday, but any insights would be appreciated!

Comment 5

[Akkana Peck]

Attachment: Updated patch (compiles but doesn't run)

Comment 6

[Kathleen :Brade]

Comment on attachment 100820 [details] [diff] [review]
Updated patch (compiles but doesn't run)

in nsMsgCompose.cpp, is "bodyText" still needed (around line 960-975)

This change in MsgComposeCommands.js looks suspicious, change it back to
editorShell?  Maybe that is the cause of your problems?
-    window.editorShell.contentWindow.focus();
+    window.editorshell.contentWindow.focus();

Should this line:
+  editor.flags |= nsIPlaintextEditorMail.eEditorReadonlyMask;
be
+  gMsgCompose.editor.flags |= nsIPlaintextEditorMail.eEditorReadonlyMask;

If so, fix these also:
+  editor.flags &= ~nsIPlaintextEditorMail.eEditorReadonlyMask;
+	 var mailEditor =
gEditor.QueryInterface(Components.interfaces.nsIEditorMailSupports);
+      window.editorshell.editor.wrapWidth = gMsgCompose.wrapLength;

Comment 7

[Jean-Francois Ducarroz]

Varada, can you help Akkana and Kathy. Thanks

Comment 8

[Akkana Peck]

Attachment: Another attempt (still doesn't work)

I couldn't make the old patch run, so I decided to start from scratch
converting the compose C++ files.  Here is the result, which compiles, but
crashes on startup, in factory code trying to create an NNTP url.  The older
patch was also crashing in factory code, but then it was trying to create an
nsITheme.  I have no idea why anything I've touched here should have anything
to do with factory code or either of those classes.  I have tried rebuilding
(depend) from the top, but I have not tried a clobber build; perhaps I'll try
that next.  If anyone has any ideas, I'd love to hear them.

Comment 9

[Akkana Peck]

Attachment: This one works!

Here's a patch that actually works, for both new messages and replies,
including quotes and signatures.

The order of things during initialization doesn't seem too well defined; I hope
setting the editor from the editorshell in the
nsMsgDocumentStateListener::NotifyDocumentCreated will handle all cases, at
least until Mike's new creation API is checked in and we can really ditch the
editorshell and use a more uniform initialization sequence.  Comments
appreciated from mailnews people who understand the initialization sequence (it
seems different from what we use for composer).

Aside from that, the main remaining use of editorshell is in MsgComposeCommands
for doing editorShell.contentWindow.focus().  Charley, what's the right way to
do this?  In editor.js we do window._content.focus(), but will that be enough
for a mail compose window that has more than one focusable area?  Should
MsgComposeCommands just define a gContentWindow that it gets from the
editorshell at init time, or can it access the gContentWindow from editor.js?

Other than that, everyone please test, review, pick holes in this.

Comment 10

[Kathleen :Brade]

Comment on attachment 101511 [details] [diff] [review]
This one works!

r=brade

My only suggestion is that you add some comments to explain why you the apis
are "noscript":
+	/* Clear the editor */
+	[noscript] void clearEditor();

Comment 11

[Akkana Peck]

As far as I'm concerned it doesn't have to be noscript; I was just copying the
other methods that are there only for the listeners.  I don't know why they're
all declared noscript; it doesn't look like they use non-scriptable interfaces,
except for notifyStateListeners.

Seth, can you sr this, and can I get a comment from you, Varada, J-F or another
mail person regarding whether you'd prefer the new ClearEditor method to be
noscript or not?  Thanks!

Comment 12

[Jean-Francois Ducarroz]

so far, we don't need to call ClearEditor from JS, therefore, it's fine with me
if it's not scriptable!

Comment 13

[(not reading, please use [e-mail] instead)]

most of this looks straight forward.

some minor nits and questions:

1)

+  *aEditorShell = m_editorShell;
+  NS_IF_ADDREF(*aEditorShell);

just do:

+  NS_IF_ADDREF(*aEditorShell = m_editorShell);


2)
 
   return rv;
+  bodyText = ToNewUnicode(bodyStr);
 
you should also add:

+  if (!bodyText)
+    return NS_ERROR_OUT_OF_MEMORY;

3)

+      var textEd = gMsgCompose.editor.QueryInterface(nsIPlaintextEditorMail);
+      textEd.wrapWidth = gMsgCompose.wrapLength;

since this is in a try block, why not just:

gMsgCompose.editor.QueryInterface(nsIPlaintextEditorMail).wrapWidth =
gMsgCompose.wrapLength

Question:  Why is one wrapWidth and one wrapLength? 

please test this well.  in addition to reply and new window (plain and html) and
signature stuff, make sure to to test the cached compose window, plain and html
(remember, not enabled by default on linux).

if everything tests ok, sr=sspitzer

Comment 14

[Akkana Peck]

Attachment: Tweaked to address Seth's comments

This has the changes Seth suggested.

> Question:  Why is one wrapWidth and one wrapLength? 

That's the way the APIs are written.  I haven't introduced any new APIs here,
I'm just trying to shift away from using the old obsolete one.

> please test this well.  in addition to reply and new window (plain and html)
and
> signature stuff, make sure to to test the cached compose window, plain and
html
> (remember, not enabled by default on linux).

I've tested plain and html, reply and new, first and second windows, on linux
and they all work fine.  It appears that it's not possible to test the recycled
compose window on linux; I tried 
pref("mail.compose.max_recycled_windows", 1);
and the first window worked fine, the second had problems that seem quite
similar to some of the problems described in bug 130581 and bug 137698 (which
is the reason that behavior is disabled on linux), third and subsequent
invocations of the window worked fine.

Kathy says she might be able to test on mac.

Comment 15

[(not reading, please use [e-mail] instead)]

>> Question:  Why is one wrapWidth and one wrapLength? 
>That's the way the APIs are written.  I haven't introduced any new APIs here,
>I'm just trying to shift away from using the old obsolete one.

ok, so beyond the scope of your bug.  but is it worth logging another bug?

>and the first window worked fine, the second had problems that seem quite
>similar to some of the problems described in bug 130581 and bug 137698 (which
>is the reason that behavior is disabled on linux), third and subsequent
>invocations of the window worked fine.

the cached compose window is a slippery fish and can be confusing to test.  (I
still need to write up a document about it for both eng / and QA).

before you check in, we need test on win & mac (both use the cached compose
window).  I'll go write up a quick doc about the cached compose window and put
it on mozilla.org.

Comment 16

[(not reading, please use [e-mail] instead)]

note visible yet, but see
http://www.mozilla.org/mailnews/arch/compose/cached.html for some info on the
cached compose window.

Comment 17

[Akkana Peck]

Attachment: Works for relaunch of cached windows

I talked to J-F a little about how the recycling works, and found out that the
editor flags were getting reset before gMsgCompose had gotten set to point to
the new editor.  Re-ordering those lines in gComposeRecyclingListener::onReopen
fixes the problem with the recycled window.  Works for me, on linux, and for
Kathy (thanks, Kathy!) on mac, plain and html.	This should be ready for sr
now.

Comment 18

[Akkana Peck]

Comment on attachment 102053 [details] [diff] [review]
Works for relaunch of cached windows

Kathy tested it on mac (I tested on linux) and gave her r=brade on IRC.

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 102053 [details] [diff] [review]
Works for relaunch of cached windows

assuming that {signature,nosig} x {reply,new} x {html,plaintext} x
{cached,notcached} x {win,mac,linux} all work, sr=sspitzer

(yikes, testing a mail compose change is non-trivial)

Comment 20

[(not reading, please use [e-mail] instead)]

moved the suggested test matrix to mozilla.org

see http://www.mozilla.org/mailnews/arch/compose/testing.html

(probably not there yet, wait an hour or two)

Comment 21

[Akkana Peck]

The remaining issues are:
1. initialization (cmanske is working on that).
2. Printing (spun off into bug 174391).
3. Content area focus (spun off into bug 174389).

So this bug can be marked fixed.

Comment 22

[Kathleen :Brade]

v