Closed Bug 1197315 Opened 4 years ago Closed 4 years ago

remove PR_snprintf calls in gfx/

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: vyas45, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 3 obsolete files)

Steps:

1. Find all calls to PR_snprintf in gfx/.
2. Add #include "mozilla/Snprintf.h" to the file(s).
3. Replace PR_snprintf(X, sizeof(X), ...) calls with snprintf_literal(X, ...).
4. Replace other PR_snprintf calls with snprintf.
5. Remove any #include "prprf.h" lines.
Attached patch 1197315.patch (obsolete) — Splinter Review
Attachment #8651454 - Flags: review?(nfroyd)
Hi Nathan,

I submitted a patch for this. Could you review it and tell what should be changed?

Thanks
Assignee: nobody → gioyik
Comment on attachment 8651454 [details] [diff] [review]
1197315.patch

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

::: gfx/layers/composite/FPSCounter.cpp
@@ +313,1 @@
>                          "FPS: %d = %d. ", fps, count);

This should be a call to snprintf instead of snprintf_literal; the indentation of the following line will also need to be fixed.
Attachment #8651454 - Flags: review?(nfroyd) → feedback+
Attached patch Patch to bug1197315 (obsolete) — Splinter Review
Hi , I am new to Mozilla development. I found some inactivity on the bug and hence put out this diff as my first diff. Please let me know if any changes are required. 

Thanks.
Attachment #8672577 - Flags: review?(nfroyd)
Comment on attachment 8672577 [details] [diff] [review]
Patch to bug1197315

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

Looks good to me!  Just one minor detail that was in the previous patch as well.

::: gfx/layers/composite/FPSCounter.cpp
@@ +309,5 @@
>      int fps = iter->first;
>      int count = iter->second;
>  
> +    length += snprintf(buffer + length, kBufferLength - length,
> +	      "FPS: %d = %d. ", fps, count);

The indentation of this line needs to be fixed up so it looks like:

... snprintf(buffer + length, ...
             "FPS: %d = %d. ", ...);
Attachment #8672577 - Flags: review?(nfroyd) → feedback+
Hi Nathan, 

    Thanks for the review. Had a noob question , since i am new to mercurial. In my source and diff (hg diff) I see that the indentation is correct, whereas on a hg export I see it reformatted. Do I need to have my hgrc/vimrc configured in some standard format ? 

Thanks,
Aniket
(In reply to Aniket Vyas from comment #6)
>     Thanks for the review. Had a noob question , since i am new to
> mercurial. In my source and diff (hg diff) I see that the indentation is
> correct, whereas on a hg export I see it reformatted. Do I need to have my
> hgrc/vimrc configured in some standard format ? 

|hg diff| shows the changes made in your working copy on disk, which means that you haven't actually committed those changes yet.  |hg export| only works for committed changes, which means that you'll have to call something like |hg commit| before calling |hg export|.

How did you get the previous changes into your source tree?  Are you using patch queues, or something else?
Attached file bug1197315 (obsolete) —
Have attached an output from "hg diff" . Please let me know if any changes are needed. 

Thanks.
Attachment #8672577 - Attachment is obsolete: true
Attachment #8672696 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Aniket Vyas from comment #6)
> >     Thanks for the review. Had a noob question , since i am new to
> > mercurial. In my source and diff (hg diff) I see that the indentation is
> > correct, whereas on a hg export I see it reformatted. Do I need to have my
> > hgrc/vimrc configured in some standard format ? 
> 
> |hg diff| shows the changes made in your working copy on disk, which means
> that you haven't actually committed those changes yet.  |hg export| only
> works for committed changes, which means that you'll have to call something
> like |hg commit| before calling |hg export|.
> 
> How did you get the previous changes into your source tree?  Are you using
> patch queues, or something else?

Hey Nathan, my workflow basically was to pull the source tree and build it. On top of that tree I made the required changes and had attached the output form hg patch to this forum.
Attached patch bug1197315.patchSplinter Review
Alright, finally an updated one. ran |hg commit| followed by |hg export|
Attachment #8672696 - Attachment is obsolete: true
Attachment #8672696 - Flags: review?(nfroyd)
Attachment #8672714 - Flags: review?(nfroyd)
Comment on attachment 8672714 [details] [diff] [review]
bug1197315.patch

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

This looks great, thanks for working through the issues!
Attachment #8672714 - Flags: review?(nfroyd) → review+
Thanks a lot Nathan for the help on my first diff ! What would be the next steps ? Is there something I need to do, or the bug to be marked against my name ?
Attachment #8651454 - Attachment is obsolete: true
(In reply to Aniket Vyas from comment #12)
> Thanks a lot Nathan for the help on my first diff ! What would be the next
> steps ? Is there something I need to do, or the bug to be marked against my
> name ?

Nothing for this patch!  I'll take care of testing it and getting it landed.

In the future, you'll need to make sure the patch compiles and passes tests on all of our platforms:

https://developer.mozilla.org/en-US/docs/Introduction#Step_6_-_Actually_get_the_code_into_the_tree

Once that's done, you can add the keyword "checkin-needed" to the bug (in the "Keywords" field near the top of the page).

But for now, sit back and bask in the glow of having your first patch accepted!  (And then go find another bug to work on... ;)
Great ! Thanks a lot Nathan. I did build this patch on my OSX development environment. Will look out for other bugs to squash :)
Assignee: gioyik → vyas45
https://hg.mozilla.org/mozilla-central/rev/4fda345579d4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1295083
You need to log in before you can comment on or make changes to this bug.