Add a "Contains" method for ns strings

RESOLVED FIXED in Firefox 42

Status

()

enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

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

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
Posted 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.
Assignee

Comment 1

4 years ago
Comment on attachment 8637424 [details] [diff] [review]
string_contains.patch

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

Updated

4 years ago
Assignee: nobody → arnaud.bienner
Status: NEW → ASSIGNED
Component: General → String
Comment on attachment 8637424 [details] [diff] [review]
string_contains.patch

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+
Assignee

Comment 3

4 years ago
(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 :)
Assignee

Comment 4

4 years ago
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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58210c299035

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]
bug1186603.patch

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+
Assignee

Updated

4 years ago
Blocks: 1190086
Assignee

Updated

4 years ago
Keywords: checkin-needed
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/6bf7a4af01c2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.