1433849 - Remove unused methods in nsIHTMLAbsPosEditor

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

The following methods is unused in m-c, c-c and bluegriffon from Java Script.

- selectionContainerAbsolutelyPositioned
- absolutePositionSelection
- relativeChangeZIndex
- absolutelyPositionElement
- setElementPosition
- getElementZIndex
- setElementZIndex
- relativeChangeElementZIndex
- showGrabberOnElement
- hideGrabber

So let't remove these method from IDL, then moving to c++ only code.

Comment 1

[Makoto Kato [:m_kato]]

also, absolutelyPositionedSelectionContainer is unused.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1433849 - Remove unused methods in nsIHTMLAbsPosEditor.

The following methods are unused in m-c, c-c and bluegriffon from JavaScript.

- selectionContainerAbsolutelyPositioned
- absolutelyPositionedSelectionContainer
- absolutePositionSelection
- relativeChangeZIndex
- absolutelyPositionElement
- setElementPosition
- getElementZIndex
- setElementZIndex
- relativeChangeElementZIndex
- showGrabberOnElement
- hideGrabber

So let't remove these method from IDL.

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

Comment 3

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

Comment on attachment 8947350 [details]
Bug 1433849 - Remove unused methods in nsIHTMLAbsPosEditor.

https://reviewboard.mozilla.org/r/217090/#review222916

If you think that renaming methods should be done in a follow up bug, it's okay because it may be better to rename more methods for consistency.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:55
(Diff revision 1)
>  using namespace dom;
>  
>  #define  BLACK_BG_RGB_TRIGGER 0xd0
>  
> -NS_IMETHODIMP
> +nsresult
>  HTMLEditor::AbsolutePositionSelection(bool aEnabled)

Perhaps, this should be renamed to SetPositionToAbsoluteOrStatic() for consistency with following method.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:116
(Diff revision 1)
> -NS_IMETHODIMP
> -HTMLEditor::RelativeChangeElementZIndex(nsIDOMElement* aElement,
> +nsresult
> +HTMLEditor::RelativeChangeElementZIndex(Element& aElement,
> -                                          int32_t aChange,
> +                                        int32_t aChange,
> -                                          int32_t* aReturn)
> +                                        int32_t* aReturn)
>  {
> -  NS_ENSURE_ARG_POINTER(aElement);
>    NS_ENSURE_ARG_POINTER(aReturn);
>    if (!aChange) // early way out, no change
>      return NS_OK;
>  
>    int32_t zIndex;
>    nsresult rv = GetElementZIndex(aElement, &zIndex);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    zIndex = std::max(zIndex + aChange, 0);
>    SetElementZIndex(aElement, zIndex);
>    *aReturn = zIndex;
>  
>    return NS_OK;
>  }

