Last Comment Bug 785091 - Remove nsIEditorObserver
: Remove nsIEditorObserver
Status: RESOLVED WONTFIX
[mentor=ehsan][lang=c++]
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 792979 811679
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-23 08:28 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2012-11-14 11:17 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (18.36 KB, patch)
2012-09-13 16:43 PDT, Mark Capella [:capella]
ehsan: feedback+
Details | Diff | Review
Patch (v2) (20.48 KB, patch)
2012-09-14 16:49 PDT, Mark Capella [:capella]
ehsan: review+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2012-08-23 08:28:40 PDT
We don't use the nsIEditorObserver interface for anything, it's time for it to go away.
Comment 1 :Ms2ger 2012-09-03 05:28:10 PDT
Ehsan, are you sure it's unused? nsEditor::NotifyEditorObservers seems to call the observers...
Comment 2 Mark Capella [:capella] 2012-09-06 01:19:58 PDT
Right ... there's some code in the function that might have to stay after yanking out the notify loop proper ... ?

But then There's the EditAction() function inherited in nsTextEditorState.cpp ... just pulling it, the nsIEditorObserver.idl and Add/Remove Notifications from nsIEditor.idl won't work ... tried and failed various mochitests ...

https://tbpl.mozilla.org/?tree=Try&rev=3a3593c8e594

Which I also re-create locally ... haven't gotten further / debugged - and will be away for close to a week ...
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-06 07:51:44 PDT
I'm sorry, I was extremely vague when I filed this bug.

The reason that I want nsIEditorObserver to go away is that it is a useless abstraction which is too generic without any good reason.  The only thing which uses it is nsTextEditorState.  I think we should remove nsIEditorObserver, add an abstract base class called EditActionListener, and allow the editor to hold a pointer to it and just call its EditAction when we currently would notify the observers.

That way, nsTextEditorState could inherit from that abstract base class and implement EditAction the same way that it does today, and then it can register and unregister itself on the nsEditor object, and nsEditor would only need to hold a single pointer to the listener since there will only ever be 0 or 1 edit action listeners.
Comment 4 Mark Capella [:capella] 2012-09-13 16:43:37 PDT
Created attachment 661047 [details] [diff] [review]
Patch (v1)

Asking for feedback as you have time. The code works / and tests out, but could be "prettier" I'm sure.
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-13 17:01:01 PDT
Comment on attachment 661047 [details] [diff] [review]
Patch (v1)

Review of attachment 661047 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!!!

I have a few comments on the patch to make it better, but you basically did the right thing throughout the patch.  Good job!  :-)

::: content/html/content/src/nsTextEditorState.cpp
@@ +639,5 @@
>    NS_DECL_NSISELECTIONLISTENER
>  
>    NS_DECL_NSIDOMEVENTLISTENER
>  
> +  nsresult EditAction();

Nit: please also include "virtual" in the declaration.

@@ +828,5 @@
>  
>    return NS_OK;
>  }
>  
> +// BEGIN EditActionListener

Nit: You can drop this and the END macro, they're pretty much useless!

::: editor/idl/nsIEditor.idl
@@ +25,5 @@
>  interface nsIEditActionListener;
>  interface nsIInlineSpellChecker;
>  interface nsITransferable;
>  
>  [scriptable, uuid(7ad59e28-f3d5-4e14-8ea3-794ad4a86de3)]

Please change the uuid of this interface as you're changing its methods.

@@ +517,5 @@
>     * and document state listeners.
>     */
>  
>    /** add an EditorObserver to the editors list of observers. */
> +  void addEditorObserver(in EditActionListener observer);

Instead of this name, please use "setEditorObserver" which does not suggest that there are multiple observers.

@@ +522,3 @@
>  
>    /** Remove an EditorObserver from the editor's list of observers. */
> +  void removeEditorObserver();

