1197315 - remove PR_snprintf calls in gfx/

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Giovanny Gongora [:gioyik]]

Attachment: 1197315.patch

Comment 2

[Giovanny Gongora [:gioyik]]

Hi Nathan,

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

Thanks

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Aniket Vyas]

Attachment: Patch to bug1197315

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.

Comment 5

[Nathan Froyd [:froydnj]]

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. ", ...);

Comment 6

[Aniket Vyas]

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

Comment 7

[Nathan Froyd [:froydnj]]

(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?

Comment 8

[Aniket Vyas]

Attachment: bug1197315

Have attached an output from "hg diff" . Please let me know if any changes are needed. 

Thanks.

Comment 9

[Aniket Vyas]

(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.

Comment 10

[Aniket Vyas]

Attachment: bug1197315.patch

Alright, finally an updated one. ran |hg commit| followed by |hg export|

Comment 11

[Nathan Froyd [:froydnj]]

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!

Comment 12

[Aniket Vyas]

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 ?

Comment 13

[Nathan Froyd [:froydnj]]

(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... ;)

Comment 14

[Aniket Vyas]

Great ! Thanks a lot Nathan. I did build this patch on my OSX development environment. Will look out for other bugs to squash :)

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fda345579d4

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fda345579d4

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fda345579d4