Closed Bug 1088054 Opened 10 years ago Closed 10 years ago

Reduce usage of nsISelection and nsIDOMRange; clean up CheckForEmptyBlock, GetGoodSelPointForNode, GetTopEnclosingMailCite, GetFirst/LastEditableChild, JoinNodesSmart, GetFirst/LastEditableLeaf, JoinNodeDeep, WillDeleteSelection

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(10 files, 6 obsolete files)

79.88 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
8.41 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
7.06 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
9.51 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
6.85 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
14.70 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
157.96 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
18.84 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
8.43 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
45.55 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
      No description provided.
Flags: in-testsuite-
I didn't get rid of it entirely because some things get their selections directly from an nsISelectionController, and it uses an out param to return it, so it's a pain to put it in an nsRefPtr<Selection>.  (I couldn't figure out a way to do it without an intermediate variable, although I figure some sort of cast would probably work.)

Is there any way to add a method to nsISelectionController that returns an actual Selection?  Or better yet -- could we just get rid of nsISelection entirely and merge it into Selection, and leave nsISelection as a typedef or something?
Attachment #8510342 - Flags: review?(ehsan.akhgari)
I'm not totally convinced ETextType actually improves code readability in this case.
Attachment #8510372 - Flags: review?(ehsan.akhgari)
I changed the types of firstChild and lastChild from Element to nsIContent in AlignBlockContents, because using Element here was probably a bug introduced by <http://hg.mozilla.org/mozilla-central/rev/4439b62ad770>.  The added type safety in this patch caught it (yay).
Attachment #8510449 - Flags: review?(ehsan.akhgari)
Attachment #8510461 - Flags: review?(ehsan.akhgari)
This is expected to be my last patch until sometime after March 22.  I will be able to review your comments Sunday morning my time, so if you could finish by then it would be ideal for me, but of course I could also make time later in the week if necessary.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=767e7bafbd35
Attachment #8510478 - Flags: review?(ehsan.akhgari)
I'm uploading several updated patch versions to fix small bugs that caused try failures (mostly typos).
Attachment #8510342 - Attachment is obsolete: true
Attachment #8510342 - Flags: review?(ehsan.akhgari)
Attachment #8511003 - Flags: review?(ehsan.akhgari)
Attachment #8510372 - Attachment is obsolete: true
Attachment #8510372 - Flags: review?(ehsan.akhgari)
Attachment #8511005 - Flags: review?(ehsan.akhgari)
Attachment #8511003 - Attachment is obsolete: true
Attachment #8511003 - Flags: review?(ehsan.akhgari)
Attachment #8511007 - Flags: review?(ehsan.akhgari)
Attachment #8510449 - Attachment is obsolete: true
Attachment #8510449 - Flags: review?(ehsan.akhgari)
Attachment #8511008 - Flags: review?(ehsan.akhgari)
Attachment #8510461 - Attachment is obsolete: true
Attachment #8510461 - Flags: review?(ehsan.akhgari)
Attachment #8511009 - Flags: review?(ehsan.akhgari)
(In reply to :Aryeh Gregor from comment #1)
> Is there any way to add a method to nsISelectionController that returns an
> actual Selection?

Yes, you can declare natives in XPIDL.

> Or better yet -- could we just get rid of nsISelection
> entirely and merge it into Selection, and leave nsISelection as a typedef or
> something?

That probably has addon-compat issues...
Attachment #8511007 - Flags: review?(ehsan.akhgari) → review+
Attachment #8510354 - Flags: review?(ehsan.akhgari) → review+
Attachment #8510363 - Flags: review?(ehsan.akhgari) → review+
Attachment #8510367 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 8511005 [details] [diff] [review]
Part 5 v2 -- Clean up nsHTMLEditRules::GetTopEnclosingMailCite

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

::: editor/libeditor/nsHTMLEditRules.h
@@ +304,5 @@
>                             nsIDOMNode *aNodeRight, 
>                             nsCOMPtr<nsIDOMNode> *aOutMergeParent, 
>                             int32_t *aOutMergeOffset);
> +  mozilla::dom::Element* GetTopEnclosingMailCite(nsINode& aNode,
> +                                                 ETextType aTextType);

It seems to me that in both cases, you can drop the aTextType argument completely and just check IsPlaintextEditor() inside the implementation of these functions, as we never call them with a value differnet than IsPlaintextEditor().
Attachment #8511005 - Flags: review?(ehsan.akhgari) → review+
Attachment #8511008 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 8510452 [details] [diff] [review]
Part 7 -- Clean up nsHTMLEditRules::JoinNodesSmart

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

r=me with the checks below fixed.

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +2165,5 @@
>            // if so, join them!
> +          nsCOMPtr<nsIContent> sibling_ = do_QueryInterface(sibling);
> +          NS_ENSURE_STATE(sibling_ || !sibling);
> +          nsCOMPtr<nsIContent> startNode_ = do_QueryInterface(startNode);
> +          NS_ENSURE_STATE(startNode_ || !startNode);

Are these checks correct?  It seems like the derefs below can in theory crash if sibling or startNode are null.
Attachment #8510452 - Flags: review?(ehsan.akhgari) → review+
Attachment #8510455 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 8511009 [details] [diff] [review]
Part 9 v2 -- Clean up nsEditor::JoinNodeDeep

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

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +2428,5 @@
>          {
> +          nsCOMPtr<nsIContent> leftParent_ = do_QueryInterface(leftParent);
> +          NS_ENSURE_STATE(leftParent_ || !leftParent);
> +          nsCOMPtr<nsIContent> rightParent_ = do_QueryInterface(rightParent);
> +          NS_ENSURE_STATE(rightParent_ || !rightParent);

This doesn't seem correct either.
Attachment #8511009 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 8511010 [details] [diff] [review]
Part 10 v2 -- Clean up nsHTMLEditRules::WillDeleteSelection

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

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +2502,5 @@
> +  // selection if we'redeleting forward, because the end of the selection will
> +  // still be in the next block. And same thing for deleting backwards
> +  // (selection should collapse to the end, because the beginning will still be
> +  // in the first block). See Bug 507936
> +  if (join ? aAction == nsIEditor::eNext : aAction == nsIEditor::ePrevious) {

I'd rewrite this as:

if (aAction == (join ? nsIEditor::eNext ...)) {
Attachment #8511010 - Flags: review?(ehsan.akhgari) → review+
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #18)
> It seems to me that in both cases, you can drop the aTextType argument
> completely and just check IsPlaintextEditor() inside the implementation of
> these functions, as we never call them with a value differnet than
> IsPlaintextEditor().

Hah!  Good catch.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #19)
> Are these checks correct?  It seems like the derefs below can in theory
> crash if sibling or startNode are null.

sibling and startNode actually can't ever be null here, so it doesn't make a difference.  IsTextNode would have returned false if they were, so we wouldn't be in the if block.

When I convert nsIDOMNode to nsINode, I only return from the function if the conversion made the variable null, not if it was already null, because otherwise I'm changing behavior (and I have caused test failures before this way).  But I guess I shouldn't bother if a null variable would cause an obvious crash -- I won't break anything by adding a check then.
Depends on: 1092923
Depends on: 1130651
No longer depends on: 1130651
Depends on: 1146883
Depends on: 1348851
You need to log in before you can comment on or make changes to this bug.