Closed
Bug 1408186
Opened 7 years ago
Closed 7 years ago
Remove nsIDOMHTMLSelectElement and nsIDOMHTMLOptionsCollection
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: qdot, Assigned: qdot)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Continuing post-addon-deprecation XPCOM interface cleanup. These interfaces depend on each other, so removing them both in the same bug.
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
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`
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.
Attachment #8926114 -
Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Pushed by kmachulis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/68970144c5df Remove nsIDOMHTMLSelectElement and nsIDOMHTMLOptionsCollection; r=bz
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/68970144c5df
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•