1197307 - remove PR_snprintf calls in layout/

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nathan Froyd [:froydnj]]

Steps:

1. Find all calls to PR_snprintf in layout/.
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: 1197307.patch

Comment 2

[Giovanny Gongora [:gioyik]]

Hi Nathan,

I created a patch for this, could you review it and tell me if is OK? I am assigning this to myself to keep it on my dashboard and not loose it with all the bugzilla email I receive.

Thanks

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8651350 [details] [diff] [review]
1197307.patch

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

Thanks for the patch!  A few little things to fix, but overall, it looks good.

::: layout/generic/nsBlockFrame.cpp
@@ +30,5 @@
>  #include "nsHTMLParts.h"
>  #include "nsGkAtoms.h"
>  #include "nsGenericHTMLElement.h"
>  #include "nsAttrValueInlines.h"
> +#include "mozilla/snprintf.h"

This should be mozilla/Snprintf.h.

::: layout/generic/nsFrame.cpp
@@ +35,5 @@
>  #include "nsPresContext.h"
>  #include "nsStyleConsts.h"
>  #include "nsIPresShell.h"
>  #include "mozilla/Logging.h"
> +#include "mozilla/snprintf.h"

Please make this "mozilla/Snprintf.h"

::: layout/generic/nsLineBox.cpp
@@ +15,5 @@
>  #include "nsFrame.h"
>  #include "nsIFrameInlines.h"
>  #include "nsPresArena.h"
>  #include "nsPrintfCString.h"
> +#include "mozilla/snprintf.h"

Please make this "mozilla/Snprintf.h".

::: layout/mathml/nsMathMLChar.cpp
@@ +28,5 @@
>  #include "nsContentUtils.h"
>  
>  #include "mozilla/LookAndFeel.h"
>  #include "nsCSSRendering.h"
> +#include "mozilla/snprintf.h"

Please make this "mozilla/Snprintf.h"

