Closed Bug 1463981 Opened Last year Closed Last year

Remove nsIDOMNode usage in editor

Categories

(Core :: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

Jorg, this is going to need some Thunderbird updates for the changes to editor/nsIEditorMailSupport.idl
Thanks Boris, I've been waiting for it, see bug 1458057 comment #3. I'll handle it over there. Looks like you've been waiting for 84-part bug 1460509.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8980190 [details] [diff] [review]
part 1.  Remove use of nsIDOMNode in spellchecker xpidl

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

::: editor/spellchecker/nsComposeTxtSrvFilter.cpp
@@ +33,3 @@
>                                                       nsGkAtoms::type,
>                                                       nsGkAtoms::cite,
>                                                       eIgnoreCase);

Please fix the indent of the 2nd ~4th arguments.

@@ +40,3 @@
>                                                       nsGkAtoms::mozquote,
>                                                       nsGkAtoms::_true,
>                                                       eIgnoreCase);

ditto.

@@ +45,3 @@
>                                                         nsGkAtoms::_class,
>                                                         nsGkAtoms::mozsignature,
>                                                         eCaseMatters);

ditto.
Attachment #8980190 - Flags: review?(masayuki) → review+
Comment on attachment 8980191 [details] [diff] [review]
part 2.  Remove use of nsIDOMNode in editor xpidl

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

::: editor/libeditor/EditorBase.cpp
@@ +1526,5 @@
>    if (!mActionListeners.IsEmpty()) {
>      AutoActionListenerArray listeners(mActionListeners);
>      for (auto& listener : listeners) {
> +      listener->DidSplitNode(aStartOfRightNode.GetContainer(),
> +                             newNode);

Please move |newNode);| to the end of the previous line.

@@ +1600,5 @@
>    if (!mActionListeners.IsEmpty()) {
>      AutoActionListenerArray listeners(mActionListeners);
>      for (auto& listener : listeners) {
> +      listener->DidJoinNodes(&aLeftNode, &aRightNode,
> +                             parent, rv);

Please move |parent, rv);| to the previous line.

::: editor/libeditor/HTMLURIRefObject.cpp
@@ +89,5 @@
> +  if (NS_WARN_IF(!mNode->IsElement())) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsCOMPtr<dom::Element> element = mNode->AsElement();

Please use RefPtr for Element if it won't receive do_QueryInterface's result.
Attachment #8980191 - Flags: review?(masayuki) → review+
Comment on attachment 8980192 [details] [diff] [review]
part 3.  Remove nsIDONNode usage in editor

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

Unfortunately, we'll land refine patches of EditorBase.h, HTMLEditor.h and TextEditor.h in bug 1463327. So, the 3rd patch will conflict with them and looks like that merging your patch to my patches is easier to rewrite my patches since my patches moves a lot of methods from public to protected. So, could you merge your patches tomorrow after bug 1463327 is fixed?

::: editor/libeditor/EditorBase.cpp
@@ +1274,5 @@
>      return NS_OK;
>    }
> +  if (aNode && aNode->IsElement()) {
> +    aNode->AsElement()->SetAttr(kNameSpaceID_None, nsGkAtoms::mozdirty,
> +                                EmptyString(), false);

I assume that setting the last argument to false won't cause firing mutation event. If not, please keep grabbing the node with RefPtr<Element>.

::: editor/libeditor/HTMLEditor.cpp
@@ +2592,4 @@
>          //  then get tagType to compare to  aTagName
>          // Clone node of each desired type and append it to the aDomFrag
>          nsINode* currentNode = iter->GetCurrentNode();
>          selectedElement = do_QueryInterface(currentNode);

Cannot this rewrite with
selectedElement = currentNode && currentNode->IsElement() ?
                    currentNode->AsElement() : nullptr;

(I assume that selectedElement is nsCOMPtr<Element>.)
Attachment #8980192 - Flags: review?(masayuki) → review+
FYI: I landed my patches for bug1463327 to autoland.
> Please fix the indent of the 2nd ~4th arguments.

Done all three places.

>Please move |newNode);| to the end of the previous line.
>Please move |parent, rv);| to the previous line.
>Please use RefPtr for Element if it won't receive do_QueryInterface's result.

All done.

> So, could you merge your patches tomorrow after bug 1463327 is fixed?

Yep, doing that.

> If not, please keep grabbing the node with RefPtr<Element>.

Will leave the RefPtr.  Safer that way.

> Cannot this rewrite with

Yes.  Done.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/166058ce19a3
part 1.  Remove use of nsIDOMNode in spellchecker xpidl.  r=masayuki
https://hg.mozilla.org/integration/autoland/rev/96a208d3ccee
part 2.  Remove use of nsIDOMNode in editor xpidl.  r=masayuki
https://hg.mozilla.org/integration/autoland/rev/7c12d4b98d22
part 3.  Remove nsIDOMNode usage in editor.  r=masayuki
You need to log in before you can comment on or make changes to this bug.