Closed Bug 1156062 Opened 9 years ago Closed 8 years ago

Yet more editor cleanup

Categories

(Core :: DOM: Editor, enhancement)

Product:
Core
Component:
DOM: Editor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(15 files, 17 obsolete files)

Part 1a -- New nsEditor::SplitNodeDeep variant
4.11 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 1b - Convert some callers of nsEditor::SplitNodeDeep
25.81 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 1c -- Convert remaining callers of nsEditor::SplitNodeDeep
18.42 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 1d -- Clean up nsEditor::SplitNodeDeep
8.31 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 2 -- Make GetBlockNodeParent and GetEnclosingTable return ordinary pointers
5.81 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 3 -- New helper nsHTMLEditor::GetBlock
15.84 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 4 updated -- Clean up ns*EditRules::WillInsert
21.41 KB, patch
Details | Diff | Splinter Review
Part 5 updated
8.10 KB, patch
Details | Diff | Splinter Review
Part 6 updated
11.41 KB, patch
Details | Diff | Splinter Review
Part 7 updated
10.02 KB, patch
Details | Diff | Splinter Review
0005-Bug-1156062-part-8-Clean-up-nsHTMLEditRules-JoinBloc.patch
25.02 KB, patch
Details | Diff | Splinter Review
0006-Bug-1156062-part-9-Clean-up-nsHTMLEditRules-WillAlig.patch
23.67 KB, patch
Details | Diff | Splinter Review
0007-Bug-1156062-part-10-Clean-up-MarginPropertyAtomForIn.patch
7.51 KB, patch
Details | Diff | Splinter Review
0008-Bug-1156062-part-11-Clean-up-nsHTMLEditRules-Before-.patch
6.15 KB, patch
Details | Diff | Splinter Review
0009-Bug-1156062-part-12-Clean-up-nsHTMLEditRules-GetAlig.patch
10.06 KB, patch
Details | Diff | Splinter Review 
Last patch set for this break.  Next break is July 27-August 14.
Flags: in-testsuite-
Nope, this patch set will have to wait for next break.  Some kind of bug in the first patch in the series that I can't work out.
Comment on attachment 8641107 [details] [diff] [review]
Part 1a -- New nsEditor::SplitNodeDeep variant

This seems to have finally passed try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56da7752f900

A later patch will remove the old variant and add a cleaned-up implementation.
Attachment #8641107 - Attachment description: Part 1a -- New nsEditor → Part 1a -- New nsEditor::SplitNodeDeep variant
Attachment #8641107 - Attachment is patch: true
Attachment #8641107 - Flags: review?(ehsan)
Attachment #8641107 - Flags: review?(ehsan) → review+
Attachment #8641116 - Flags: review?(ehsan) → review+

        Attachment #8641173 -
        Attachment is obsolete: true

        Attachment #8641173 -
        Flags: review?(ehsan)

        Attachment #8641174 -
        Flags: review?(ehsan)

    
    

    
  


  
I think errors here are unlikely enough to be ignored (if you don't need the offset).  It might cause errors if there are non-editable elements lurking around, but I guess we won't do anything sensible in that case anyway, so we may as well ignore it.

        Attachment #8641179 -
        Attachment is obsolete: true

        Attachment #8641182 -
        Flags: review?(ehsan)

    
    

    
  


  

        Attachment #8641182 -
        Attachment is obsolete: true

        Attachment #8641182 -
        Flags: review?(ehsan)

        Attachment #8641183 -
        Flags: review?(ehsan)

    
    

    
  


  
They won't have a refcount of one, so this is as safe as returning raw pointers ever is.

        Attachment #8641184 -
        Flags: review?(ehsan)

    
    

    
  


  
Maybe this should take nsIContent&.  I'm not sure with this sort of thing.

        Attachment #8641186 -
        Flags: review?(ehsan)

    
    

    
  


  
The refcount will still always be nonzero, because it's inserted into the DOM before being returned.

        Attachment #8641197 -
        Flags: review?(ehsan)

    
    

    
  
Past experience has shown that landing these changes en masse is a mistake, since they are regression prone.  I'd really appreciate if you could split up the work and try to land one or two of these patches per bug...

    
    

    
  
I'm pushing these patches one per day, by Ehsan's request.
Keywords: leave-open

    
    

    
  
Comment on attachment 8641183 [details] [diff] [review]
Part 1d -- Clean up nsEditor::SplitNodeDeep

> int32_t
>-nsEditor::SplitNodeDeep(nsIContent& aNode, nsIContent& aSplitPointParent,
>+nsEditor::SplitNodeDeep(nsIContent& aNode,
>+                        nsIContent& aSplitPointParent,
>                         int32_t aSplitPointOffset,
>                         EmptyContainers aEmptyContainers,
>-                        nsIContent** outLeftNode,
>-                        nsIContent** outRightNode)
>+                        nsIContent** aOutLeftNode,
>+                        nsIContent** aOutRightNode)
[Huh, why aren't these changes in patch 1a in the first place?]

    
  

        Attachment #8641174 -
        Flags: review?(ehsan) → review+

    
    

    
  
Comment on attachment 8641183 [details] [diff] [review]
Part 1d -- Clean up nsEditor::SplitNodeDeep

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

::: editor/libeditor/nsEditor.cpp
@@ +3854,5 @@
> +    // if you are after the last <li> or before the first, etc.  For now we
> +    // just have some smarts about unneccessarily splitting text nodes, which
> +    // should be universal enough to put straight in this nsEditor routine.
> +
> +    bool doSplit = false;

Uber-nit: didSplit sounds like a better name!

        Attachment #8641183 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #8641184 -
        Flags: review?(ehsan) → review+

    
    

    
  
Comment on attachment 8641186 [details] [diff] [review]
Part 3 -- New helper nsHTMLEditor::GetBlock

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

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +2333,5 @@
>        // Figure out block parents
> +      NS_ENSURE_STATE(mHTMLEditor);
> +      nsCOMPtr<Element> leftParent = mHTMLEditor->GetBlock(*startNode);
> +
> +      NS_ENSURE_STATE(mHTMLEditor);

Don't need to do this again.

@@ +3597,5 @@
>      int32_t offset;
>      NS_ENSURE_STATE(mHTMLEditor);
>      nsresult res = mHTMLEditor->GetStartNodeAndOffset(aSelection, getter_AddRefs(node), &offset);
>      NS_ENSURE_SUCCESS(res, res);
> +    NS_ENSURE_STATE(mHTMLEditor);

Ditto.

@@ +4950,5 @@
>    if (IsInlineNode(GetAsDOMNode(aBodyNode))) {
>      return NS_OK;
>    }
> +  NS_ENSURE_STATE(mHTMLEditor);
> +  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);

Out of curiosity, why do you need a death grip?  Just out of caution?

@@ +7464,5 @@
>    }
>  
>    // make sure we aren't in an empty block - user will see no cursor.  If this
>    // is happening, put a <br> in the block if allowed.
> +  NS_ENSURE_STATE(mHTMLEditor);

