1434171 - Clean up CheckSelectionStateForAnonymousButtons

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Description

[Makoto Kato [:m_kato]]

Since CheckSelectionStateForAnonymousButtons is called from selection listener, I would like to reduce some QI for this.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1434171 - Clean up CheckSelectionStateForAnonymousButtons.

Since CheckSelectionStateForAnonymousButtons is called from selection listener,
We should reduce some QI for this.

Review commit: https://reviewboard.mozilla.org/r/216576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216576/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8946551 [details]
Bug 1434171 - Clean up CheckSelectionStateForAnonymousButtons.

https://reviewboard.mozilla.org/r/216576/#review222254


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLAbsPositionEditor.cpp:111
(Diff revision 1)
> -      *aContainer = nullptr;
> +      return nullptr;
> -      return rv;
>      }
> -    if (positionStr.EqualsLiteral("absolute"))
> -      resultNode = node;
> -    else {
> +    if (positionStr.EqualsLiteral("absolute")) {
> +      return element.forget();
> +    } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8946551 [details]
Bug 1434171 - Clean up CheckSelectionStateForAnonymousButtons.

https://reviewboard.mozilla.org/r/216576/#review222256

::: editor/libeditor/HTMLAbsPositionEditor.cpp:642
(Diff revision 1)
>    ret.forget(aReturn);
>    return NS_OK;
>  }
>  
>  nsresult
> -HTMLEditor::CheckPositionedElementBGandFG(nsIDOMElement* aElement,
> +HTMLEditor::CheckPositionedElementBGandFG(Element& aElement,

Could you rename this to |GetTemporaryStyleForFocusedPositionedElement()| or something? This result is set used for _moz_abspos attribute and it's styled as here:
https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/editor/composer/res/EditorOverride.css#165-174

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:330
(Diff revision 1)
>  // The following method is mostly called by a selection listener. When a
>  // selection change is notified, the method is called to check if resizing
>  // handles, a grabber and/or inline table editing UI need to be displayed
>  // or refreshed
>  NS_IMETHODIMP
>  HTMLEditor::CheckSelectionStateForAnonymousButtons(nsISelection* aSelection)

Could you rename this to |ShowAllContextualUIForSelection()| or something? |CheckSomething()| is really unclear name.

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8946551 [details]
Bug 1434171 - Clean up CheckSelectionStateForAnonymousButtons.

https://reviewboard.mozilla.org/r/216576/#review222256

> Could you rename this to |ShowAllContextualUIForSelection()| or something? |CheckSomething()| is really unclear name.

This is XPCOM method and BlueGriffon uses this method.  When I clean up nsIHTMLEditor IDL, I will consider better way.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8946551 [details]
Bug 1434171 - Clean up CheckSelectionStateForAnonymousButtons.

https://reviewboard.mozilla.org/r/216576/#review222256

> This is XPCOM method and BlueGriffon uses this method.  When I clean up nsIHTMLEditor IDL, I will consider better way.

Ah, okay to keep current name. If you create internal non-virtual method and wrap it with the XPCOM method, I'd like you to use better name.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c575964fb57
Clean up CheckSelectionStateForAnonymousButtons. r=masayuki

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4c575964fb57