Clean up CheckSelectionStateForAnonymousButtons

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Since CheckSelectionStateForAnonymousButtons is called from selection listener, I would like to reduce some QI for this.
(Assignee)

Updated

a year ago
Blocks: 1433849
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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 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.
Attachment #8946551 - Flags: review?(masayuki) → review+
(Assignee)

Comment 4

a year ago
mozreview-review-reply
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 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

a year ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c575964fb57
Clean up CheckSelectionStateForAnonymousButtons. r=masayuki

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c575964fb57
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.