Closed Bug 1186603 Opened 5 years ago Closed 5 years ago

Add a "Contains" method for ns strings


(Core :: String, enhancement)

Not set



Tracking Status
firefox42 --- fixed


(Reporter: arnaud.bienner, Assigned: arnaud.bienner)




(1 file, 1 obsolete file)

Attached patch string_contains.patch (obsolete) — Splinter Review
While working on bug 1155154, I needed to check if a string contains a char.
"FindChar != -1" does what I want, but IMHO it would be more intuitive to have a "Contains" method, which returns a bool.

Attached is a quick patch I wrote to describe what I mean (with an example).
I need to check carefully if this works for all kind of strings, etc. but I wanted to have some feedback first to know if it's worth it.
Comment on attachment 8637424 [details] [diff] [review]

Nathan: smaug suggested me you're the person to contact to review such patches.
What are your thoughts about this?
Attachment #8637424 - Flags: feedback?(nfroyd)
Assignee: nobody → arnaud.bienner
Component: General → String
Comment on attachment 8637424 [details] [diff] [review]

Review of attachment 8637424 [details] [diff] [review]:

Thanks for the patch!  Everything here looks good; please do write a proper commit message, though.

::: dom/html/HTMLInputElement.cpp
@@ +7083,5 @@
>        filterMask = nsIFilePicker::filterVideo;
>        filterBundle->GetStringFromName(MOZ_UTF16("videoFilter"),
>                                        getter_Copies(extensionListStr));
>      } else if (token.First() == '.') {
> +      if (token.Contains(';') || token.FindChar('*') >= 0) {

Surely the second call can use Contains as well?
Attachment #8637424 - Flags: feedback?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #2)
> Surely the second call can use Contains as well?

Yes. I kept the old one so you can see both old and new way to do the check side by side.
I will update my patch then :)
Attached patch bug1186603.patchSplinter Review
Updated patch, with commit message + second call using "Contains" as well.

Though this was probably not really needed for this case, I pushed this to try just to be sure, and everything looks good:

Not sure if it's worth to update other places where this could be use: it could make the code more readable in those cases too, but I'm reluctant updating already tested/working code for such a small readability improvement. Having it available for new code is probably enough IMO.
Attachment #8637424 - Attachment is obsolete: true
Attachment #8641305 - Flags: review?(nfroyd)
Comment on attachment 8641305 [details] [diff] [review]

Review of attachment 8641305 [details] [diff] [review]:

Looks great, thank you!

Would you please file a followup bug for using Contains() more widely?  (Not saying you have to do it, of course.)
Attachment #8641305 - Flags: review?(nfroyd) → review+
Blocks: 1190086
Keywords: checkin-needed
Version: unspecified → Trunk
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.