Yet more editor cleanup

RESOLVED FIXED in Firefox 48

Status

()

Core
Editor
--
enhancement
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(15 attachments, 17 obsolete attachments)

4.11 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
25.81 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
18.42 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
8.31 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
5.81 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
15.84 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
21.41 KB, patch
Details | Diff | Splinter Review
8.10 KB, patch
Details | Diff | Splinter Review
11.41 KB, patch
Details | Diff | Splinter Review
10.02 KB, patch
Details | Diff | Splinter Review
25.02 KB, patch
Details | Diff | Splinter Review
23.67 KB, patch
Details | Diff | Splinter Review
7.51 KB, patch
Details | Diff | Splinter Review
6.15 KB, patch
Details | Diff | Splinter Review
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.
Created attachment 8641107 [details] [diff] [review]
Part 1a -- New nsEditor::SplitNodeDeep variant
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)

Updated

2 years ago
Attachment #8641107 - Flags: review?(ehsan) → review+
Created attachment 8641116 [details] [diff] [review]
Patch part 1b - Convert some callers of nsEditor::SplitNodeDeep
Attachment #8641116 - Flags: review?(ehsan)

Updated

2 years ago
Attachment #8641116 - Flags: review?(ehsan) → review+
Created attachment 8641173 [details] [diff] [review]
Part 1c -- Convert remaining callers of nsEditor::SplitNodeDeep
Attachment #8641173 - Flags: review?(ehsan)
Created attachment 8641174 [details] [diff] [review]
Part 1c -- Convert remaining callers of nsEditor::SplitNodeDeep
Attachment #8641173 - Attachment is obsolete: true
Attachment #8641173 - Flags: review?(ehsan)
Attachment #8641174 - Flags: review?(ehsan)
Created attachment 8641179 [details]
Part 1d -- Clean up nsEditor
Created attachment 8641182 [details] [diff] [review]
Part 1d -- Clean up nsEditor::SplitNodeDeep

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)
Created attachment 8641183 [details] [diff] [review]
Part 1d -- Clean up nsEditor::SplitNodeDeep
Attachment #8641182 - Attachment is obsolete: true
Attachment #8641182 - Flags: review?(ehsan)
Attachment #8641183 - Flags: review?(ehsan)
Created attachment 8641184 [details] [diff] [review]
Part 2 -- Make GetBlockNodeParent and GetEnclosingTable return ordinary pointers

They won't have a refcount of one, so this is as safe as returning raw pointers ever is.
Attachment #8641184 - Flags: review?(ehsan)
Created attachment 8641186 [details] [diff] [review]
Part 3 -- New helper nsHTMLEditor::GetBlock

Maybe this should take nsIContent&.  I'm not sure with this sort of thing.
Attachment #8641186 - Flags: review?(ehsan)
Created attachment 8641192 [details] [diff] [review]
Part 4 -- Clean up ns*EditRules::WillInsert
Attachment #8641192 - Flags: review?(ehsan)
Created attachment 8641197 [details] [diff] [review]
Part 5 -- Make CreateBR return regular pointer

The refcount will still always be nonzero, because it's inserted into the DOM before being returned.
Attachment #8641197 - Flags: review?(ehsan)
Created attachment 8641198 [details] [diff] [review]
Part 6 -- Clean up nsHTMLEditRules::WillInsertBreak
Attachment #8641198 - Flags: review?(ehsan)

Comment 15

2 years ago
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...
Created attachment 8641202 [details] [diff] [review]
Part 7 -- Clean up nsHTMLEditRules::StandardBreakImpl
Attachment #8641202 - Flags: review?(ehsan)
Created attachment 8641209 [details] [diff] [review]
Part 8 -- Clean up nsHTMLEditRules::JoinBlocks
Attachment #8641209 - Flags: review?(ehsan)
Created attachment 8642165 [details] [diff] [review]
Part 9 -- Clean up nsHTMLEditRules::WilAlign
Attachment #8642165 - Flags: review?(ehsan)
Created attachment 8642166 [details] [diff] [review]
Part 10 -- Clean up MarginPropertyAtomForIndent
Attachment #8642166 - Flags: review?(ehsan)
Created attachment 8642167 [details] [diff] [review]
Part 11 -- Clean up nsHTMLEditRules::Before/AfterEdit
Attachment #8642167 - Flags: review?(ehsan)
Created attachment 8642168 [details] [diff] [review]
Part 12 -- Clean up nsHTMLEditRules::GetAlignment
Attachment #8642168 - Flags: review?(ehsan)
I'm pushing these patches one per day, by Ehsan's request.
Keywords: leave-open

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d52990c9dea8

