1289890 - Change nsCOMArray::ReplaceObjectAt() return type from "bool" to "void", since it always succeeds

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Holbert [:dholbert]]

nsCOMArray::ReplaceObjectAt always succeeds (returns true), so there's no reason for it to have a boolean return type.

Its return type should just be "void", and its callers shouldn't have to care about checking its return value.

Filing this as a mentored bug.  The implementation is here (note the "// XXX make this return void" comment inside the function, too -- this bug is addressing that comment):
https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMArray.cpp#201

We need to get rid of the "return true", and replace "bool" with "void" in the function-signature, and update any callers to remove their failure-checking code.

Comment 1

[Daniel Holbert [:dholbert]]

(Anyone wanting to take this as their first bug, please have a look at...
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
...to learn how to compile Firefox, and feel free to ask questions in irc.mozilla.org #introduction or #developers )

Comment 2

[Igor]

Attachment: bug-1289890.patch

This is my first patch. Please let me know if there is anything else that I could do to help.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8776232 [details] [diff] [review]
bug-1289890.patch

Thank you for the patch! Sorry for the slow response -- I've been camping in the woods for a few days, and I'm just catching up on email/reviews now.

The code changes look good to me!  However, a few things that need to be addressed before this can be landed:
 (1) The patch needs a commit message and author metadata (your name/email address) to be included in the headers at the top, before someone can land it. See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for instructions on that (parts 1 and 3 there) -- or someone in irc.mozilla.org #introduction / #developers can help you with that.

(In this case, the bug's title should be a reasonable commit message -- e.g. you could use the following:
> Bug 1289890 - Change nsCOMArray::ReplaceObjectAt() return type from "bool" to "void", since it always succeeds. r?froyndnj
The bug title isn't *always* a good commit message, though, as described here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment -- you want something that describes/explains the change.  The bug title happens to fit the bill here.)


Also, this needs review from someone who has authority over /xpcom code, which probably means froydnj (module owner of that area of code). I'll CC him, and you should tag him for review when you upload your next version (with the metadata noted above.)  (You'll also want to include "r?froydnj" at the end of your commit message to indicate that he's the reviewer, as I showed in my sample commit message above)

Comment 4

[Daniel Holbert [:dholbert]]

Since I'm not expecting the patch's actual-code will need to change here, I took the liberty of pushing this to our "TryServer" to verify that it compiles everywhere & passes tests. (I only ran tests on one platform, to save resources, since a platform-specific test-failure seems unlikely here).

That Try Run is here:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8573b54a60a
More info here if you're curious: https://wiki.mozilla.org/ReleaseEngineering/TryServer

(Don't necessarily worry if it ends up showing a few failures; there are some tests that occasionally fail sporadically, and someone can triage those before this patch lands to see whether any of the Try run's failures are related to your changes or not.)

Comment 5

[Igor]

Attachment: bug-1289890.patch - with metadata

Hi, thanks for the feedback! I was expecting that, since I made the patch before I reached this part of the documentation.

I attached a new patch generated with metadata, and marked froydnj for review.

As for the building, it seems that it succeeded against the 'Linux x64 opt', if I understood correctly. Is there anything else that I can do to help now?

Thanks again.

Comment 6

[Daniel Holbert [:dholbert]]

Thanks! Looks good. RE the try run, it's got a bit more to do (notice the % complete / in-progress data at the upper-right -- all of the gray letters are for jobs that are still running or waiting to start).  I think test/build-failures are unlikely, though, so there's nothing more for you to do at this point.

Once this has r+ from froydnj, you can add the "checkin-needed" keyword (since it'll already have been tested on Try) and someone will come along and land it for you within a day or so.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8777145 [details] [diff] [review]
bug-1289890.patch - with metadata

Review of attachment 8777145 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you!  (It's considered good practice to obsolete previous patches when you post new ones: click the "details" link for the patch you'd like to obsolete, then click "edit details", check the box for "obsolete", and hit the "submit" button on the details page.  If somebody wants to see previous patches, links are available in the history of the bug.)

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8776232 [details] [diff] [review]
bug-1289890.patch

Ah, just remembered that's it's possible your account may not have the necessary permissions to do things like that.  I've gone ahead and obsoleted the previous patch for you.

Comment 9

[Igor]

Thanks! 

I still have to get used to the messages produced by the testing system, but it seems that it went ok.

I already marked it for checkin. Is there anything else that I am supposed to do? Also, if any of you would like to recommend other bug to work next, feel free to do that.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8777145 [details] [diff] [review]
bug-1289890.patch - with metadata

Thanks for tagging the patch as checkin?.  Unfortunately, you have stumbled over a quirk in how we do things; the checkin flag is only used to track the landing of multiple patches that don't all land simultaneously.  Those that have landed are marked as checkin+, and those that haven't receive no special marking.  (And we're not entirely consistent with doing that, either.)

The proper way to flag things as requiring checkin is to add the checkin-needed keyword to the bug, which I'll do after submitting this comment.  If you are setting checkin-needed, you'll also need proof of a green Try run, which comment 4 provides.  (If you have multiple patches, but only some of them need landing, I suppose you can set checkin-needed, and mark the particular patches you need committed as checkin?...but that's pretty unusual--by the time that you need to do something like that, you can usually check in your own patches!)

Comment 11

[Nathan Froyd [:froydnj]]

As to recommending other bugs...I'll try to have a think about that tomorrow and see if I come up with anything!

Comment 12

[Igor]

Perfect! Thanks for the explanation.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a56f87d804a
Change nsCOMArray::ReplaceObjectAt() return type from "bool" to "void", since it always succeeds. r=froyndnj

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a56f87d804a