Hmm, I was going to suggest that we can remove these from the nsIEditor interface and move them to nsEditor, but that requires nsIEditor to be builtinclass, otherwise JS and other things can also implement nsIEditor, in which case we don't wanna cast it to nsEditor*.  So this looks good for now.

::: editor/libeditor/base/EditActionListener.h
@@ +9,5 @@
> +class EditActionListener
> +{
> +public:
> +
> +  virtual nsresult EditAction() { return NS_OK; }

Please make this pure virtual.  Also, we don't use the return value, so let's just make it void.  :-)

::: editor/libeditor/base/nsEditor.cpp
@@ +69,5 @@
>  #include "nsIDOMText.h"                 // for nsIDOMText
>  #include "nsIDocument.h"                // for nsIDocument
>  #include "nsIDocumentStateListener.h"   // for nsIDocumentStateListener
>  #include "nsIEditActionListener.h"      // for nsIEditActionListener
> +#include "EditActionListener.h"         // for EditActionListener

Nit: please reorder this so that the include list would still be alphabetical.

@@ -174,5 @@
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mTxnMgr)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mIMETextRangeList)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mIMETextNode)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mActionListeners)
> - NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mEditorObservers)

You should probably point to the new member object using a native ptr macro...

@@ -193,5 @@
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mTxnMgr, nsITransactionManager)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mIMETextRangeList)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mIMETextNode)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mActionListeners)
> - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mEditorObservers)

Same here.

@@ -445,5 @@
>  
>    // Unregister event listeners
>    RemoveEventListeners();
>    mActionListeners.Clear();
> -  mEditorObservers.Clear();

Please set the observer to null here.

@@ +1766,3 @@
>    NS_ENSURE_TRUE(aObserver, NS_ERROR_NULL_POINTER);
>  
> +  mEditActionListener = aObserver;

Also, MOZ_ASSET that mEditActionListener is null when this is called, to make sure that nobody uses this as an observer list.

::: editor/libeditor/base/nsEditor.h
@@ +53,5 @@
>  class nsIDOMRange;
>  class nsIDocument;
>  class nsIDocumentStateListener;
>  class nsIEditActionListener;
> +class EditActionListener;

This should not be needed, since this file #includes nsIEditor.h.
Comment 6 Mark Capella [:capella] 2012-09-13 18:43:44 PDT
The only question I have is with the "You should probably point to the new member object using a native ptr macro..." part ...

I really don't understand the cycle collection macros yet. I currently have mEditActionListener defined as a raw pointer, vs nsCOMPtr<> kinda thing, so adding a NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mEditActionListener) doesn't leap out at me.

And if you're thinking I need to use NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_NATIVE somehow then I'm really lost, as example code I scanned is sparse.

Can you explain a little further?
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-14 11:03:36 PDT
Hmm, you know what?  That pointer is not a strong reference, so I may in fact be wrong, and you may not need to mark that at all in the cycle collection functions.  CCing smaug to confirm this.
Comment 8 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-09-14 11:54:46 PDT
Yeah, if it isn't a ref counted pointer, you shouldn't need to tell the cycle collector about it.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-14 13:00:18 PDT
(In reply to comment #8)
> Yeah, if it isn't a ref counted pointer, you shouldn't need to tell the cycle
> collector about it.

OK good.  So please ignore my comment about the cycle collector then, Mark!
Comment 10 Mark Capella [:capella] 2012-09-14 16:49:13 PDT
Created attachment 661414 [details] [diff] [review]
Patch (v2)

B)

Made the changes, re-tested, push to TRY looks good:
https://tbpl.mozilla.org/?tree=Try&rev=61dd7a5e116f
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-17 08:13:09 PDT
Comment on attachment 661414 [details] [diff] [review]
Patch (v2)