Comment 24

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/18664c635ae0
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?]
https://hg.mozilla.org/mozilla-central/rev/d52990c9dea8
https://hg.mozilla.org/mozilla-central/rev/18664c635ae0

Updated

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

Comment 27

2 years ago
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+

Updated

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

Comment 28

2 years ago
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)

Comment 30

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/311d1b1ba00b

Comment 31

2 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/311d1b1ba00b

Comment 34

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e5c9314b72

Comment 35

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d83a21f9dd57

Comment 36

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/36543f58a3cf
https://hg.mozilla.org/mozilla-central/rev/74e5c9314b72
https://hg.mozilla.org/mozilla-central/rev/d83a21f9dd57
https://hg.mozilla.org/mozilla-central/rev/36543f58a3cf

Comment 38

2 years ago
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+

Updated

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

Updated

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

Updated

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

Comment 39

2 years ago
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 40

2 years ago
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 41

2 years ago
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 42

2 years ago
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)

Updated

2 years ago
Attachment #8641209 - Flags: review?(ehsan) → review+
See Also: → bug 1260651
Blocks: 1260651
Created attachment 8739778 [details] [diff] [review]
Part 9 -- Clean up nsHTMLEditRules::WillAlign

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)
Created attachment 8739779 [details] [diff] [review]
0007-Bug-1156062-part-10-Clean-up-MarginPropertyAtomForIn.patch
Attachment #8642166 - Attachment is obsolete: true
Attachment #8739779 - Flags: review?(ehsan)
Created attachment 8739780 [details] [diff] [review]
Part 11 -- Clean up nsHTMLEditRules::Before/AfterEdit
Attachment #8642167 - Attachment is obsolete: true
Attachment #8739780 - Flags: review?(ehsan)
Created attachment 8739781 [details] [diff] [review]
Part 12 -- Clean up nsHTMLEditRules::GetAlignment
Attachment #8642168 - Attachment is obsolete: true
Attachment #8739781 - Flags: review?(ehsan)

Comment 47

2 years ago
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)

Updated

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

Updated

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

Updated

2 years ago
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)
Created attachment 8740923 [details] [diff] [review]
Patch part 9, v2 - Clean up nsHTMLEditRules::WillAlign

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+

Comment 60

2 years ago
(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)
Created attachment 8742778 [details] [diff] [review]
Part 4 updated -- Clean up ns*EditRules::WillInsert
Attachment #8641192 - Attachment is obsolete: true
Created attachment 8742783 [details] [diff] [review]
Part 5 updated
Attachment #8641197 - Attachment is obsolete: true
Created attachment 8742788 [details] [diff] [review]
Part 6 updated
Attachment #8641198 - Attachment is obsolete: true
Created attachment 8742794 [details] [diff] [review]
Part 7 updated
Attachment #8641202 - Attachment is obsolete: true
Created attachment 8742799 [details] [diff] [review]
0005-Bug-1156062-part-8-Clean-up-nsHTMLEditRules-JoinBloc.patch
Attachment #8641209 - Attachment is obsolete: true
Created attachment 8742805 [details] [diff] [review]
0006-Bug-1156062-part-9-Clean-up-nsHTMLEditRules-WillAlig.patch
Attachment #8740923 - Attachment is obsolete: true
Created attachment 8742829 [details] [diff] [review]
0007-Bug-1156062-part-10-Clean-up-MarginPropertyAtomForIn.patch
Attachment #8739779 - Attachment is obsolete: true
Created attachment 8742833 [details] [diff] [review]
0008-Bug-1156062-part-11-Clean-up-nsHTMLEditRules-Before-.patch
Attachment #8739780 - Attachment is obsolete: true
Created attachment 8742837 [details] [diff] [review]
0009-Bug-1156062-part-12-Clean-up-nsHTMLEditRules-GetAlig.patch
Attachment #8739781 - Attachment is obsolete: true
Checkin instructions at bug 1191356 comment 20.
Keywords: leave-open → checkin-needed

Comment 74

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59109ec3822
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3195e53c53
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13c2d3edee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b36ab490c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f777b347e97f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdc241352d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f11856b7db
https://hg.mozilla.org/integration/mozilla-inbound/rev/13755b9cd5d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3732da92e8fc
Keywords: checkin-needed

