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.

also, absolutelyPositionedSelectionContainer is unused.

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.

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/66e1fd211eca Remove unused methods in nsIHTMLAbsPosEditor. r=masayuki