Boris Zbarsky [:bzbarsky] Assignee
	Description • 6 years ago

Jorg, this is going to need some Thunderbird updates for the changes to editor/nsIEditorMailSupport.idl

	Jorg K (CEST = GMT+2)
	Comment 1 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 5 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 6 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 7 • 6 years ago

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>.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 8 • 6 years ago

FYI: I landed my patches for bug1463327 to autoland.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 6 years ago

> 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.

	Pulsebot
	Comment 10 • 6 years ago

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

	Cristian Brindusan [:cbrindusan]
	Comment 11 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/166058ce19a3
https://hg.mozilla.org/mozilla-central/rev/96a208d3ccee
https://hg.mozilla.org/mozilla-central/rev/7c12d4b98d22