Comment 75

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/94a006ee5874

Comment 76

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d59109ec3822
https://hg.mozilla.org/mozilla-central/rev/6e3195e53c53
https://hg.mozilla.org/mozilla-central/rev/e13c2d3edee7
https://hg.mozilla.org/mozilla-central/rev/46b36ab490c1
https://hg.mozilla.org/mozilla-central/rev/f777b347e97f
https://hg.mozilla.org/mozilla-central/rev/7cdc241352d4
https://hg.mozilla.org/mozilla-central/rev/54f11856b7db
https://hg.mozilla.org/mozilla-central/rev/13755b9cd5d9
https://hg.mozilla.org/mozilla-central/rev/3732da92e8fc
https://hg.mozilla.org/mozilla-central/rev/94a006ee5874
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 77

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ec158d3a91
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddcf4130768d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2cccdb88dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/c91b0d2f2ed9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9191bfcd54fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/227293880f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a31346e941ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5967250fc96
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa95332a325b
Backed out per bug 1191356 comment 23.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Merged backouts:
https://hg.mozilla.org/mozilla-central/rev/b7ec158d3a91
https://hg.mozilla.org/mozilla-central/rev/ddcf4130768d
https://hg.mozilla.org/mozilla-central/rev/0d2cccdb88dc
https://hg.mozilla.org/mozilla-central/rev/c91b0d2f2ed9
https://hg.mozilla.org/mozilla-central/rev/9191bfcd54fb
https://hg.mozilla.org/mozilla-central/rev/227293880f3e
https://hg.mozilla.org/mozilla-central/rev/a31346e941ae
https://hg.mozilla.org/mozilla-central/rev/a5967250fc96
https://hg.mozilla.org/mozilla-central/rev/aa95332a325b
Target Milestone: mozilla48 → ---
Whiteboard: [leave open]
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b3e9577b3cf35f8a8008af93adaff7dcb7f896
Bug 1156062 part 4 - Clean up ns*EditRules::WillInsert; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/42af5753ad68b7d06f0a8ba85d5b361f9ce37f6d
Bug 1156062 part 5 - Make CreateBR return regular pointer; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a6d2316a3b9972fb2cb77f82837bdd95283cc7
Bug 1156062 part 6 - Clean up nsHTMLEditRules::WillInsertBreak; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/02a332642272c61550235152f0e7ff89a139faac
Bug 1156062 part 7 - Clean up nsHTMLEditRules::StandardBreakImpl; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0cca233833e1cd574e7d73dcfa9d341c8b5e83
Bug 1156062 part 8 - Clean up nsHTMLEditRules::JoinBlocks; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e142031ab51ed18a6526766707151fdc9e8ed71
Bug 1156062 part 9 - Clean up nsHTMLEditRules::WillAlign
https://hg.mozilla.org/integration/mozilla-inbound/rev/a530f2e280a48ff03ffb45c0bb0eb5fedb26b2a0
Bug 1156062 part 10 - Clean up MarginPropertyAtomForIndent; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9577aa83ab06d2182cd88325a435ebd930e2ba1
Bug 1156062 part 11 - Clean up nsHTMLEditRules::Before/AfterEdit; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf13f411417bbb226757530162162215c5f964bc
Bug 1156062 part 12 - Clean up nsHTMLEditRules::GetAlignment; r=masayuki
Whiteboard: [leave open]
https://hg.mozilla.org/integration/mozilla-inbound/rev/645d28b3957926e969c8add1757ed6caebe108d6
Backed out due to bustage on all platforms (bug 1156062 (9-12) and bug 1190172 (1-12)
> 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)

Comment 92

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b08998731145

Comment 93

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9d679b1e77

Comment 94

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/19dfc13809d1

Comment 95

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ff6e7956f7

Comment 96

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b08998731145
https://hg.mozilla.org/mozilla-central/rev/0f9d679b1e77
https://hg.mozilla.org/mozilla-central/rev/19dfc13809d1
https://hg.mozilla.org/mozilla-central/rev/99ff6e7956f7

Comment 97

2 years ago
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
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(ayg)
Resolution: --- → FIXED
Whiteboard: [leave open]

Comment 99

2 years ago
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
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
(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!

Comment 104

2 years ago
(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)

Comment 106

2 years ago
(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.