Closed Bug 1054735 Opened 6 years ago Closed 6 years ago

Clean up nsAutoReplaceContainerSelNotify; nsEditor::RemoveContainer, InsertContainerAbove

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Notice that I put it in mozilla::dom to avoid having to use "dom::Element".  It seems reasonable to me to put new editor things in mozilla::dom, since we use dom so heavily, but let me know if just "mozilla" is better.

Actually, while I'm doing this, I guess it makes sense to remove the "ns", right?
Attachment #8474213 - Flags: review?(ehsan)
Based on the name of this function, I considered making it take an Element* instead of nsIContent*, but figured some callers might be calling it on things that might or might not be containers.

As a general note, my approach these days is not to overload functions to take both old-style and new-style nodes, but rather to just change all the callers to use new-style nodes.  Otherwise you have to go back later and check that all the callers of the old version have actually been removed, which may take a really long time.  This way I can actually purge entire files of nsIDOMNode* (excepting NS_IMETHOD stuff).
Attachment #8474214 - Flags: review?(ehsan)
What's up with all the "NS_ENSURE_STATE(mHTMLEditor);" in this file?  Is it okay to remove these checks, or are they actually necessary?  If something is really going to null mHTMLEditor from under us, should we just take a reference in a local nsCOMPtr?

Also, is there any reason to have nsEditProperty, or is it better to just use nsGkAtoms?  I've been assuming the latter.  If that's right, should I just kill nsEditProperty?
Attachment #8474216 - Flags: review?(ehsan)
Summary: Clean up nsAutoReplaceContainerSelNotify; nsEditor::RemoveContainer, InsertContainerAbove, MoveNode → Clean up nsAutoReplaceContainerSelNotify; nsEditor::RemoveContainer, InsertContainerAbove
Previous version had a silly bug that got caught by tests.
Attachment #8474216 - Attachment is obsolete: true
Attachment #8474216 - Flags: review?(ehsan)
Attachment #8474512 - Flags: review?(ehsan)
Comment on attachment 8474213 [details] [diff] [review]
Part 1 -- Clean up nsAutoReplaceContainerSelNotify

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

FWIW putting the names in mozilla::dom and removing the ns prefix in general are good ideas.

::: editor/libeditor/nsSelectionState.h
@@ +212,5 @@
> +                                    Element* aOriginalElement,
> +                                    Element* aNewElement)
> +    : mRU(aRangeUpdater)
> +    , mOriginalElement(aOriginalElement)
> +    , mNewElement(aNewElement)