Review of attachment 661414 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  Thanks so much for your patch, Mark!  :-)
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-17 08:16:00 PDT
Landed with my first name fixed in the commit message.  :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a470dfb0b2
Comment 13 Ed Morley [:emorley] 2012-09-17 12:23:37 PDT
https://hg.mozilla.org/mozilla-central/rev/f4a470dfb0b2
Comment 14 Daniel Glazman (:glazou) 2012-11-13 07:10:45 PST
Guys, *please*, when you change something like that, add me to the cc list...
I'm only one of the largest nsIEditor users around, eh?
For the record, BlueGriffon is/was using it :-/
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-11-13 07:22:07 PST
glazou, do you need nsIEditorObserver?
It is not too late to add it back, if someone provides backout patch.

Also, perhaps you could watch all the editor bugs. There shouldn't be too many bugmails.
Comment 16 Daniel Glazman (:glazou) 2012-11-13 08:03:38 PST
(In reply to Olli Pettay [:smaug] from comment #15)

> glazou, do you need nsIEditorObserver?
> It is not too late to add it back, if someone provides backout patch.

I am using it at least once, and that's a pretty major piece of code
to observe everything going on in the editor. I'll investigate more
tomorrow when I'm back at the office (I'm in town right now) and will
keep you posted.

> Also, perhaps you could watch all the editor bugs. There shouldn't be too
> many bugmails.

Yeah. Will do that. But cc:ing me on important editor stuff is not that
difficult, is it?
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-11-13 08:22:13 PST
Not everybody knows that you'd want to be CC'ed. The same way that not everybody knows that
I should be CC'ed to all the DOM Events and Event Handling bugs - so I watch both those components.
Comment 18 Daniel Glazman (:glazou) 2012-11-14 01:36:05 PST
Ok, it seems I *could* use the new code but there is something looking
wrong with it... Here's the new API or nsIEditor for the observer:

  void setEditorObserver(in EditActionListener observer);

and if in my JS code, I try to reach it, it's undefined, the only method
available on nsIEditor that remains undefined although it's in the IDL.

Is it because the argument is an EditActionListener defined as a C++
class and not an IDL and in that case made automatically unscriptable?

I *absolutely* need to be able to set that observer from JS code because
the observer is notified from nsPlaintextEditor::Undo and ::Redo...
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-11-14 02:27:11 PST
Ok, I think we should back this out. Ehsan, comments?
Comment 20 Daniel Glazman (:glazou) 2012-11-14 05:45:42 PST
(In reply to Olli Pettay [:smaug] from comment #19)

> Ok, I think we should back this out. Ehsan, comments?

Another possibility that I would be fine with would be to extend
nsIEditActionListener and add DidUndo() DidRedo() stuff there and
call them from the nsPlaintextEditor Undo/Redo methods. But that
could impact all current users of nsIEditActionListener.

Unfortunately, nsIEditorObserver is currently the _only_ way to
be notified an Undo or Redo event occurred in the editor. I tried
other approaches w/O success in the past.
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2012-11-14 10:15:49 PST
Sorry for the late reply here, Daniel.  Have you tried to see if you can use MutationObserver instead of nsIEditorObserver?  The Thunderbird people managed to use it because they needed to get notified every time that the editable document changed.  If that doesn't work for you, we can look into backing this out, but I'd like to avoid doing that if possible.  :-)

Please ping me on IRC if you want to discuss this.
Comment 22 Daniel Glazman (:glazou) 2012-11-14 10:19:23 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #21)
> Sorry for the late reply here, Daniel.  Have you tried to see if you can use
> MutationObserver instead of nsIEditorObserver?  The Thunderbird people
> managed to use it because they needed to get notified every time that the
> editable document changed.  If that doesn't work for you, we can look into
> backing this out, but I'd like to avoid doing that if possible.  :-)
> 
> Please ping me on IRC if you want to discuss this.

I tried that strategy in the past and it has issues for me because it
reports far more than I need and because I need something specific
to Undo and Redo. As I said earlier, I don't *absolutely* need a
backout, but I need a *scriptable* way of being notified an Undo or
Redo occurred and the current code, from this bug, does not allow it.
See what I said in comment 20 above.

Note You need to log in before you can comment on or make changes to this bug.