Same.

@@ +7510,5 @@
>    {
>      // is nearNode also a descendant of same block?
> +    NS_ENSURE_STATE(mHTMLEditor);
> +    nsCOMPtr<Element> block = mHTMLEditor->GetBlock(*selNode);
> +    NS_ENSURE_STATE(mHTMLEditor);

Same.

        Attachment #8641186 -
        Flags: review?(ehsan) → review+

    
    

    
  
NS_ENSURE_STATE(mHTMLEditor) is a pattern that occurs everywhere in nsHTMLEditRules.  (By "everywhere" I mean 14628 times.)  Methods tend to call it before every access to mHTMLEditor.  In cleanup, I've been replacing it with a single call at the beginning of the method and a kungFuDeathGrip.  I don't know why this is done, but I assumed it was because if the nsHTMLEditor dies, it will somehow null the nsHTMLEditRules' reference to it.  (Otherwise what's to stop mHTMLEditor from being a dangling pointer?)

If all these checks are just wrong, let me know and I'll get rid of them as I go!
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to :Aryeh Gregor (working until October 13) from comment #29)
> NS_ENSURE_STATE(mHTMLEditor) is a pattern that occurs everywhere in
> nsHTMLEditRules.  (By "everywhere" I mean 14628 times.)  Methods tend to
> call it before every access to mHTMLEditor.  In cleanup, I've been replacing
> it with a single call at the beginning of the method and a kungFuDeathGrip. 
> I don't know why this is done, but I assumed it was because if the
> nsHTMLEditor dies, it will somehow null the nsHTMLEditRules' reference to
> it.  (Otherwise what's to stop mHTMLEditor from being a dangling pointer?)
> 
> If all these checks are just wrong, let me know and I'll get rid of them as
> I go!

Well, some methods can destroy the world and some can't.  GetBlock is an example of one that can't, so if you have null checked mHTMLEditor before, you don't need to do it after.  And obviously you don't need it at all if you have a death grip over the object.

When in doubt, keep the null checks, they don't hurt that badly.  :-)
Flags: needinfo?(ehsan)

    
    

    
  
I like coding patterns that don't require me to think so much, because if I think then maybe I'm wrong.  :)  Or maybe I'm right but someone will change the code eight years from now and undermine my assumptions.  Besides, when I get around to rewriting these functions I'll use a kungFuDeathGrip there too and get rid of all these extra NS_ENSURE_STATE()s.

    
    

    
  
