deCOMtaminate nsIFrame::IsSelectable

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

While reviewing bug 389283, I noticed:

 * nsIFrame::IsSelectable is virtual, but is implemented only on nsFrame and not overridden by subclasses.  It should just be non-virtual, with the implementation on nsIFrame instead of nsFrame.

 * it returns nsresult and has a boolean out parameter, but the only time it returns a failure result is if the caller provides a null out-parameter.  It should just return the boolean result instead!
I wonder if there is any tool which can scan the whole build and list all virtual functions which only have one implementation (which means they probably shouldn't be virtual).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8815105 [details]
Bug 1320815 - Make nsIFrame::IsSelectable non-virtual, and move implementation from nsFrame to nsIFrame.

https://reviewboard.mozilla.org/r/96092/#review96244
Attachment #8815105 - Flags: review?(xidorn+moz) → review+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8815106 [details]
Bug 1320815 - DeCOMtaminate nsIFrame::IsSelectable by returning boolean instead of nsresult.

https://reviewboard.mozilla.org/r/96094/#review96246
Attachment #8815106 - Flags: review?(xidorn+moz) → review+

Comment 6

2 years ago
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02cc5b7bf39c
Make nsIFrame::IsSelectable non-virtual, and move implementation from nsFrame to nsIFrame.  r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ae6227686e47
DeCOMtaminate nsIFrame::IsSelectable by returning boolean instead of nsresult.  r=xidorn

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02cc5b7bf39c
https://hg.mozilla.org/mozilla-central/rev/ae6227686e47
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.