If GetElementZIndex() won't return error, this should just return z-index, instead of nsresult and using out-param. But it's okay to do it in another bug.  Perhaps, renaming to |AddZIndex()| is easier to read.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:136
(Diff revision 1)
> -NS_IMETHODIMP
> -HTMLEditor::SetElementZIndex(nsIDOMElement* aElement,
> +nsresult
> +HTMLEditor::SetElementZIndex(Element& aElement,
>                               int32_t aZindex)
>  {
> -  nsCOMPtr<Element> element = do_QueryInterface(aElement);
> -  NS_ENSURE_ARG_POINTER(element);
> -
>    nsAutoString zIndexStr;
>    zIndexStr.AppendInt(aZindex);
>  
> -  mCSSEditUtils->SetCSSProperty(*element, *nsGkAtoms::z_index, zIndexStr);
> +  mCSSEditUtils->SetCSSProperty(aElement, *nsGkAtoms::z_index, zIndexStr);
>    return NS_OK;
>  }

This always returns NS_OK. So, please make it void. And perhaps, |SetZIndex()| is enough name but renaming it is okay to put off until renaming other related methods for consistency with them.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:148
(Diff revision 1)
> -  mCSSEditUtils->SetCSSProperty(*element, *nsGkAtoms::z_index, zIndexStr);
> +  mCSSEditUtils->SetCSSProperty(aElement, *nsGkAtoms::z_index, zIndexStr);
>    return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> +nsresult
>  HTMLEditor::RelativeChangeZIndex(int32_t aChange)

So, perhaps, |AddZIndex()| is better name but up to you if renaming this in another bug.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:173
(Diff revision 1)
>  nsresult
> -HTMLEditor::GetElementZIndex(Element* aElement,
> +HTMLEditor::GetElementZIndex(Element& aElement,
>                               int32_t* aZindex)
>  {
> -  if (NS_WARN_IF(!aElement)) {
> -    return NS_ERROR_INVALID_ARG;
> -  }
> -
>    nsAutoString zIndexStr;
>    *aZindex = 0;
>  
>    nsresult rv =
> -    mCSSEditUtils->GetSpecifiedProperty(*aElement, *nsGkAtoms::z_index,
> +    mCSSEditUtils->GetSpecifiedProperty(aElement, *nsGkAtoms::z_index,
>                                          zIndexStr);
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (zIndexStr.EqualsLiteral("auto")) {
>      // we have to look at the positioned ancestors
>      // cf. CSS 2 spec section 9.9.1
> -    nsCOMPtr<nsINode> node = aElement->GetParentNode();
> +    nsCOMPtr<nsINode> node = aElement.GetParentNode();
>      nsAutoString positionStr;
>      while (node && zIndexStr.EqualsLiteral("auto") &&
>             !node->IsHTMLElement(nsGkAtoms::body)) {
>        rv = mCSSEditUtils->GetComputedProperty(*node, *nsGkAtoms::position,
>                                                positionStr);
>        NS_ENSURE_SUCCESS(rv, rv);
>        if (positionStr.EqualsLiteral("absolute")) {
>          // ah, we found one, what's its z-index ? If its z-index is auto,
>          // we have to continue climbing the document's tree
>          rv = mCSSEditUtils->GetComputedProperty(*node, *nsGkAtoms::z_index,
>                                                  zIndexStr);
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
>        node = node->GetParentNode();
>      }
>    }
>  
>    if (!zIndexStr.EqualsLiteral("auto")) {
>      nsresult errorCode;
>      *aZindex = zIndexStr.ToInteger(&errorCode);
>    }
>  
>    return NS_OK;
>  }

It's okay to do in another bug though, looks like that GetElementZIndex() should just return z-index value since this doesn't handle error code in some cases, e.g., if aElement is an orphan, this returns 0 without error.  So, this just returns error only when style system doesn't work properly.  I guess that anyway, we cannot handle anything as expected in such case. And perhaps, |GetZIndex()| is enough (and can we make it const method?).

::: editor/libeditor/HTMLAbsPositionEditor.cpp:256
(Diff revision 1)
> -NS_IMETHODIMP
> +nsresult
>  HTMLEditor::HideGrabber()
>  {
>    nsresult rv = mAbsolutelyPositionedObject->UnsetAttr(kNameSpaceID_None,
>                                                         nsGkAtoms::_moz_abspos,
>                                                         true);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    mAbsolutelyPositionedObject = nullptr;
>    NS_ENSURE_TRUE(mGrabber, NS_ERROR_NULL_POINTER);
>  
>    // get the presshell's document observer interface.
>    nsCOMPtr<nsIPresShell> ps = GetPresShell();
>    // We allow the pres shell to be null; when it is, we presume there
>    // are no document observers to notify, but we still want to
>    // UnbindFromTree.
>  
>    DeleteRefToAnonymousNode(Move(mGrabber), ps);
>    DeleteRefToAnonymousNode(Move(mPositioningShadow), ps);
>  
>    return NS_OK;
>  }

I think that it's really irregular case of failing to unset attribute and if mGrabber is nullptr, we don't need to anything. So, I think that the result can be void.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:280
(Diff revision 1)
> -  NS_ENSURE_ARG_POINTER(element);
> -  return ShowGrabberOnElement(*element);
> -}
> -
>  nsresult
>  HTMLEditor::ShowGrabberOnElement(Element& aElement)

Perhaps, just |ShowGrabber()| is enough.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:286
(Diff revision 1)
>    if (mGrabber) {
>      NS_ERROR("call HideGrabber first");
>      return NS_ERROR_UNEXPECTED;
>    }

I wonder, do we need to return error when grabber is visible? (out of scope of this bug.)

::: editor/libeditor/HTMLAbsPositionEditor.cpp:455
(Diff revision 1)
>    aX += positioningOffset;
>    aY += positioningOffset;
>  }
>  
> -NS_IMETHODIMP
> -HTMLEditor::AbsolutelyPositionElement(nsIDOMElement* aElement,
> +nsresult
> +HTMLEditor::AbsolutelyPositionElement(Element& aElement,

Perhaps, |SetPositionToAbsoluteOrStatic()| is better name and the callers are separated. So, making this private and wrap with inline public methods |SetPositionToAbsolute()| and |SetPositionToStatic()|.

::: editor/libeditor/HTMLAbsPositionEditor.cpp:556
(Diff revision 1)
> -  SetElementPosition(*element, aX, aY);
> -  return NS_OK;
> -}
> -
>  void
>  HTMLEditor::SetElementPosition(Element& aElement,

Perhaps, |SetTopAndLeft()| is better.

::: editor/libeditor/HTMLEditRules.cpp:9467
(Diff revision 1)
>  {
> +  if (!mNewBlock) {
> +    return NS_OK;
> +  }
>    NS_ENSURE_STATE(mHTMLEditor);
> -  nsCOMPtr<nsIHTMLAbsPosEditor> absPosHTMLEditor = mHTMLEditor;
> +  return mHTMLEditor->AbsolutelyPositionElement(*mNewBlock, true);

Hmm, you makes this stop grabbing mHTMLEditor with a local strong pointer. But probably, it's dangerous. The method changes CSS property and might change DOM tree. So, it could be interrupted by JS and make mHTMLEditor destroyed.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e1fd211eca
Remove unused methods in nsIHTMLAbsPosEditor. r=masayuki

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/66e1fd211eca