Comment on attachment 8641192 [details] [diff] [review]
Part 4 -- Clean up ns*EditRules::WillInsert

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

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +1212,5 @@
> +  }
> +
> +  // If we are after a mozBR in the same block, then move selection to be
> +  // before it
> +  NS_ENSURE_TRUE(aSelection.GetRangeAt(0) &&

Nit: use NS_ENSURE_TRUE_VOID please.

@@ +1231,5 @@
> +      // to be before the mozBR.
> +      selNode = priorNode->GetParentNode();
> +      selOffset = selNode->IndexOf(priorNode);
> +      nsresult res = aSelection.Collapse(selNode, selOffset);
> +      NS_ENSURE_SUCCESS(res, );

Ditto.

@@ +1240,5 @@
>        (mTheAction == EditAction::insertText ||
>         mTheAction == EditAction::insertIMEText ||
>         mTheAction == EditAction::deleteSelection)) {
> +    nsresult res = ReapplyCachedStyles();
> +    NS_ENSURE_SUCCESS(res, );

And here.

::: editor/libeditor/nsTextEditRules.cpp
@@ +351,5 @@
>    *aCancel = false;
>  
>    // check for the magic content node and delete it if it exists
> +  if (mBogusNode) {
> +    NS_ENSURE_TRUE(mEditor, );

Here too.

        Attachment #8641192 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #8641197 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #8641198 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #8641202 -
        Flags: review?(ehsan) → review+

    
    

    
  
Comment on attachment 8642165 [details] [diff] [review]
Part 9 -- Clean up nsHTMLEditRules::WilAlign

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

Wrong patch.

        Attachment #8642165 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8642166 [details] [diff] [review]
Part 10 -- Clean up MarginPropertyAtomForIndent

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

Wrong patch.

        Attachment #8642166 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8642167 [details] [diff] [review]
Part 11 -- Clean up nsHTMLEditRules::Before/AfterEdit

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

Wrong patch.

        Attachment #8642167 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8642168 [details] [diff] [review]
Part 12 -- Clean up nsHTMLEditRules::GetAlignment

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

Wrong patch.

        Attachment #8642168 -
        Flags: review?(ehsan)

    
  

        Attachment #8641209 -
        Flags: review?(ehsan) → review+

    
    

    
  


  
I know you're no longer owner of the editor component, and that your name currently says not to ask for review, but I'll ask you to review these last four patches anyway, so that everything can finally land.  (Unless you have someone else to suggest.)

        Attachment #8642165 -
        Attachment is obsolete: true

        Attachment #8739778 -
        Flags: review?(ehsan)

    
    

    
  


  

        Attachment #8642167 -
        Attachment is obsolete: true

        Attachment #8739780 -
        Flags: review?(ehsan)

    
    

    
  


  

        Attachment #8642168 -
        Attachment is obsolete: true

        Attachment #8739781 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8739778 [details] [diff] [review]
Part 9 -- Clean up nsHTMLEditRules::WillAlign

Masayuki, can you please review these?  Thanks!

        Attachment #8739778 -
        Flags: review?(ehsan) → review?(masayuki)

    
  

        Attachment #8739779 -
        Flags: review?(ehsan) → review?(masayuki)

    
  

        Attachment #8739780 -
        Flags: review?(ehsan) → review?(masayuki)

    
  

        Attachment #8739781 -
        Flags: review?(ehsan) → review?(masayuki)

    
    

    
  
Comment on attachment 8739778 [details] [diff] [review]
Part 9 -- Clean up nsHTMLEditRules::WillAlign

> nsresult
>-nsHTMLEditRules::WillAlign(Selection* aSelection,
>-                           const nsAString *alignType,
>-                           bool *aCancel,
>-                           bool *aHandled)
>+nsHTMLEditRules::WillAlign(Selection& aSelection,
>+                           const nsAString& aAlignType,
>+                           bool* aCancel,
>+                           bool* aHandled)

Making methods use reference rather than pointer is good for security, I think.

Although, I think that it's not a scope of this bug, perhaps, a lot of methods in editor returns two or more values, such as succeeded/error with reason, consumed, handled and something. If we will make it includes a stuct, we can avoid to use out arguments in a lot of place.


>-  WillInsert(*aSelection, aCancel);
>+  NS_ENSURE_STATE(mHTMLEditor);

Although, NS_ENSURE_* is deprecated and I like to use NS_WARN_IF() style. But fine.

>+  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);
> 
>-  // initialize out param
>-  // we want to ignore result of WillInsert()
>+  WillInsert(aSelection, aCancel);

nit: I wonder don't we need to check mHTMLEditor here? Looks like mBogusNode will be removed. And you removed some checks of mHTMLEditor from following code.

>+  // Initialize out param.  We want to ignore result of WillInsert().
>   *aCancel = false;
>   *aHandled = false;
> 
>-  nsresult res = NormalizeSelection(aSelection);
>+  nsresult res = NormalizeSelection(&aSelection);

nit: Could you rename res to rv?

>-    if (nsTextEditUtils::IsBreak(theNode))
>+    if (node->IsHTMLElement(nsGkAtoms::br)) {

I don't like this change. Until nsTextEditUtils::IsBreak() is removed, all developers expects adding new rule to this method changes whole behavior of similar code. So, please keep current code here until you'll remove the utility method.

However, looks like that you should add |!emptyDiv &&| here.

>-      if (!nsHTMLEditUtils::IsTableElement(parent) || nsHTMLEditUtils::IsTableCellOrCaption(parent))
>+      if (!nsHTMLEditUtils::IsTableElement(parent) ||
>+          nsHTMLEditUtils::IsTableCellOrCaption(parent)) {
>         emptyDiv = true;

I like this better:

emptyDev = !nsHTMLEditUtils::IsTableElement(parent) ||
           nsHTMLEditUtils::IsTableCellOrCaption(parent);

because emptyDiv is always false before here.

>+      }
>     }
>   }

It seems that separating this block to another method which explains what it does with its name. However, it's okay for now because separating to multiple methods should be changed in different patch or bug for easier to review.

