Closed Bug 1228051 Opened 4 years ago Closed 4 years ago
_snprintf calls in ns CSSParser .cpp's CSSParser Impl::Parse Color
[spinning this bug off for the web-exposed pieces of bug 1197307, so that in the unlikely event this causes a noticeable change, we can backout or otherwise address it in a targeted way] Filing this bug on removing PR_snprintf calls in nsCSSParser.cpp -- probably replacing them with snprintf_literal. Per end of bug 1197307 comment 12, there's a chance this might cause a behavior-change, depending on how snprintf is implemented under-the-hood on our various platforms; there's some variation, apparently. See bug 1197307 comment 14 for more details. Seems likely that things will be OK, though, so I think we should proceed here & just be ready to backout if we discover an intolerable behavior-change.
I added you as the mentor for this bug. If you don't have a problem with that can you assign this bug to me. So that I can work on this bug.
Sounds good, thanks!
Assignee: nobody → allamsetty.anup
Removed the PR_snprintf calls in nsCSSParser.cpp Tested the patch locally. Build is successful.
Attachment #8692362 - Flags: review?(dholbert)
can you push the patch to the tryserver? currently I can't do it because I have classes to attend. Will be available only after 4 hours.
Comment on attachment 8692362 [details] [diff] [review] removed the PR_snprintf calls in nsCSSParser.cpp Thanks for the patch! ># HG changeset patch ># User Anup Kumar<email@example.com> ># Parent abbd213422a560f1180c4ec6e3bf4792c2ea81ba >Bug 1228051 - Remove PR_snprintf calls in nsCSSParser.cpp's CSSParserImpl::ParseColor r=dohlbert Two nits on patch metadata: (1) s/dohlbert/dholbert/ in the commit message (2) You should add a space between your name and the "<", in the "User" line. (That's how people normally format it, at least -- I'm not sure if the lack-of-a-space actually breaks any tools in practice, but I wouldn't be surprised if it did break something.) You can fix this by hand-editing the patch if you like, but really you want to update your .hgrc file. (and then after doing that, you can run "hg qref -U" to update the User header in the currently-applied MQ patch.) r=me with (1) addressed, but it's probably worth fixing (2) while you're at it. Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba7198d1256
Attachment #8692362 - Flags: review?(dholbert) → review+
Fixed the issues mentioned in the above comment. I hope now it can be pushed to the tryserver. Can you do it dholbert?
Comment on attachment 8692468 [details] [diff] [review] Fixed the issues in the metadata for the patch Looks good! No need to re-run through Try -- I did at the end of comment 5, and the oranges there look intermittent & unrelated. I suppose there's a chance one platform's snprintf impl will cause us trouble, but (given froydnj's assurances linked in comment 0) that seems unlikely enough that I think we should just land this, and backout in the unlikely event of trouble.
Attachment #8692468 - Flags: review?(dholbert) → review+
Adding "checkin-needed" to get this landed.
You need to log in before you can comment on or make changes to this bug.