@@ +314,5 @@
>    // Update our cache if it is not associated to this character
>    if (mCharCache != aChar) {
>      // The key in the property file is interpreted as ASCII and kept
>      // as such ...
> +    char key[10]; snprintf_literal(key, "\\u%04X", aChar);

I think |aChar| here should be explicitly casted to |int|.

::: layout/mathml/nsMathMLmactionFrame.cpp
@@ +6,5 @@
>  #include "nsMathMLmactionFrame.h"
>  #include "nsCOMPtr.h"
>  #include "nsPresContext.h"
>  #include "nsNameSpaceManager.h"
> +#include "mozilla/snprintf.h"

Please make this "mozilla/Snprintf.h"

::: layout/style/CounterStyleManager.cpp
@@ +16,5 @@
>  #include "nsStyleSet.h"
>  #include "nsTArray.h"
>  #include "nsTHashtable.h"
>  #include "nsUnicodeProperties.h"
> +#include "mozilla/snprintf.h"

Please make this "mozilla/Snprintf.h"

@@ +210,5 @@
>  DecimalToText(CounterValue aOrdinal, nsSubstring& aResult)
>  {
>    // 3 for additional digit, negative sign, and null
>    char cbuf[std::numeric_limits<CounterValue>::digits10 + 3];
> +  snprintf_literal(cbuf, "%ld", aOrdinal);

This format string should be "%d".

::: layout/style/nsCSSParser.cpp
@@ +38,5 @@
>  #include "nsIMediaList.h"
>  #include "nsStyleUtil.h"
>  #include "nsIPrincipal.h"
>  #include "nsICSSUnprefixingService.h"
> +#include "mozilla/snprintf.h"

Please make this "mozilla/Snprintf.h"

Comment 4

[Giovanny Gongora [:gioyik]]

Attachment: 1197307.patch

Comment 5

[Giovanny Gongora [:gioyik]]

Nathan, I attached you a new patch for this with changes you suggested, hope this is the good one.

Thanks for the feedback

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8651441 [details] [diff] [review]
1197307.patch

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

Hi, this looks good, just a few last nits.

::: layout/generic/nsBlockFrame.cpp
@@ +1425,1 @@
>                  ": %lld elapsed (%lld per line) (%d lines; %d new lines)",

Please fix the indentation of the rest of the arguments to this function.

@@ +6557,1 @@
>                  ": %lld elapsed (%lld per line) lines=%d drawn=%d skip=%d",

Here too.

::: layout/generic/nsFrame.cpp
@@ +9031,1 @@
>                  nsAtomCString(aContent->NodeInfo()->NameAtom()).get(), aFrame);

Here too.

::: layout/generic/nsLineBox.cpp
@@ +216,1 @@
>                IsBlock() ? "block" : "inline",

Here too.

::: layout/mathml/nsMathMLChar.cpp
@@ +314,5 @@
>    // Update our cache if it is not associated to this character
>    if (mCharCache != aChar) {
>      // The key in the property file is interpreted as ASCII and kept
>      // as such ...
> +    char key[10]; snprintf_literal(key, "\\u%04X", aChar);

Looks like you forgot the explicit cast to |int| that I requested.  Please put it in.

Comment 7

[Anup Kumar [:reznord]]

Attachment: changes made as requested in comment 6

Hi Nathan,

I have made the changes as you requested in the previous comment and have created a new patch, please review that patch and tell me whether any more changes need to be done or not.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8672612 [details] [diff] [review]
changes made as requested in comment 6

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

Thanks for updating the patch!  There are a couple of things that I forgot to catch last time around, and it looks like not all the issues from comment 6 were addressed...perhaps you forgot to update your patch queue prior to submitting the patch?

::: layout/generic/nsBlockFrame.cpp
@@ +1422,5 @@
>      ListTag(stdout);
>      char buf[400];
> +    snprintf_literal(buf,
> +         ": %lld elapsed (%lld per line) (%d lines; %d new lines)",
> +         delta, perLineDelta, numLines, ectc - ctc);

The indentation for these two lines still needs to be fixed.

@@ +6570,5 @@
>      char buf[400];
> +    snprintf_literal(buf,
> +         ": %lld elapsed (%lld per line) lines=%d drawn=%d skip=%d",
> +         delta, deltaPerLine, numLines,
> +         drawnLines, numLines - drawnLines);

Same here, for these three lines.

::: layout/generic/nsFrame.cpp
@@ +9087,5 @@
>  GetTagName(nsFrame* aFrame, nsIContent* aContent, int aResultSize,
>             char* aResult)
>  {
>    if (aContent) {
> +    snprintf_literal(aResult, aResultSize, "%s@%p",

This should be a call to snprintf, not snprintf_literal.

@@ +9088,5 @@
>             char* aResult)
>  {
>    if (aContent) {
> +    snprintf_literal(aResult, aResultSize, "%s@%p",
> +        nsAtomCString(aContent->NodeInfo()->NameAtom()).get(), aFrame);

The indentation on this line needs to be fixed.

@@ +9093,3 @@
>    }
>    else {
> +    snprintf_literal(aResult, aResultSize, "@%p", aFrame);

This should be a call to snprintf, not snprintf_literal.

::: layout/generic/nsLineBox.cpp
@@ +212,5 @@
>  
>  char*
>  nsLineBox::StateToString(char* aBuf, int32_t aBufSize) const
>  {
> +  snprintf_literal(aBuf, aBufSize, "%s,%s,%s,%s,%s,before:%s,after:%s[0x%x]",

This should be an snprintf call, since |aBuf| is not a character array.

@@ +220,5 @@
> +      IsImpactedByFloat() ? "impacted" : "not impacted",
> +      IsLineWrapped() ? "wrapped" : "not wrapped",
> +      BreakTypeToString(GetBreakTypeBefore()),
> +      BreakTypeToString(GetBreakTypeAfter()),
> +      mAllFlags);

All these argument lines need to be aligned with the |aBuf| argument to snprintf.

::: layout/mathml/nsMathMLChar.cpp
@@ +28,5 @@
>  #include "nsContentUtils.h"
>  
>  #include "mozilla/LookAndFeel.h"
>  #include "nsCSSRendering.h"
> +#include "mozilla/Snprintf.h"         // For snpritf_literal()

Minor nit: This comment should read "For snprintf_literal()"

@@ +314,5 @@
>    // Update our cache if it is not associated to this character
>    if (mCharCache != aChar) {
>      // The key in the property file is interpreted as ASCII and kept
>      // as such ...
> +    char key[10]; snprintf_literal(key, "\\u%04X", int);

You seem to have dropped the |aChar| argument here; I think you meant |int(aChar)|

::: layout/style/CounterStyleManager.cpp
@@ +211,5 @@
>  DecimalToText(CounterValue aOrdinal, nsSubstring& aResult)
>  {
>    // 3 for additional digit, negative sign, and null
>    char cbuf[std::numeric_limits<CounterValue>::digits10 + 3];
> +  snprintf_literal(cbuf, "%ld", aOrdinal);

This format string should be "%d".

Comment 9

[Anup Kumar [:reznord]]

Attachment: fixed the indentation problems

Fixed all the problems as per you requested in comment 8.

Comment 10

[Anup Kumar [:reznord]]

Attachment: included "mozilla/Snprintf.h" in CounterStyleManager.cpp

Included the "mozilla/Snprintf.h" header file in CounterStyleManager.cpp and the build is successful.

Comment 11

[Anup Kumar [:reznord]]

Here is the tryserver pull request for this patch suggested by Nathan Froyd.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e349dbd12d48

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8673213 [details] [diff] [review]
included "mozilla/Snprintf.h" in CounterStyleManager.cpp

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

The tests generally look good: the debug build errors are because of the nsLineBox.cpp problem below, and the test oranges all look like known problems not introduced by your patch.

Given that the nsLineBox problem is easily correctable, I'm r+'ing this patch, but I'm asking Daniel Holbert to take a look at the nsCSSParser.cpp bit.  It's not a problem with the work that you did; I'm just not certain that the change wouldn't have consequences for how web pages render.

Assuming Daniel (or somebody else) approves of the nsCSSParser bit, a patch with the nsLineBox.cpp issue fixed is OK.

::: layout/generic/nsLineBox.cpp
@@ +212,5 @@
>  
>  char*
>  nsLineBox::StateToString(char* aBuf, int32_t aBufSize) const
>  {
> +  snprintf_literal(aBuf, aBufSize, "%s,%s,%s,%s,%s,before:%s,after:%s[0x%x]",

It looks like this use needs a #include "mozilla/Snprintf.h" added further up in the file.

::: layout/style/nsCSSParser.cpp
@@ +6425,5 @@
>          break;
>  
>        case eCSSToken_Dimension:
>          if (tk->mIdent.Length() <= 6) {
> +          snprintf_literal(buffer, "%06.0f", tk->mNumber);

This change is the only non-obvious change here: we've moved from consistent floating-point printing behavior (PR_snprintf ought to work the same everywhere) to platform-specific behavior (snprintf can and probably does vary between platforms).  My reading of this code suggests that it's probably not an issue, but I'm not entirely sure about that.

We could just punt here by calling nsTSubstring.AppendPrintf (which does have behavior consistent across platforms, because it gets used for formatting CSS colors elsewhere), but that's a non-mechanical change to make to the current code.

A behavior change here would be quirks-mode only, if that makes any difference.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> ::: layout/style/nsCSSParser.cpp
> This change is the only non-obvious change here: we've moved from consistent
> floating-point printing behavior (PR_snprintf ought to work the same
> everywhere) to platform-specific behavior (snprintf can and probably does
> vary between platforms).  My reading of this code suggests that it's
> probably not an issue, but I'm not entirely sure about that.

Thanks for highlighting this -- I hadn't realized (or had forgotten) that snprintf (or rather vsnprintf, which is what we use under the hood) varies between platforms.

Can you comment on how it varies?

Why was this not an issue for other PR_snprintf-->mozilla/Snprintf.h conversions? (maybe because other snprintf usages are only for debugging output?)

> We could just punt here by calling nsTSubstring.AppendPrintf (which does
> have behavior consistent across platforms, because it gets used for
> formatting CSS colors elsewhere), but that's a non-mechanical change to make
> to the current code.

I'd be on board with this, if we've got any reservations about snprintf's consistency. I'd be happy if we spun off a followup bug for that, and dropped an XXX comment in the code here pointing to that bug (to avoid making Anup fix that as part of this bug here).

> A behavior change here would be quirks-mode only, if that makes any
> difference.

I don't think it makes much of a difference.  A hypothetical unintended behavior-change that only breaks quirks-mode websites (on some platforms) would still be bad.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] (less responsive Oct 9-12 & 15-18) from comment #13)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > ::: layout/style/nsCSSParser.cpp
> > This change is the only non-obvious change here: we've moved from consistent
> > floating-point printing behavior (PR_snprintf ought to work the same
> > everywhere) to platform-specific behavior (snprintf can and probably does
> > vary between platforms).  My reading of this code suggests that it's
> > probably not an issue, but I'm not entirely sure about that.
> 
> Can you comment on how it varies?
> 
> Why was this not an issue for other PR_snprintf-->mozilla/Snprintf.h
> conversions? (maybe because other snprintf usages are only for debugging
> output?)

I believe the variations are mostly in how floating-point numbers are formatted (perhaps issues about %p as well); I am willing to assert that everybody gets formatting integers and strings right just because there's no argument about "is this formatting correct?".  But for floating-point numbers, there's issues of precision and how willing one is to get The Right Answer 100% of the time and so forth.

All the other conversions fell into categories of:

- No differences (formatting strings, integers)
- Differences in non-DEBUG code that could be tolerated (e.g. formatting an FPS counter)
- Differences in DEBUG code that could be tolerated (e.g. logging floating point numbers)

> > We could just punt here by calling nsTSubstring.AppendPrintf (which does
> > have behavior consistent across platforms, because it gets used for
> > formatting CSS colors elsewhere), but that's a non-mechanical change to make
> > to the current code.
> 
> I'd be on board with this, if we've got any reservations about snprintf's
> consistency. I'd be happy if we spun off a followup bug for that, and
> dropped an XXX comment in the code here pointing to that bug (to avoid
> making Anup fix that as part of this bug here).

I meant AppendFloat  here, but AppendPrintf could work as well.  AppendPrintf has the virtue (?) of using NSPR under the hood; AppendFloat uses a different, faster algorithm, but is compatible with AppendPrintf due to webcompat concerns.

Let's punt this part to a separate bug, then; there's enough fiddliness in doing this that it merits a different patch and more careful review.

Comment 15

[Daniel Holbert [:dholbert]]

I wonder if the calls in nsMathMLChar, nsMathMLmactionFrame, and CounterStyleManager should really be using AppendPrintf and/or its wrapper AppendInt()?

e.g. right now, the patch will leave us with the following in nsMathMLmactionFrame:
      nsAutoString value;
      char cbuf[10];
      snprintf_literal(cbuf, "%d", selection);
      value.AssignASCII(cbuf);

This could be condensed to:
      nsAutoString value;
      value.AppendInt(selection);
...which is shorter, easier to read, and more foolproof.

(For that matter, I wonder if all of the calls here should really be condensed to use AppendPrintf. I care less about the debug-only ones, though, since any mistakes in those won't affect users.)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8673213 [details] [diff] [review]
included "mozilla/Snprintf.h" in CounterStyleManager.cpp

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

Here are my review comments (just stylistic nits).

Before this lands, though:
 - The nsCSSParser.cpp changes need to be stripped out, per end of comment 14.
 - I'm curious to hear froydnj's thoughts about comment 15 (which would obsolete some/all of this patch)

::: layout/generic/nsBlockFrame.cpp
@@ +1420,5 @@
>      perLineDelta = delta / lines;
>  
>      ListTag(stdout);
>      char buf[400];
> +    snprintf_literal(buf, 

Drop the trailing space character on this line.

@@ +6569,5 @@
>      ListTag(stdout);
>      char buf[400];
> +    snprintf_literal(buf,
> +                     ": %lld elapsed (%lld per line) lines=%d drawn=%d skip=%d",
> +                     delta, deltaPerLine, numLines, 

Drop trailing space here, too.

::: layout/generic/nsFrame.cpp
@@ +9092,1 @@
>                  nsAtomCString(aContent->NodeInfo()->NameAtom()).get(), aFrame);

Please deindent this line by 3 spaces, to keep it aligned with the args on the 1st line (after the "PR_" deletion)

::: layout/mathml/nsMathMLChar.cpp
@@ +28,5 @@
>  #include "nsContentUtils.h"
>  
>  #include "mozilla/LookAndFeel.h"
>  #include "nsCSSRendering.h"
> +#include "mozilla/Snprintf.h"         // For snprintf_literal()

Just drop the comment here, instead of updating it.

I'm guessing the comment was there because "prprf.h" was a bit cryptically-named & non-obvious what it's for, but "Snprintf.h" is pretty obvious.

::: layout/mathml/nsMathMLmactionFrame.cpp
@@ +6,5 @@
>  #include "nsMathMLmactionFrame.h"
>  #include "nsCOMPtr.h"
>  #include "nsPresContext.h"
>  #include "nsNameSpaceManager.h"
> +#include "mozilla/Snprintf.h"         // For snprintf_literal()

As above, just drop the comment here.

::: layout/style/CounterStyleManager.cpp
@@ +11,5 @@
>  #include "mozilla/CheckedInt.h"
>  #include "mozilla/MathAlgorithms.h"
>  #include "mozilla/Types.h"
>  #include "mozilla/WritingModes.h"
> +#include "mozilla/Snprintf.h"

The mozilla/ includes here are sorted, so please insert this new include in sorted order (up one line).

Comment 17

[Daniel Holbert [:dholbert]]

[ni=froydnj for thoughts on whether going straight to AppendPrintf / AppendInt in all cases would be better here, per comment 15]

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> e.g. right now, the patch will leave us with the following in
> nsMathMLmactionFrame:
>       nsAutoString value;
>       char cbuf[10];
>       snprintf_literal(cbuf, "%d", selection);
>       value.AssignASCII(cbuf);
> 
> This could be condensed to:
>       nsAutoString value;
>       value.AppendInt(selection);
> ...which is shorter, easier to read, and more foolproof.

Sure, we could do that.

> (For that matter, I wonder if all of the calls here should really be
> condensed to use AppendPrintf. I care less about the debug-only ones,
> though, since any mistakes in those won't affect users.)

I have a slight preference for converting single-value printfs to use the nsString helpers while leaving multi-value printfs for manual snprintf uses.  It would be nice for the multi-value printf cases to use standard escape sequences (which I think is half of the benefit of converting to snprintf), which AppendPrintf does not.  AppendInt et al at least hide the details of the escape sequence used.  But this isn't my code, so I'm happy to recommend whichever route you like.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> I have a slight preference for converting single-value printfs to use the
> nsString helpers while leaving multi-value printfs for manual snprintf uses.

This makes sense to me.

One perf consideration: it looks like AppendPrintf() effectively calls AppendASCII one character at a time (through several layers of abstraction). And if we need to reallocate, this causes some inefficiency/memory-churn, since AppendASCII extends the string's buffer conservatively (not with a doubling algorithm), which will mean 1 character at a time. (These calls happen via http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prprf.c?rev=e65ce3ee7cde#720 via calls to "ss->stuff" which is a wrapped version of AppendASCII in this case.)

BUT, we're unlikely to trip over this perf issue as long as we're using a nsAutoString, since it brings along a decently-sized buffer up-front & it's unlikely we'll need to reallocate. And I'm pretty sure that's really what we're using in all of these cases.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8673213 [details] [diff] [review]
included "mozilla/Snprintf.h" in CounterStyleManager.cpp

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

OK, I'm happy to sign off on this, once prior review comments addressed, & also with the trivial single-value snprintf calls in nsMathMLmactionFrame.cpp and CounterStyleManager.cpp converted to AppendInt(), per below.

(and with a new bug filed for the nsCSSParser changes, per end of comment 14)

It's probably worth a final look-over after those changes have been made, so I'm calling this "r-" for the moment, and I'll promise swift r+ once an updated patch is posted. Thanks!

::: layout/mathml/nsMathMLmactionFrame.cpp
@@ +331,1 @@
>        value.AssignASCII(cbuf);

Here, let's just use:
  value.AppendInt(selection);
and do away with cbuf & snprintf.  (So, we won't need the Snprintf include here, either.)

::: layout/style/CounterStyleManager.cpp
@@ +212,5 @@
>  DecimalToText(CounterValue aOrdinal, nsSubstring& aResult)
>  {
>    // 3 for additional digit, negative sign, and null
>    char cbuf[std::numeric_limits<CounterValue>::digits10 + 3];
> +  snprintf_literal(cbuf, "%d", aOrdinal);

Here, let's just simplify to:
 aResult.AppendInt(aOrdinal);
and do away with cbuf & snprintf.  (So, we won't need the Snprintf include here, either.)

(Note that CounterValue is just a typedef for int32_t.)

Comment 21

[Haitham]

I would like to try this bug as im new to this

Comment 22

[Daniel Holbert [:dholbert]]

Haitham: Thanks for your interest! This bug here is already assigned (and already has a patch from the assignee, which just needs a bit of tweaks).

You can look for an unassigned good-first-bug at http://www.joshmatthews.net/bugsahoy/, though!

Comment 23

[Daniel Holbert [:dholbert]]

[Anup, see review feedback in comment 16 and comment 20.]

Comment 24

[Anup Kumar [:reznord]]

(In reply to Daniel Holbert [:dholbert] from comment #23)
> [Anup, see review feedback in comment 16 and comment 20.]

Thanks, will update the patch soon. 

Clearing needinfo

Comment 25

[Anup Kumar [:reznord]]

Attachment: Fixed all the PR_snprintf calls in layout/

Fixed all the errors from the previous comments and tested locally in my tree. The build compiled successfully without any errors. 

Please tell me if there is anything more needs to be done here.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8691765 [details] [diff] [review]
Fixed all the PR_snprintf calls in layout/

As noted at end of comment 14 & beginning of comment 16, let's split the nsCSSParser changes into their own patch on a different bug, because it's conceivable we might need to back those changes out if we discover they cause breakage due to a platform-specific snprintf formatting quirk.

I've just filed bug 1228051 for this. Could you post the nsCSSParser.cpp changes there, and repost the bug here without those changes?

With that, I think this looks good!

Comment 27

[Anup Kumar [:reznord]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> Comment on attachment 8691765 [details] [diff] [review]
> Fixed all the PR_snprintf calls in layout/
> 
> As noted at end of comment 14 & beginning of comment 16, let's split the
> nsCSSParser changes into their own patch on a different bug, because it's
> conceivable we might need to back those changes out if we discover they
> cause breakage due to a platform-specific snprintf formatting quirk.
> 
> I've just filed bug 1228051 for this. Could you post the nsCSSParser.cpp
> changes there, and repost the bug here without those changes?
> 
> With that, I think this looks good!

Sure will do that. Will update the patch today evening after I'm done with my classes. 

Can I get that bug assigned to me?

Comment 28

[Daniel Holbert [:dholbert]]

As noted in bug 1228051 -- before you re-post this bug's final patch (with the cssparser changes removed), you probably want to tweak the "User" patch-header to add a space between your name & your email address.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8691765 [details] [diff] [review]
Fixed all the PR_snprintf calls in layout/

[Canceling review requests, per comment 26.]

Comment 30

[Anup Kumar [:reznord]]

(In reply to Daniel Holbert [:dholbert] from comment #29)
> Comment on attachment 8691765 [details] [diff] [review]
> Fixed all the PR_snprintf calls in layout/
> 
> [Canceling review requests, per comment 26.]

I will update the patch today, leaving nsCSSParser.cpp because we have fixed that in the other bug i.e. bug 1228051

Did the patch for the bug 1228051 got landed?

Comment 31

[Anup Kumar [:reznord]]

Attachment: Fixed all the PR_snprintf calls in layout/ except for style/nsCSSParser.cpp

Didn't make changes to the PR_snprintf calls in nsCSSParser.cpp since it is made into a separate bug i.e. bug 1228051 

@dholbert, should I make anymore changes to the patch?

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8693152 [details] [diff] [review]
Fixed all the PR_snprintf calls in layout/ except for style/nsCSSParser.cpp

(In reply to Anup Kumar from comment #30)
> Did the patch for the bug 1228051 got landed?

Nope, it's still marked "checkin-needed". Once a sheriff lands it (in the next couple of days, probably), you'll see a mozilla-inbound URL get posted there & "checkin-needed" get removed.

> @dholbert, should I make anymore changes to the patch?

Just needs "r=dholbert" in the commit message, I think. (That seems to have been dropped between the last version and this version.)

No need to re-request review from me or Nathan on the new patch; just re-post with "r=dholbert" added to the commit message, and then you can add "checkin-needed" here.

Thanks!

Comment 33

[Anup Kumar [:reznord]]

(In reply to Daniel Holbert [:dholbert] from comment #32)
> Comment on attachment 8693152 [details] [diff] [review]
> Fixed all the PR_snprintf calls in layout/ except for style/nsCSSParser.cpp
> 
> (In reply to Anup Kumar from comment #30)
> > Did the patch for the bug 1228051 got landed?
> 
> Nope, it's still marked "checkin-needed". Once a sheriff lands it (in the
> next couple of days, probably), you'll see a mozilla-inbound URL get posted
> there & "checkin-needed" get removed.
> 
> > @dholbert, should I make anymore changes to the patch?
> 
> Just needs "r=dholbert" in the commit message, I think. (That seems to have
> been dropped between the last version and this version.)
> 
> No need to re-request review from me or Nathan on the new patch; just
> re-post with "r=dholbert" added to the commit message, and then you can add
> "checkin-needed" here.
> 
> Thanks!

Can I just edit the patch and add your name as a reviewer? I hope it will not create a problem, if I change the metadata of the patch.

Comment 34

[Anup Kumar [:reznord]]

Attachment: Fixed all the PR_snprintf calls in layout/ except for style/nsCSSParser.cpp

added "r=dholbert" in the metadata for the patch

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8693167 [details] [diff] [review]
Fixed all the PR_snprintf calls in layout/ except for style/nsCSSParser.cpp

Yup, you can hand-edit patch files to adjust things like commit messages.

I'll mark this r+ explicitly, just to make sure sheriffs see that this is reviewed when they triage checkin-needed stuff.  (I did double-check to be sure that the commit message is the only thing that changed.)

thanks!

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/427dc5894094

Comment 37

[Carsten Book [:Tomcat]]

sorry backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=18055456&repo=mozilla-inbound

Comment 38

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/301532805d40

Comment 39

[Anup Kumar [:reznord]]

(In reply to Carsten Book [:Tomcat] from comment #37)
> sorry backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=18055456&repo=mozilla-
> inbound

I didn't get any errors while I tried to check the patch locally. It worked perfectly fine for me.

Comment 40

[Nathan Froyd [:froydnj]]

Comment on attachment 8693167 [details] [diff] [review]
Fixed all the PR_snprintf calls in layout/ except for style/nsCSSParser.cpp

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

::: layout/style/CounterStyleManager.cpp
@@ +209,5 @@
>  static bool
>  DecimalToText(CounterValue aOrdinal, nsSubstring& aResult)
>  {
>    // 3 for additional digit, negative sign, and null
>    char cbuf[std::numeric_limits<CounterValue>::digits10 + 3];

You need to remove this declaration as well as fixing the compile errors that led to the patch being backed out.

Comment 41

[Daniel Holbert [:dholbert]]

Looks like the compile error in comment 37's log is:
{
layout/generic/nsLineBox.cpp:224:29: error: no matching function for call to 'snprintf_literal(char*&, int32_t&, const char [40], const char*, const char [6], const char [16], const char*, const char*, const char*, const char*, const uint32_t&)'
}

(Ignore the stuff about pip/http -- that's bug 1152749, & isn't what's causing the build bustage.)

Comment 42

[Daniel Holbert [:dholbert]]

Attachment: patch w/ build bustage fixed

Anup pinged me in IRC to let me know he's prepping for exams, so I fixed the build bustage & am reposting the patch. I'll give it a Try run to be sure I didn't miss anything.

(The problem in nsLineBox.cpp is actually something that froydnj caught in comment 8 -- looks like that one review comment was just missed.)

Comment 43

[Daniel Holbert [:dholbert]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4fff92e54e7

Comment 44

[Daniel Holbert [:dholbert]]

Try looks good; oranges all look like intermittents. Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/805a09eaffc0

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/805a09eaffc0