Closed Bug 1408169 Opened 2 years ago Closed 2 years ago

Remove nsIDOMHTMLMenuItemElement

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Continuing post-addon-deprecation XPCOM interface cleanup
Priority: -- → P3
Comment on attachment 8918021 [details]
Bug 1408169 - Remove nsIDOMHTMLMenuItemElement;

https://reviewboard.mozilla.org/r/188908/#review199112

::: dom/html/HTMLMenuItemElement.h:83
(Diff revision 2)
>    void SetLabel(const nsAString& aLabel, ErrorResult& aError)
>    {
> -    SetAttrHelper(nsGkAtoms::label, aLabel);
> +    SetHTMLAttr(nsGkAtoms::label, aLabel, aError);
>    }
>  
> -  // The XPCOM GetIcon is OK for us
> +  // needed for HTMLMenuElement

"nsAString needed"?

::: dom/html/HTMLMenuItemElement.h:152
(Diff revision 2)
>    friend class ClearCheckedVisitor;
>    friend class SetCheckedDirtyVisitor;
>  
> +  void SetChecked(bool aChecked)
> +  {
> +    ErrorResult rv;

IgnoredErrorResult, right?

::: dom/html/HTMLMenuItemElement.cpp:215
(Diff revision 2)
> -  *aChecked = mChecked;
> +  GetEnumAttr(nsGkAtoms::type, kMenuItemDefaultType->tag, aValue);
> -  return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> -HTMLMenuItemElement::SetChecked(bool aChecked)
> +void
> +HTMLMenuItemElement::SetChecked(bool aChecked, ErrorResult& aError)

If we never throw on aError, then why do we need that arg at all?  That is, why is "checked" marked as [SetterThrows] in the webidl at all, if the setter can't throw?

Probably better to just remove that annotation, remove this aError arg, remove the new overload that was added.

::: dom/html/HTMLMenuItemElement.cpp:254
(Diff revision 2)
>          originalCheckedValue = mChecked;
>          SetChecked(!originalCheckedValue);
>          aVisitor.mItemFlags |= NS_CHECKED_IS_TOGGLED;
>          break;
>        case CMD_TYPE_RADIO:
> -        nsCOMPtr<nsIDOMHTMLMenuItemElement> selectedRadio = GetSelectedRadio();
> +        // casting back to nsIDOMNode here to resolve nsISupports ambiguity.

Please just use "Element*" instead:

    Element* supports = GetSelectedRadio();

::: dom/html/nsIMenuBuilder.idl:31
(Diff revision 2)
>     * Add a new menu item. All menu item details can be obtained from
>     * the element. This method is not called for hidden elements or elements
>     * with no or empty label. The icon should be loaded only if aCanLoadIcon
>     * is true.
>     */
> -  void addItemFor(in nsIDOMHTMLMenuItemElement aElement,
> +  void addItemFor(in nsIDOMNode aElement,

nsIDOMElement, please.
Attachment #8918021 - Flags: review?(bzbarsky) → review+
Didn't see bug 1372276 until after this passed review. I'll go ahead and land, since they're mostly just deleting the rest of this work anyways.
Blocks: 1372276
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c745952b8b76
Remove nsIDOMHTMLMenuItemElement; r=bz
https://hg.mozilla.org/mozilla-central/rev/c745952b8b76
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.