Closed Bug 1228051 Opened 4 years ago Closed 4 years ago

Remove PR_snprintf calls in nsCSSParser.cpp's CSSParserImpl::ParseColor

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: dholbert, Assigned: reznord, Mentored)

Details

(Whiteboard: [lang=CPP])

Attachments

(1 file, 1 obsolete file)

[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.
Mentor: dholbert
Whiteboard: [lang=CPP]
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.
Flags: needinfo?(dholbert)
Sounds good, thanks!
Assignee: nobody → allamsetty.anup
Flags: needinfo?(dholbert)
Removed the PR_snprintf calls in nsCSSParser.cpp

Tested the patch locally. Build is successful.
Attachment #8692362 - Flags: review?(dholbert)
Status: NEW → ASSIGNED
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<allamsetty.anup@gmail.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?
Attachment #8692362 - Attachment is obsolete: true
Attachment #8692468 - Flags: review?(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/767cee28e83c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.