Nit: please indent the initializer list.
Attachment #8474213 - Flags: review?(ehsan) → review+
Attachment #8474214 - Flags: review?(ehsan) → review+
Comment on attachment 8474512 [details] [diff] [review]
Part 3, v2 -- Clean up nsEditor::InsertContainerAbove

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +4935,3 @@
>    }
>    if (deepestStyle) {
> +    *aOutBrNode = GetAsDOMNode(CreateBR(deepestStyle, 0).take());

Nit: please use .forget() when forgetting into as nsInterface**.
Attachment #8474512 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Nit: please use .forget() when forgetting into as nsInterface**.

Hmm, I don't understand.  .forget() on what?  CreateBR returns already_AddRefed, and I only see a .forget() method on nsCOMPtr/nsRefPtr, not already_AddRefed.  Do you mean you'd prefer creating an explicit temporary nsCOMPtr instead of .take()?
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor from comment #7)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #6)
> > Nit: please use .forget() when forgetting into as nsInterface**.
> 
> Hmm, I don't understand.  .forget() on what?  CreateBR returns
> already_AddRefed, and I only see a .forget() method on nsCOMPtr/nsRefPtr,
> not already_AddRefed.  Do you mean you'd prefer creating an explicit
> temporary nsCOMPtr instead of .take()?

Oh hmm, no I was wrong, nevermind.  Sorry!
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor from comment #3)
> What's up with all the "NS_ENSURE_STATE(mHTMLEditor);" in this file?  Is it
> okay to remove these checks, or are they actually necessary?  If something
> is really going to null mHTMLEditor from under us, should we just take a
> reference in a local nsCOMPtr?
> 
> Also, is there any reason to have nsEditProperty, or is it better to just
> use nsGkAtoms?  I've been assuming the latter.  If that's right, should I
> just kill nsEditProperty?
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor from comment #9)
> (In reply to :Aryeh Gregor from comment #3)
> > What's up with all the "NS_ENSURE_STATE(mHTMLEditor);" in this file?  Is it
> > okay to remove these checks, or are they actually necessary?  If something
> > is really going to null mHTMLEditor from under us, should we just take a
> > reference in a local nsCOMPtr?

mHTMLEditor can be null after a call to nsHTMLEditRules::DetachEditor.  That can be called either when we detach an editor from an element (for example during frame reconstruction) or when we destroy the editor (the rules object can technically outlive the editor object!)

Neither of these scenarios are things that we can easily protect against without these types of checks.

> > Also, is there any reason to have nsEditProperty, or is it better to just
> > use nsGkAtoms?  I've been assuming the latter.  If that's right, should I
> > just kill nsEditProperty?

It would be nice if you killed nsEditProperty.  It may have some atoms not in nsGkAtoms, and those we can easily add there.  I _think_ nsEditProperty may predate nsGkAtoms, but there is no good reason for it to exist any more.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> mHTMLEditor can be null after a call to nsHTMLEditRules::DetachEditor.  That
> can be called either when we detach an editor from an element (for example
> during frame reconstruction) or when we destroy the editor (the rules object
> can technically outlive the editor object!)
> 
> Neither of these scenarios are things that we can easily protect against
> without these types of checks.

Why can't we keep a reference to the editor in a local nsCOMPtr for the duration of the method's execution?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Comment on attachment 8474213 [details] [diff] [review]
> Part 1 -- Clean up nsAutoReplaceContainerSelNotify
> 
> Review of attachment 8474213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW putting the names in mozilla::dom and removing the ns prefix in general
> are good ideas.

mozilla::dom is for DOM code, though.
(In reply to :Aryeh Gregor from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > mHTMLEditor can be null after a call to nsHTMLEditRules::DetachEditor.  That
> > can be called either when we detach an editor from an element (for example
> > during frame reconstruction) or when we destroy the editor (the rules object
> > can technically outlive the editor object!)
> > 
> > Neither of these scenarios are things that we can easily protect against
> > without these types of checks.
> 
> Why can't we keep a reference to the editor in a local nsCOMPtr for the
> duration of the method's execution?

One danger here is if someone else has already freed the editor by the time we get to run this code.  If that happens, a local nsCOMPtr on the stack won't save us.  The other danger here is if the code in the method destroys the editor, but I'm fairly certain that all of those bugs have been fixed by now.  But yeah, it's probably worth doing what you suggest.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> One danger here is if someone else has already freed the editor by the time
> we get to run this code.  If that happens, a local nsCOMPtr on the stack
> won't save us.  The other danger here is if the code in the method destroys
> the editor, but I'm fairly certain that all of those bugs have been fixed by
> now.  But yeah, it's probably worth doing what you suggest.

How could either of those things happen if the refcount is nonzero?  Aren't refcounted classes supposed to be deleted only by their own Release methods?
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor from comment #16)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #14)
> > One danger here is if someone else has already freed the editor by the time
> > we get to run this code.  If that happens, a local nsCOMPtr on the stack
> > won't save us.  The other danger here is if the code in the method destroys
> > the editor, but I'm fairly certain that all of those bugs have been fixed by
> > now.  But yeah, it's probably worth doing what you suggest.
> 
> How could either of those things happen if the refcount is nonzero?  Aren't
> refcounted classes supposed to be deleted only by their own Release methods?

On the first case, what I meant was a scenario where you create a transaction object, hand it off an nsIEditor* with non-zero refcount, which is stored as a plain pointer, and _then_ the refcount dropping to zero, killing the editor object, therefore making our weak nsIEditor* pointer a dangling one.

On the second case, the issue is where you have a weak pointer to a refcounted object, you call into some code which ends up dropping the last reference to the object, and by the time that call returns, your pointer would be dangling, but like I said, I don't think that these classes of bugs can happen in the editor code any more.  (But I could be wrong on that, which is why I said it's probably worth doing what you suggest.)
Flags: needinfo?(ehsan)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.