>+  if (emptyDiv) {
>+    NS_ENSURE_STATE(aSelection.GetRangeAt(0) &&
>+                    aSelection.GetRangeAt(0)->GetStartParent());
>+    nsCOMPtr<nsINode> parent = aSelection.GetRangeAt(0)->GetStartParent();

Hmm, then, this should be:

nsCOMPtr<nsINode> parent =
  aSelection.GetRangeAt(0) ? aSelection.GetRangeAt(0)->GetStartParent() :
                             nullptr;
NS_ENSURE_STATE(parent);

>-    NS_ENSURE_STATE(mHTMLEditor);
>-    res = mHTMLEditor->GetNextHTMLNode(GetAsDOMNode(parent), offset,
>-                                       address_of(brNode));
>+    nsCOMPtr<nsIContent> brNode = mHTMLEditor->GetNextHTMLNode(parent, offset);

As I said above, mHTMLEditor may be nullptr here...

And I like |brContent| rather than |brNode| because its type is nsIContent.

>-    if (brNode && nsTextEditUtils::IsBreak(brNode))
>-    {
>+    if (brNode && brNode->IsHTMLElement(nsGkAtoms::br)) {

Same above. I don't think that it is the time to stop using nsTextEditUtils::IsBreak().

>-    NS_ENSURE_STATE(mHTMLEditor);
>-    nsCOMPtr<Element> theDiv = mHTMLEditor->CreateNode(nsGkAtoms::div, parent,
>-                                                       offset);
>-    NS_ENSURE_STATE(theDiv);
>-    // remember our new block for postprocessing
>-    mNewBlock = GetAsDOMNode(theDiv);
>-    // set up the alignment on the div, using HTML or CSS
>-    nsCOMPtr<nsIDOMElement> divElem = do_QueryInterface(theDiv);
>-    res = AlignBlock(divElem, alignType, true);
>+    nsCOMPtr<Element> div = mHTMLEditor->CreateNode(nsGkAtoms::div, parent,
>+                                                    offset);

nit: I like divElement but up to you because this block is enough small.

>+  for (uint32_t i = 0; i < nodeArray.Length(); i++) {

nit: size_t?

>+    auto& curNode = nodeArray[i];

nit: I like currentNode better, but up to you.

>+    nsCOMPtr<nsINode> curParent = curNode->GetParentNode();

nit: I like currentParentNode better, but up to you.

>+    int32_t offset = curParent ? curParent->IndexOf(curNode) : -1;

nit: I like offsetInCurrentParent better, but up to you.

>       res = SplitAsNeeded(*nsGkAtoms::div, curParent, offset);
>       NS_ENSURE_SUCCESS(res, res);
>-      NS_ENSURE_STATE(mHTMLEditor);
>       curDiv = mHTMLEditor->CreateNode(nsGkAtoms::div, curParent, offset);

Oh... I guess that you need to check mHTMLEditor after some methods above if you don't like to check it before calling a method of the instance.


If you're ready to use MozReview, please use it. The diff tools in MozReview is useful/helpful especially for refactoring like this.

        Attachment #8739778 -
        Flags: review?(masayuki) → review-

    
    

    
  
Comment on attachment 8739779 [details] [diff] [review]
0007-Bug-1156062-part-10-Clean-up-MarginPropertyAtomForIn.patch

>-static nsIAtom* MarginPropertyAtomForIndent(nsHTMLCSSUtils* aHTMLCSSUtils,
>-                                            nsIDOMNode* aNode) {
>+static nsIAtom& MarginPropertyAtomForIndent(nsHTMLCSSUtils& aHTMLCSSUtils,
>+                                            nsINode& aNode) {

Put the |{| to the next line.

        Attachment #8739779 -
        Flags: review?(masayuki) → review+

    
    

    
  
Comment on attachment 8739780 [details] [diff] [review]
Part 11 -- Clean up nsHTMLEditRules::Before/AfterEdit

>-      nsresult res = CacheInlineStyles(selNode);
>+      nsresult res = CacheInlineStyles(GetAsDOMNode(selNode));

Rename |res| to |rv|.


> NS_IMETHODIMP
> nsHTMLEditRules::AfterEdit(EditAction action,
>                            nsIEditor::EDirection aDirection)
> {
>-  if (mLockRulesSniffing) return NS_OK;
>+  if (mLockRulesSniffing) {
>+    return NS_OK;
>+  }
>+
>+  NS_ENSURE_STATE(mHTMLEditor);
>+  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);
> 
>   nsAutoLockRulesSniffing lockIt(this);
> 
>-  NS_PRECONDITION(mActionNesting>0, "bad action nesting!");
>+  MOZ_ASSERT(mActionNesting > 0);
>   nsresult res = NS_OK;

I hope that you rename |res| to |rv| here.

        Attachment #8739780 -
        Flags: review?(masayuki) → review+

    
    

    
  
> If you're ready to use MozReview, please use it. The diff tools in MozReview is useful/helpful
> especially for refactoring like this.

Er, I meant that please use it in new bugs. Not for this because changing the review tool cannot check the difference between the patches.

    
    

    
  
Comment on attachment 8739781 [details] [diff] [review]
Part 12 -- Clean up nsHTMLEditRules::GetAlignment

>-  if (selection->Collapsed()) {
>-    // if it is, we want to look at 'parent' and its ancestors
>-    // for divs with alignment on them
>-    nodeToExamine = parent;
>-  }
>-  else if (!mHTMLEditor) {
>-    return NS_ERROR_UNEXPECTED;
>-  }
>-  else if (mHTMLEditor->IsTextNode(parent))
>-  {
>-    // if we are in a text node, then that is the node of interest
>+  if (selection->Collapsed() || parent->GetAsText()) {

You actually change the logic but looks fine. Handling this method without mHTMLEditor is odd.

>+    nsresult res = GetNodesForOperation(arrayOfRanges, arrayOfNodes,
>+                                        EditAction::align, TouchContent::no);

Please use |rv| rather than |res|.

        Attachment #8739781 -
        Flags: review?(masayuki) → review+

    
    

    
  
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #48)
> >-  WillInsert(*aSelection, aCancel);
> >+  NS_ENSURE_STATE(mHTMLEditor);
> 
> Although, NS_ENSURE_* is deprecated and I like to use NS_WARN_IF() style.
> But fine.

Either way is fine by me, but Ehsan prefers NS_ENSURE_* last I checked, so I was using it in editor.  All of editor consistently uses NS_ENSURE_* right now.

> 
> >+  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);
> > 
> >-  // initialize out param
> >-  // we want to ignore result of WillInsert()
> >+  WillInsert(aSelection, aCancel);
> 
> nit: I wonder don't we need to check mHTMLEditor here? Looks like mBogusNode
> will be removed. And you removed some checks of mHTMLEditor from following
> code.

Ehsan told me that as long as I have the kungFuDeathGrip, I shouldn't need to check mHTMLEditor.  The kungFuDeathGrip keeps the reference count above zero, and apparently the only thing that should null mHTMLEditor is the destructor (or CC code) of nsPlaintextEditor.  (I see a call to DetachEditor in nsPlaintextEditor::Init also, but I guess that should never get called once the editor is started.)  So I've been replacing all these checks with kungFuDeathGrip, and Ehsan has been giving me r+.

> >-    if (nsTextEditUtils::IsBreak(theNode))
> >+    if (node->IsHTMLElement(nsGkAtoms::br)) {
> 
> I don't like this change. Until nsTextEditUtils::IsBreak() is removed, all
> developers expects adding new rule to this method changes whole behavior of
> similar code. So, please keep current code here until you'll remove the
> utility method.

I think if IsBreak() returned true for things other than <br>, it would be very confusing!  I think the only reason this method existed was because without it, originally you'd have to write

  nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("br"))

which is very long.  Since now we use nsINode, and it has an IsHTMLElement method which is about the same length to type, I think we should use that instead and phase out these methods, unless we think we're really going to add more things in the future to IsBreak.  Ehsan asked that I not remove nsHTMLEditUtils::IsImage, because we might want to add other things in the future like <picture>, but there's no reason we should ever want to add anything to IsBreak.

Asking for needinfo to request reconsideration of this change.

> However, looks like that you should add |!emptyDiv &&| here.

This code is only run if nodeArray.Length() == 1, and emptyDiv is set to nodeArray.IsEmpty(), so it should always be false.  Correct me if I'm wrong!

> >+  if (emptyDiv) {
> >+    NS_ENSURE_STATE(aSelection.GetRangeAt(0) &&
> >+                    aSelection.GetRangeAt(0)->GetStartParent());
> >+    nsCOMPtr<nsINode> parent = aSelection.GetRangeAt(0)->GetStartParent();
> 
> Hmm, then, this should be:
> 
> nsCOMPtr<nsINode> parent =
>   aSelection.GetRangeAt(0) ? aSelection.GetRangeAt(0)->GetStartParent() :
>                              nullptr;
> NS_ENSURE_STATE(parent);

Even better if this were OwningNonNull, right?  (Then your proposed change wouldn't work.)

> If you're ready to use MozReview, please use it. The diff tools in MozReview
> is useful/helpful especially for refactoring like this.

I'll look at it for future patches I ask you to review.


I intend to check this in on Sunday, with one push to m-i per patch.  That way mozregression will be able to bisect regressions easily.  I used to push all at once, but they would cause regressions, and Ehsan had to spend hours building to bisect the regression, so he asked me to do it this way.  In #developers I was told to wait till Sunday so as not to overload the builders.
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to :Aryeh Gregor (working until May 8) from comment #53)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #48)
> > >-  WillInsert(*aSelection, aCancel);
> > >+  NS_ENSURE_STATE(mHTMLEditor);
> > 
> > Although, NS_ENSURE_* is deprecated and I like to use NS_WARN_IF() style.
> > But fine.
> 
> Either way is fine by me, but Ehsan prefers NS_ENSURE_* last I checked, so I
> was using it in editor.  All of editor consistently uses NS_ENSURE_* right
> now.

Yeah, right. The reasons why editor still uses NS_ENSURE_* are the two thing. But Ehsan stepped down from editor module owner. So, I think that ideally, we should rewrite NS_ENSURE_* with NS_WARN_IF since the new coding rules recommend the latter.

But for now, especially for consistency in editor code, it's fine to use NS_ENSURE_*.

> > >+  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);
> > > 
> > >-  // initialize out param
> > >-  // we want to ignore result of WillInsert()
> > >+  WillInsert(aSelection, aCancel);
> > 
> > nit: I wonder don't we need to check mHTMLEditor here? Looks like mBogusNode
> > will be removed. And you removed some checks of mHTMLEditor from following
> > code.
> 
> Ehsan told me that as long as I have the kungFuDeathGrip, I shouldn't need
> to check mHTMLEditor.  The kungFuDeathGrip keeps the reference count above
> zero, and apparently the only thing that should null mHTMLEditor is the
> destructor (or CC code) of nsPlaintextEditor.  (I see a call to DetachEditor
> in nsPlaintextEditor::Init also, but I guess that should never get called
> once the editor is started.)  So I've been replacing all these checks with
> kungFuDeathGrip, and Ehsan has been giving me r+.

Hmm, I worried about the nsPlaintextEditor::Init() case.

Could you check if it never runs at least our tests? You can test it with adding MOZ_CRASH() into the |if (mRules)| and run whole tests on tryserver.

If that doesn't cause crash, it's fine to remove the null checks. Otherwise, removing the null checks could cause new crashes, especially via mutation events.

> > >-    if (nsTextEditUtils::IsBreak(theNode))
> > >+    if (node->IsHTMLElement(nsGkAtoms::br)) {
> > 
> > I don't like this change. Until nsTextEditUtils::IsBreak() is removed, all
> > developers expects adding new rule to this method changes whole behavior of
> > similar code. So, please keep current code here until you'll remove the
> > utility method.
> 
> I think if IsBreak() returned true for things other than <br>, it would be
> very confusing!  I think the only reason this method existed was because
> without it, originally you'd have to write
> 
>   nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("br"))
> 
> which is very long.  Since now we use nsINode, and it has an IsHTMLElement
> method which is about the same length to type, I think we should use that
> instead and phase out these methods, unless we think we're really going to
> add more things in the future to IsBreak.  Ehsan asked that I not remove
> nsHTMLEditUtils::IsImage, because we might want to add other things in the
> future like <picture>, but there's no reason we should ever want to add
> anything to IsBreak.

Yeah, right. However, I'd still you not to change it in the patch. If you really want to change them, could you remove nsTextEditUtils::IsBreak() completely with additional patch?

> > However, looks like that you should add |!emptyDiv &&| here.
> 
> This code is only run if nodeArray.Length() == 1, and emptyDiv is set to
> nodeArray.IsEmpty(), so it should always be false.  Correct me if I'm wrong!

Oh, sorry, I realized that during the review, but I just forgot to remove it before posting. So, it's my wrong, please ignore this.

> > >+  if (emptyDiv) {
> > >+    NS_ENSURE_STATE(aSelection.GetRangeAt(0) &&
> > >+                    aSelection.GetRangeAt(0)->GetStartParent());
> > >+    nsCOMPtr<nsINode> parent = aSelection.GetRangeAt(0)->GetStartParent();
> > 
> > Hmm, then, this should be:
> > 
> > nsCOMPtr<nsINode> parent =
> >   aSelection.GetRangeAt(0) ? aSelection.GetRangeAt(0)->GetStartParent() :
> >                              nullptr;
> > NS_ENSURE_STATE(parent);
> 
> Even better if this were OwningNonNull, right?  (Then your proposed change
> wouldn't work.)

If the variable type is OwningNonNull, checking before initializing is fine like others. (Although, I hope that compiler optimizes the redundant calls...)

> I intend to check this in on Sunday, with one push to m-i per patch.  That
> way mozregression will be able to bisect regressions easily.  I used to push
> all at once, but they would cause regressions, and Ehsan had to spend hours
> building to bisect the regression, so he asked me to do it this way.  In
> #developers I was told to wait till Sunday so as not to overload the
> builders.

Yeah, it's the best approach.
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #54)
> Hmm, I worried about the nsPlaintextEditor::Init() case.
> 
> Could you check if it never runs at least our tests? You can test it with
> adding MOZ_CRASH() into the |if (mRules)| and run whole tests on tryserver.
> 
> If that doesn't cause crash, it's fine to remove the null checks. Otherwise,
> removing the null checks could cause new crashes, especially via mutation
> events.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6b2f6c17492

If this is green, we should probably get rid of the mRules check and make it a MOZ_ASSERT instead.

> Yeah, right. However, I'd still you not to change it in the patch. If you
> really want to change them, could you remove nsTextEditUtils::IsBreak()
> completely with additional patch?

There are still some callers of the nsIDOMNode* variant, which can't be changed until they're converted to nsINode*.  Do you want me to remove just the nsINode* variant?

> If the variable type is OwningNonNull, checking before initializing is fine
> like others. (Although, I hope that compiler optimizes the redundant
> calls...)

I hope so too, but editor isn't performance-sensitive at all, so I don't worry about microoptimization.
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to :Aryeh Gregor (working until May 8) from comment #55)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #54)
> > Hmm, I worried about the nsPlaintextEditor::Init() case.
> > 
> > Could you check if it never runs at least our tests? You can test it with
> > adding MOZ_CRASH() into the |if (mRules)| and run whole tests on tryserver.
> > 
> > If that doesn't cause crash, it's fine to remove the null checks. Otherwise,
> > removing the null checks could cause new crashes, especially via mutation
> > events.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6b2f6c17492
> 
> If this is green, we should probably get rid of the mRules check and make it
> a MOZ_ASSERT instead.

I don't think that we should remove the mRules check because the tests have not tested all cases. I think that you should just add MOZ_ASSERT() in the if block instead. Then, somebody may find the case later.

> > Yeah, right. However, I'd still you not to change it in the patch. If you
> > really want to change them, could you remove nsTextEditUtils::IsBreak()
> > completely with additional patch?
> 
> There are still some callers of the nsIDOMNode* variant, which can't be
> changed until they're converted to nsINode*.  Do you want me to remove just
> the nsINode* variant?

So, my original suggestion is, you shouldn't get rid of nsTextEditUtils::IsBreak() from the method. Instead, you should get rid of them with an independent patch (or bug).

> > If the variable type is OwningNonNull, checking before initializing is fine
> > like others. (Although, I hope that compiler optimizes the redundant
> > calls...)
> 
> I hope so too, but editor isn't performance-sensitive at all, so I don't
> worry about microoptimization.

Yeah, up to you.
Flags: needinfo?(masayuki)

    
    

    
  


  
New try run to test the whole patchset, plus the testing for mRules: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb087aa6f22b

        Attachment #8739778 -
        Attachment is obsolete: true

        Attachment #8740923 -
        Flags: review?(masayuki)

    
    

    
  
Ehsan, I think you told me that if I have a kungFuDeathGrip on mHTMLEditor in an nsHTMLEditRules method, it was safe to remove all the NS_ENSURE_STATE(mHTMLEditor)s.  But it seems nsPlaintextEditor::Init() can call mRules->DetachEditor(), and this code path is actually hit in practice:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb087aa6f22b (see last patch)

So is this really safe?  If so, why?  Do we know somehow that nsPlaintextEditor::Init() will never get called when nsHTMLEditRules methods are in the middle of executing?
Flags: needinfo?(ehsan)

    
    

    
  
Comment on attachment 8740923 [details] [diff] [review]
Patch part 9, v2 - Clean up nsHTMLEditRules::WillAlign

Looks good except mHTMLEditor issue. r+ if you revert the null checks or add follow up patch to this bug.

        Attachment #8740923 -
        Flags: review?(masayuki) → review+

    
    

    
  
(In reply to :Aryeh Gregor (working until May 8) from comment #58)
> Ehsan, I think you told me that if I have a kungFuDeathGrip on mHTMLEditor
> in an nsHTMLEditRules method, it was safe to remove all the
> NS_ENSURE_STATE(mHTMLEditor)s.

I think that's OK, yes.

> But it seems nsPlaintextEditor::Init() can
> call mRules->DetachEditor(), and this code path is actually hit in practice:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb087aa6f22b (see
> last patch)
> 
> So is this really safe?  If so, why?  Do we know somehow that
> nsPlaintextEditor::Init() will never get called when nsHTMLEditRules methods
> are in the middle of executing?

Here we are re-initializing a plaintext editor object right?

As far as I know, that's the only case where an editor object can get initialized more than once.
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #60)
> (In reply to :Aryeh Gregor (working until May 8) from comment #58)
> > Ehsan, I think you told me that if I have a kungFuDeathGrip on mHTMLEditor
> > in an nsHTMLEditRules method, it was safe to remove all the
> > NS_ENSURE_STATE(mHTMLEditor)s.
> 
> I think that's OK, yes.

I don't think so as the results of tests on tryserver. At least, shouldn't we do this?

nsCOMPtr<nsIHTMLEditor> htmlEditor(mHTMLEditor);

and

- mHTMLEditor->FooBar();
+ htmlEditor->FooBar();
Flags: needinfo?(ehsan)

    
    

    
  
I also don't know if or why this pattern is safe, but there are a bunch of places that use it even before this whole series landed, like:

http://hg.mozilla.org/mozilla-central/file/ae7413abfa4d/editor/libeditor/nsHTMLEditRules.cpp#l4966
http://hg.mozilla.org/mozilla-central/file/ae7413abfa4d/editor/libeditor/nsHTMLEditRules.cpp#l5712
http://hg.mozilla.org/mozilla-central/file/ae7413abfa4d/editor/libeditor/nsHTMLEditRules.cpp#l5797
http://hg.mozilla.org/mozilla-central/file/ae7413abfa4d/editor/libeditor/nsHTMLEditRules.cpp#l5962

And ~7 more.  Plus other places in these unlanded bugs that Ehsan already gave r+ to.  So if it does need to be fixed, we need to make a new bug anyway to fix all the existing cases.  Given that, are you okay with landing this as-is, and if it turns out to be a problem, fixing it later?  This patch series is blocking other editor work, so I'd like to get it landed.
Flags: needinfo?(masayuki)

    
    

    
  
Sounds reasonable. Go ahead.
Flags: needinfo?(masayuki)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 5 updated
        — 
          — Splinter Review
      

    


  

        Attachment #8641197 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 6 updated
        — 
          — Splinter Review
      

    


  

        Attachment #8641198 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 7 updated
        — 
          — Splinter Review
      

    


  

        Attachment #8641202 -
        Attachment is obsolete: true

    
    

    
  
Backed out per bug 1191356 comment 23.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Whiteboard: [leave open]

    
    

    
  
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f0837841e7dff6a9c5ecc26ecb8de5e9a0c07e77
> Backout changeset bf13f411417b (bug 1156062 part 12)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/65a2aa44161b485ca727a6be66249b8ae9b3940b
> Backout changeset e9577aa83ab0 (bug 1156062 part 11)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c604e054da8721ce9afe831cf27e097bd86e77de
> Backout changeset a530f2e280a4 (bug 1156062 part 10)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2102e672350f76dc86fbfdcb14b2432578a52d27
> Backout changeset 1e142031ab51 (bug 1156062 part 9)

I and/or Aryeh should've test to build on tryserver. Sigh...

Anway, I landed ~ part 8.

Aryeh: Please fix the bugstage in part.9 and land following patches everyday separately.
Flags: needinfo?(ayg)
Whiteboard: [leave open]

    
    

    
  
I did test the whole series on the try server, several times, most recently: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e156c70327  Maybe it was bustage in one patch that was fixed in a subsequent patch.  I did locally compile every intermediate revision, though, IIRC.

It helps if you give a link to the bustage on treeherder so I can identify what broke . . .
Flags: needinfo?(ayg)

    
    

    
  
I'm a little confused here. Did parts 9, 10, 11, 12 get landed or not?
The links in comment #96 suggest they did. That would mean that the bug is done.
Please clarify before you disappear until August ;-)
Flags: needinfo?(ayg)

    
    

    
  
Thanks for pointing it out.  The bug is fixed, but wasn't marked resolved because "[Leave open]" was accidentally left in the whiteboard.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Flags: needinfo?(ayg)
Resolution: --- → FIXED
Whiteboard: [leave open]

    
    

    
  
You forgot the version ;-) And I don't think Ehsan needs to answer anything any more.
Flags: needinfo?(ehsan)
Target Milestone: --- → mozilla49

    
    

    
  
Masayuki still has a needinfo for Ehsan in comment 61.
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to :Aryeh Gregor (away until August 15) from comment #91)
> I did test the whole series on the try server, several times, most recently:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e156c70327  Maybe
> it was bustage in one patch that was fixed in a subsequent patch.  I did
> locally compile every intermediate revision, though, IIRC.
> 
> It helps if you give a link to the bustage on treeherder so I can identify
> what broke . . .

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1e142031ab51ed18a6526766707151fdc9e8ed71

(In reply to Jorg K (PTO during summer, NI me) from comment #97)
> I'm a little confused here. Did parts 9, 10, 11, 12 get landed or not?
> The links in comment #96 suggest they did. That would mean that the bug is
> done.
> Please clarify before you disappear until August ;-)

That's odd. I backed out them from m-i here:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2102e672350f76dc86fbfdcb14b2432578a52d27

    
    

    
  
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #102)
> Hmm, but looks like the part.9's change was applied to m-c. Why??
> https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/
> nsHTMLEditRules.cpp#128,136

Oh, you've already relanded at comment 92~. Sorry for the spam and thank you for your work!

    
    

    
  
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #61)
> (In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment
> #60)
> > (In reply to :Aryeh Gregor (working until May 8) from comment #58)
> > > Ehsan, I think you told me that if I have a kungFuDeathGrip on mHTMLEditor
> > > in an nsHTMLEditRules method, it was safe to remove all the
> > > NS_ENSURE_STATE(mHTMLEditor)s.
> > 
> > I think that's OK, yes.
> 
> I don't think so as the results of tests on tryserver. At least, shouldn't
> we do this?
> 
> nsCOMPtr<nsIHTMLEditor> htmlEditor(mHTMLEditor);

That variable _is_ the kungfu death grip that Aryeh was mentioning.  The idea behind doing this is to increment the refcount of the HTML editor object in the beginning of some code and decrementing it at the end in the destructor for the smart pointer.  This way you can ensure that the HTML editor object won't accidentally get destroyed as a result of some operation that you call into ending up releasing all of the remaining references to the HTML editor object and therefore freeing it before you're done using it.

After you have a kungfu death grip, you'll guarantee that the object is going to be alive for the rest of the method, which helps eliminate some null checks such as the one that Aryeh was mentioning.  Hope this helps clear the ideas behind this setup.

> and
> 
> - mHTMLEditor->FooBar();
> + htmlEditor->FooBar();

Once you make sure that the HTML editor object is alive per above, it won't really make any difference which one of these two variables you'd use to call FooBar() on, so you can go either way (either call methods on the kungfu death grip or on the original variable -- they both should point to the same object at runtime anyway.)
Flags: needinfo?(ehsan)

    
    

    
  
I think Masayuki's question was -- what if DetachEditor() gets called on the edit rules object somehow in the interim (such as from nsPlaintextEditor::Init)?  Then the kungFuDeathGrip is no longer holding a reference to mHTMLEditor, it's holding a reference to some other nsHTMLEditor object.  So why can't mHTMLEditor now have its refcount reduced to 0?  If you called all methods on the htmlEditRules variable that you're holding, then you'd know its refcount is nonzero.
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to :Aryeh Gregor (away until August 15) from comment #105)
> I think Masayuki's question was -- what if DetachEditor() gets called on the
> edit rules object somehow in the interim (such as from
> nsPlaintextEditor::Init)?  Then the kungFuDeathGrip is no longer holding a
> reference to mHTMLEditor, it's holding a reference to some other
> nsHTMLEditor object.  So why can't mHTMLEditor now have its refcount reduced
> to 0?  If you called all methods on the htmlEditRules variable that you're
> holding, then you'd know its refcount is nonzero.

Sure, but that is only a hypothetical scenario that can *never* happen in practice.  As I said in comment 60, only plaintext editors can be reinitialized, and even those should never be reinitialized while an editing operation is in progress.

If there are cases where these invariants don't hold true, those are serious bugs that we should fix, but it's fairly trivial to prove via code inspection that those invariants are indeed true.

Perhaps we should add some explicit assertion in nsHTMLEditor to ensure that it can never be reinitialized?
Flags: needinfo?(ehsan)
Depends on: 1266882

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