Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Description • 7 years ago

Continuing post-addon-deprecation XPCOM interface cleanup.

These interfaces depend on each other, so removing them both in the same bug.

	Code Review Bot [:reviewbot]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8926114 [details]
Bug 1408186 - Remove nsIDOMHTMLSelectElement and nsIDOMHTMLOptionsCollection;

https://reviewboard.mozilla.org/r/197348/#review202530


C/C++ static analysis found 0 defects in this patch.

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

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8926114 [details]
Bug 1408186 - Remove nsIDOMHTMLSelectElement and nsIDOMHTMLOptionsCollection;

https://reviewboard.mozilla.org/r/197348/#review202592

This is awesome.  ;)

::: browser/extensions/formautofill/FormAutofillHandler.jsm:378
(Diff revision 1)
>        if (!fieldDetail) {
>          continue;
>        }
>  
>        let element = fieldDetail.elementWeakRef.get();
> -      if (!(element instanceof Ci.nsIDOMHTMLSelectElement)) {
> +      if (!(ChromeUtils.getClassName(element) === "HTMLSelectElement")) {

Please use !== here instead.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:313
(Diff revision 1)
>     * @returns {boolean}
>     *          Return true if we observe the trait of month select in
>     *          the current element.
>     */
>    _isExpirationMonthLikely(element) {
> -    if (!(element instanceof Ci.nsIDOMHTMLSelectElement)) {
> +    if (!(ChromeUtils.getClassName(element) === "HTMLSelectElement")) {

!==

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:340
(Diff revision 1)
>     * @returns {boolean}
>     *          Return true if we observe the trait of year select in
>     *          the current element.
>     */
>    _isExpirationYearLikely(element) {
> -    if (!(element instanceof Ci.nsIDOMHTMLSelectElement)) {
> +    if (!(ChromeUtils.getClassName(element) === "HTMLSelectElement")) {

!==

::: dom/html/HTMLOptionsCollection.h:142
(Diff revision 1)
>    virtual Element*
>    GetFirstNamedElement(const nsAString& aName, bool& aFound) override
>    {
>      return NamedGetter(aName, aFound);
>    }
> -
> +  void GetSelect(HTMLSelectElement** aReturn);

Why is this needed?  I'm not seeing obvious callers for that function.

::: dom/html/HTMLOptionsCollection.h:154
(Diff revision 1)
>    void IndexedSetter(uint32_t aIndex, HTMLOptionElement* aOption,
>                       ErrorResult& aError);
>    virtual void GetSupportedNames(nsTArray<nsString>& aNames) override;
>  
> +
> +  NS_IMETHOD GetLength(uint32_t* aLength) override;

Why not NS_DECL_NSIDOMHTMLCOLLECTION, back where the comment for it was?

That said, can you please file a followup bug depending on this one to move the impl of nsIDOMHTMLCollection up to nsIHTMLCollection?  I think now that we're not shadowing it here, there is absolutely no reason not to do that. I'd be happy to fix that bug once this lands.  ;)

::: dom/html/HTMLOptionsCollection.cpp:122
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  HTMLOptionsCollection::SetLength(uint32_t aLength)

I think this entire method can go away.

HTMLOptionsCollection::IndexedSetter will need to be converted over to calling the WebIDL SetLength, but that's fine; it's working with an ErrorResult anyway.

::: dom/html/HTMLOptionsCollection.cpp:193
(Diff revision 1)
>    if (!mSelect) {
>      aError.Throw(NS_ERROR_UNEXPECTED);
>      return 0;
>    }
>  
> -  int32_t selectedIndex;
> +  int32_t selectedIndex = mSelect->SelectedIndex();

I'd just ditch the temporary:

    return mSelect->SelectedIndex();

::: dom/html/HTMLOptionsCollection.cpp:296
(Diff revision 1)
>    for (uint32_t i = 0; i < atomsLen; ++i) {
>      atoms[i]->ToString(names[i]);
>    }
>  }
>  
> -NS_IMETHODIMP
> +void

I believe this method can go away.

::: dom/html/HTMLSelectElement.h:252
(Diff revision 1)
>    }
>    HTMLOptionElement* NamedItem(const nsAString& aName) const
>    {
>      return mOptions->GetNamedItem(aName);
>    }
> +  nsresult Add(nsIDOMHTMLElement* aElement,

Is this actually used?  I bet it's not, with the HTMLOptionsCollection changes you made.

::: dom/html/HTMLSelectElement.cpp:602
(Diff revision 1)
>    nsCOMPtr<nsINode> refNode = aBefore;
>    parent->InsertBefore(aElement, refNode, aError);
>  }
>  
> -NS_IMETHODIMP
> +nsresult
>  HTMLSelectElement::Add(nsIDOMHTMLElement* aElement,

I suspect this can go away.

::: layout/forms/nsListControlFrame.cpp:1057
(Diff revision 1)
>  
>      // Scroll to the selected index
>      int32_t indexToSelect = kNothingSelected;
>  
> -    nsCOMPtr<nsIDOMHTMLSelectElement> selectElement(do_QueryInterface(mContent));
> +    HTMLSelectElement* selectElement = HTMLSelectElement::FromContent(mContent);
>      NS_ASSERTION(selectElement, "No select element!");

Might as well ditch that; FromContent will crash on null mContent anyway.  Not that we should have one here!

::: toolkit/modules/FormLikeFactory.jsm:67
(Diff revision 1)
>     * @return {FormLike}
>     * @throws Error if aField isn't a password or username field in a document
>     */
>    createFromField(aField) {
> -    if ((!(aField instanceof Ci.nsIDOMHTMLInputElement) && !(aField instanceof Ci.nsIDOMHTMLSelectElement)) ||
> +    if ((!(aField instanceof Ci.nsIDOMHTMLInputElement) &&
> +         !(ChromeUtils.getClassName(aField) === "HTMLSelectElement")) ||

!== please.

	Pulsebot
	Comment 7 • 7 years ago

Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68970144c5df
Remove nsIDOMHTMLSelectElement and nsIDOMHTMLOptionsCollection; r=bz

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 8 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/68970144c5df