Closed Bug 673820 Opened 8 years ago Closed 8 years ago

Stop parsing integers when something else than [0-9] is found

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mounir, Assigned: atulagrwl)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=volkmar])

Attachments

(1 file, 12 obsolete files)

Basically, the rules for parsing integers say that as soon as +/- and whitespaces are parsed, the parsing should stop when something else than [0-9] is found. In other words, that means that this string "42foo" should be parsed as "42". Our current implementation returns an error.

Specifications:
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-non-negative-integers
and
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-integers

Ainsley, this could be an interesting bug if you want to write some C++ code. Would you be interested?
The parsing is done here:
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.cpp#1073
Attached patch Diff to fix bug (obsolete) — Splinter Review
This patch fixes this bug. A good test case is this URL: http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A...%3Col%3E%3Cli%20value%3D22foo%3EBar%3C%2Fol%3E

When you go there, the resulting list should show 22. Bar. Before this fix, firefox would just show 1. Bar due to the "foo" being in the integer value.
Attachment #549357 - Flags: review?(qheaden)
Attachment #549357 - Flags: feedback?(qheaden)
Attachment #549357 - Flags: review?(qheaden)
Attachment #549357 - Flags: review?(mounir)
Attachment #549357 - Flags: feedback?(qheaden)
Comment on attachment 549357 [details] [diff] [review]
Diff to fix bug

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

The general idea seems good.

Please update the patch and ask a review to bz.

::: content/base/src/nsAttrValue.cpp
@@ +1420,5 @@
>                *aIsPercent = PR_TRUE;
>              } else {
>                *aStrict = PR_FALSE;
>              }
> +		  } else{ 

Seems to be an indentation issue here. Are you using tabs instead of spaces?

@@ +1421,5 @@
>              } else {
>                *aStrict = PR_FALSE;
>              }
> +		  } else{ 
> +			  *aStrict = PR_TRUE;

You don't need to set *aStrict to PR_TRUE here.

@@ +1424,5 @@
> +		  } else{ 
> +			  *aStrict = PR_TRUE;
> +
> +			  while(iter != end)
> +				  iter++;

I think |break;| would do it here.
Attachment #549357 - Flags: review?(mounir) → feedback+
Thank you for working on this BTW :)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
Attached patch Revision to the previous patch (obsolete) — Splinter Review
Updated the patch with the suggestions given. It came down to becoming a one line change.
Attachment #549357 - Attachment is obsolete: true
Attachment #549523 - Flags: review?(bzbarsky)
Comment on attachment 549523 [details] [diff] [review]
Revision to the previous patch

Jonas knows this stuff better than I do.
Attachment #549523 - Flags: review?(bzbarsky) → review?(jonas)
So what is the review status of this bug?
Unfortunately, this patch doesn't pass the try server, a lot of tests are failing:
http://tbpl.mozilla.org/?tree=Try&rev=46229062e6ab

Quentin, do you think you can have a look at those failures and try to see what is happening? It's whether the tests that are wrong or the patch that have unexpected side effects.
(In reply to comment #7)
> Unfortunately, this patch doesn't pass the try server, a lot of tests are
> failing:
> http://tbpl.mozilla.org/?tree=Try&rev=46229062e6ab
> 
> Quentin, do you think you can have a look at those failures and try to see
> what is happening? It's whether the tests that are wrong or the patch that
> have unexpected side effects.

Hmm strange. Thanks for pointing this out, as I would not have known which tests to run. I'll take a look at it.
OK, I think I see the issue here. Since my patch modified the very function that parses all integer attributes it likes to get rid of extra important letters such as "px" and "em" which specify sizes and locations for certain elements.

That's my theory anyway. I'm working on a new patch that will attempt to fix this.

P.S. I am going away this weekend and might not have net access. If I do, I'll submit the patch, if not, I'll wait till I come back.
Quentin, any news about the patch? :)
Sorry about the delay. I've applied a few days ago for level 1 try server access and I am still waiting for the confirmation. I am waiting until I can use the try server before working on the patch anymore. I want to test it myself before submitting it and breaking so many tests.

(In reply to Mounir Lamouri (:volkmar) from comment #10)
> Quentin, any news about the patch? :)
(In reply to Quentin Headen from comment #11)
> Sorry about the delay. I've applied a few days ago for level 1 try server
> access and I am still waiting for the confirmation. I am waiting until I can
> use the try server before working on the patch anymore. I want to test it
> myself before submitting it and breaking so many tests.

What's the bug number?
Ok. I've gotten my try server access a couple of days ago. So I am ready to start working back with this bug.
I'm sorry that I am being a bit slow on this bug. I've started going to college last week, so I am going to try to work on a bit this weekend. I can't promise a fix, but I hope I can bring one to the table.
This bug looks interesting to me. Quentin, If you are not able to work on this bug, can I work on it?
Assignee: qheaden → nobody
Be my guest. I've unassigned myself from the bug because I can't really find a solution to it. I'm positive it is because of my being a newbie to the project, but the fix wasn't as simple as I thought.

Take a shot at it and see if you can fix it.

(In reply to Atul Aggarwal from comment #15)
> This bug looks interesting to me. Quentin, If you are not able to work on
> this bug, can I work on it?
Found the problem. The problem is not in the nsAttrValue.cpp but in nsTStringObsolete.cpp (in function ToString line 213 and 232). 
The problem is that if an integer is followed by a-f or A-F, it is being treated as error and 0 is returned. All other characters work as per expectation. 
This can be verified by looking at http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A...%3Col%3E%3Cli%20value%3D22oo%3EBar%3C%2Fol%3E

I would create a patch to fix this problem soon and will attach it with this bug but access to try server will help me to test the patch as this kind of fixes are risky and I would like not to break any thing.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #556485 - Flags: review?(mounir)
Comment on attachment 556485 [details] [diff] [review]
Patch v1

I'm not sure if this solution is going to pass all tests (tests might be wrong though). In general, I would prefer a fix in nsAttrValue because I don't know how nsString::ToInteger should behave. I even think we should remove the call to ::ToInteger at the end of the method converting a string to an integer in nsAttrValue. Or call the same method used for parseInt.

Anyway, that might be a real bug in XPCOM String code so I'm moving the review to Benjamin.
Attachment #556485 - Flags: review?(mounir) → review?(benjamin)
volkmar, Can you please post this patch to test server to see if there are failures or not? I suspect some failures but this code might not have test coverage. Moreover it will help us understand the impact of this fix.
I understand some of the failures and will try to work to fix them in next patch.
Comment on attachment 556485 [details] [diff] [review]
Patch v1

I don't think that we should change the existing XPCOM method: I know of other callers which expect malformed numbers to report an error.
Attachment #556485 - Flags: review?(benjamin) → review-
Can I write another method to avoid the above problem and follow strictly http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-integers?

We can then use this method call in case of parsing attribute values.
Sounds like the best approach to me.
Isn't that the purpose of nsAttrValue::StringToInteger? I think we shouldn't call the XPCOM method at the end because it creates unwanted behavior but that should be another bug. I believe Quentin's patch was correct but we probably have to fix some tests or even some code. I would bet Quentin's patch and Atul's patch have more or less the same failures.
Atul, would you be interested to investigate the failures if I push Quentin's patch to try?
Attachment #556485 - Attachment is obsolete: true
Depends on: 684221
Ehsan and Fabien, any of you understand why this patch produce these errors:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6209405#error0

<font size="18"> or <font size="18px"> is the same thing though, the test expect <font size="18px"> but for some reasons, with Quentin's patch, the editor does not generate that...
Attached patch Fix tests (obsolete) — Splinter Review
This patch should fix most of the tests issues (with the patch in bug 684221). Likely, the editor failure will remain.
Sent to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=10085e2bbdd4
(In reply to Mounir Lamouri (:volkmar) from comment #28)
> Ehsan and Fabien, any of you understand why this patch produce these errors:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=6209405#error0
> 
> <font size="18"> or <font size="18px"> is the same thing though, the test
> expect <font size="18px"> but for some reasons, with Quentin's patch, the
> editor does not generate that...

This is the code in question: http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsComposerCommands.cpp#831, which I think ends up triggeting this code: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/ChangeAttributeTxn.cpp#94.  Somebody should debug it to figure out what's going wrong, but I don't want this test to regress because of this patch.
This patch tries to make attribute reading more conformed to what spec says (http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#signed-integers).

During several iteration of try to fix all the tests. Lastest being https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2d2c6a4def6b, I found some problem in the tests also. I have tried to fix them also. Changes in test are:

1. Setting -0, -0% to attribute value which is now returned as 0, 0%. I don't think this is wrong in any way. Moreover both are equivalent in integer form.

2. Some tests have to be enabled in canvas which is perfect.

3. Earlier algorithm was failing to read attribute value as "3em" ('xxem' format) due to treating 'e' as hex character due to which a reftest has to be updated. (Reference file looks like blunder to me).

Note this patch also fixes Bug 679672.
Assignee: nobody → atulagrwl
Attachment #549523 - Attachment is obsolete: true
Attachment #557832 - Attachment is obsolete: true
Attachment #559222 - Flags: review?(mounir)
Attachment #559222 - Flags: review?(jonas)
Comment on attachment 559222 [details] [diff] [review]
Patch v1 to fix attribute value problem.

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

I'm not setting r+ because there is a test you should not have changed (thus it's showing a failure) and I think this patch still fails some test.
Actually, I don't even understand why this patch doesn't fail as many test as Quentin's. I didn't compare the methods line by line though.

For the -0 -> 0 issue, I think the reason is you should special cased '-' followed by '0'. This is the case in the actual code and that should stay. The |strict| variable is used by the callers to know if they can use the integer value for the string representation or not. With '-0' being strict, they use the integer 0 to represent the string, which is '0' instead of '-0'.

So, could you refresh that, send to try and we will see what still needs to be fixed.

::: content/base/src/nsAttrValue.cpp
@@ +1406,5 @@
>    aValue.BeginReading(iter);
>    aValue.EndReading(end);
>    PRBool negate = PR_FALSE;
>    PRInt32 value = 0;
> +  PRInt32 pValue = 0;

What does pValue means? Reader shouldn't have to guess. Better to put a clear variable name or put a comment if that's hard to explain.

@@ +1412,5 @@
> +  //Skip whitespace characters and parse negative symbol
> +  while (iter != end) {
> +    if (nsContentUtils::IsHTMLWhitespace(*iter)) {
> +        ++iter;
> +    } else if (*iter == PRUnichar('-')) {

You should manage the '+' case in that loop.

@@ +1423,4 @@
>      }
>    }
>  
> +  while (iter != end && *aStrict) {

Use a for loop, that seems more appropriate.

@@ +1426,5 @@
> +  while (iter != end && *aStrict) {
> +    if (*iter >= PRUnichar('0') && *iter <= PRUnichar('9')) {
> +      value = (value * 10) + (*iter - PRUnichar('0'));
> +      ++iter;
> +      if (pValue > value) {

How could that happen?

@@ +1437,5 @@
> +      }
> +    } else if (aCanBePercent && *iter == PRUnichar('%')) {
> +      ++iter;
> +      if (iter == end) {
> +        *aIsPercent = PR_TRUE;

I don't think '%' is required to be at the end of the integer. I believe everything following should be ignored. Though, I don't know that part of the specs so I might be wrong.

::: content/base/test/test_bug433533.html
@@ +179,5 @@
>  is(iframe.getAttribute("marginwidth"), "-134217730%",
>     "Marginwidth attribute didn't store the original value");
>  
>  iframe.setAttribute("marginwidth", "-0");
> +is(iframe.getAttribute("marginwidth"), "0",

That is wrong. You shouldn't change this test.

@@ +184,3 @@
>     "Marginwidth attribute didn't store the original value");
>  iframe.setAttribute("marginwidth", "-0%");
> +is(iframe.getAttribute("marginwidth"), "0%",

Same thing here.

::: content/canvas/test/test_canvas.html
@@ +20189,5 @@
>  
>  var canvas = document.getElementById('c637');
>  var ctx = canvas.getContext('2d');
>  
> +ok(canvas.width == 100, "canvas.width == 100");

That should be changed to:
is(canvas.width, 100, ...);

@@ +20399,5 @@
>  var canvas = document.getElementById('c648');
>  var ctx = canvas.getContext('2d');
>  
>  canvas.setAttribute('width', '100foo');
> +ok(canvas.width == 100, "canvas.width == 100");

ditto
Attachment #559222 - Flags: review?(mounir)
Attachment #559222 - Flags: review?(jonas)
Attachment #559222 - Flags: feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
I have tried to solve all the issues raised except the loop (for loop). I like while loop while using iterators :). However if you still feel like changing loop, I can do that.
Attachment #560672 - Flags: review?(mounir)
Comment on attachment 560672 [details] [diff] [review]
Patch v2

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

The patch seems correct but you have to check that tinderbox is still green (currently your patch is red because of a build bustage).
I'm particularly interested to know if the change in '%' will break stuff or not. Did you check if the change matches the specs? (at least what other browsers do?)

r=me with green tests and a check for the change of percentage parsing.
Though, my review isn't enough to push your patch to m-c (I'm no DOM peer). Jonas seems the more appropriate to review this.
Asking a feedback to Ms2ger in case he has something to add.

::: content/base/src/nsAttrValue.cpp
@@ +1450,5 @@
> +  }
> +  if (negate) {
> +    value = -value;
> +    // Checking the special case of -0.
> +    if (!value) {

nit: I would check (value == 0) but that's only a syntax preference.
Attachment #560672 - Flags: review?(mounir)
Attachment #560672 - Flags: review?(jonas)
Attachment #560672 - Flags: review+
Attachment #560672 - Flags: feedback?(Ms2ger)
Attached patch Patch v3 (obsolete) — Splinter Review
Sorry, I forgot to qrefresh before I pulled the patch form hg directory. I have asked on irc for pushing the patch to try server. As soon I got the link, I will post it to bug.
Comment on attachment 560680 [details] [diff] [review]
Patch v3

Do we need to set aStrict to false if skipping whitespace or '+'? Needs tests.

For the first loop, I think I'd prefer something like

while (iter != end && nsContentUtils::IsHTMLWhitespace(*iter)) {
  ++iter;
}

if (iter == end) {
  return 0;
}

bool negate = false;
if (*iter == PRUnichar('-')) {
  negate = true;
  ++iter;
} else if (*iter == PRUnichar('+')) {
  ++iter;
}

What do you think? Also add tests for +-1, -+1, ++1, --1.

Please revert your ok() -> is() changes in test_canvas; it should remain as close as possible to the original code.
Attachment #560680 - Flags: review?(jonas)
Attachment #560680 - Flags: feedback+
Attachment #560672 - Attachment is obsolete: true
Attachment #560672 - Flags: review?(jonas)
Attachment #560672 - Flags: feedback?(Ms2ger)
Comment on attachment 560680 [details] [diff] [review]
Patch v3

David, it looks like you wrote this reftest, could you check if the change is correct?
Attachment #560680 - Flags: feedback?(dbaron)
And please check if using mozilla::CheckedInt would make the code cleaner.
(In reply to Ms2ger from comment #38)
> Comment on attachment 560680 [details] [diff] [review]
> Patch v3
> 
> Do we need to set aStrict to false if skipping whitespace or '+'? Needs
> tests.
> 
I have no idea how is aStrict is being used by the client. Currently, I soon I get an valid digit, I set it to FALSE. I am relying on the expertise of you guys to tell me what to do.

> For the first loop, I think I'd prefer something like
> 
> while (iter != end && nsContentUtils::IsHTMLWhitespace(*iter)) {
>   ++iter;
> }
> 
> if (iter == end) {
>   return 0;
> }
> 
> bool negate = false;
> if (*iter == PRUnichar('-')) {
>   negate = true;
>   ++iter;
> } else if (*iter == PRUnichar('+')) {
>   ++iter;
> }
> 
> What do you think? Also add tests for +-1, -+1, ++1, --1.
> 
I completely agree this looks much cleaner than my version. Shouldn't test be covered in reflectInt or reflectUnsignedInt tests?

> Please revert your ok() -> is() changes in test_canvas; it should remain as
> close as possible to the original code.

I will wait for David feedback and will attach new version of patch including above suggestions.
(In reply to Ms2ger from comment #40)
> And please check if using mozilla::CheckedInt would make the code cleaner.

I am thinking it will be kind of overkill to use CheckedInt and will not make code cleaner as we can do the same task by checking a single check. I think, CheckedInt will be proper solution when we have to perform several operations but here we need ot perform a single operation.

What do you think?
(In reply to Atul Aggarwal from comment #41)
> (In reply to Ms2ger from comment #38)
> > Comment on attachment 560680 [details] [diff] [review]
> > Patch v3
> > 
> > Do we need to set aStrict to false if skipping whitespace or '+'? Needs
> > tests.
> > 
> I have no idea how is aStrict is being used by the client. Currently, I soon
> I get an valid digit, I set it to FALSE. I am relying on the expertise of
> you guys to tell me what to do.

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.h#375

375   // aStrict is set PR_TRUE if stringifying the return value equals with
376   // aValue.

is all I know.

> > For the first loop, I think I'd prefer something like
> > 
> > while (iter != end && nsContentUtils::IsHTMLWhitespace(*iter)) {
> >   ++iter;
> > }
> > 
> > if (iter == end) {
> >   return 0;
> > }
> > 
> > bool negate = false;
> > if (*iter == PRUnichar('-')) {
> >   negate = true;
> >   ++iter;
> > } else if (*iter == PRUnichar('+')) {
> >   ++iter;
> > }
> > 
> > What do you think? Also add tests for +-1, -+1, ++1, --1.
> > 
> I completely agree this looks much cleaner than my version. Shouldn't test
> be covered in reflectInt or reflectUnsignedInt tests?

Yeah, makes sense.

> > Please revert your ok() -> is() changes in test_canvas; it should remain as
> > close as possible to the original code.
> 
> I will wait for David feedback and will attach new version of patch
> including above suggestions.

OK.

(In reply to Atul Aggarwal from comment #42)
> (In reply to Ms2ger from comment #40)
> > And please check if using mozilla::CheckedInt would make the code cleaner.
> 
> I am thinking it will be kind of overkill to use CheckedInt and will not
> make code cleaner as we can do the same task by checking a single check. I
> think, CheckedInt will be proper solution when we have to perform several
> operations but here we need ot perform a single operation.
> 
> What do you think?

Fair enough.
(In reply to Ms2ger from comment #43)
> (In reply to Atul Aggarwal from comment #41)
> > (In reply to Ms2ger from comment #38)
> > > Comment on attachment 560680 [details] [diff] [review]
> > > Patch v3
> > > 
> > > Do we need to set aStrict to false if skipping whitespace or '+'? Needs
> > > tests.
> > > 
> > I have no idea how is aStrict is being used by the client. Currently, I soon
> > I get an valid digit, I set it to FALSE. I am relying on the expertise of
> > you guys to tell me what to do.
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.
> h#375
> 
> 375   // aStrict is set PR_TRUE if stringifying the return value equals with
> 376   // aValue.
> 
> is all I know.

If we need to set *aStrict to false, reflectInt() tests should catch that.

> > > Please revert your ok() -> is() changes in test_canvas; it should remain as
> > > close as possible to the original code.

Keep is() please. ok() was wrong and should be changed.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #32)
> Actually, I don't even understand why this patch doesn't fail as many test
> as Quentin's. I didn't compare the methods line by line though.

I finally understand that. Quentin's patch is removing *aStrict = PR_FALSE making "42foo" strictly parsed as 42. That means getAttribute() will return "42" in that case. Which isn't what we want. Unfortunately, the way the method was written was so broken it would have been hard to fix it without re-writing it entirely like Atul did.

BTW, the patch seems to be green on try. I will give it a try with the WIP reflectInt() patch in bug 669578.
Summary: Stop parsing integers when something elso than [0-9] is found → Stop parsing integers when something else than [0-9] is found
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #44)
> (In reply to Ms2ger from comment #43)
> > (In reply to Atul Aggarwal from comment #41)
> > > (In reply to Ms2ger from comment #38)
> > > > Comment on attachment 560680 [details] [diff] [review]
> > > > Patch v3
> > > > 
> > > > Do we need to set aStrict to false if skipping whitespace or '+'? Needs
> > > > tests.
> > > > 
> > > I have no idea how is aStrict is being used by the client. Currently, I soon
> > > I get an valid digit, I set it to FALSE. I am relying on the expertise of
> > > you guys to tell me what to do.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.
> > h#375
> > 
> > 375   // aStrict is set PR_TRUE if stringifying the return value equals with
> > 376   // aValue.
> > 
> > is all I know.
> 
> If we need to set *aStrict to false, reflectInt() tests should catch that.

And they did catch that ;) So you should fix that too.
Changes suggested by Ms2ger and volkmar.
Attachment #560722 - Flags: review?(jonas)
Attachment #560722 - Flags: feedback?(dbaron)
Attachment #560722 - Attachment is obsolete: true
Attachment #560722 - Flags: review?(jonas)
Attachment #560722 - Flags: feedback?(dbaron)
Attached patch Patch v5 (obsolete) — Splinter Review
Attachment #560727 - Flags: review?(jonas)
Attachment #560727 - Flags: feedback?(dbaron)
Attachment #559222 - Attachment is obsolete: true
Comment on attachment 560680 [details] [diff] [review]
Patch v3

Usually, when you attach a new patch you mark the previous one obsolete even if someone did f+/r+ it. You will just have to keep track of those when setting the commit message.
Attachment #560680 - Attachment is obsolete: true
Attachment #560680 - Flags: review?(jonas)
Attachment #560680 - Flags: feedback?(dbaron)
Comment on attachment 560727 [details] [diff] [review]
Patch v5

r=dbaron on the change to layout/reftests/bugs/539880-1-ref.html
Attachment #560727 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 560727 [details] [diff] [review]
Patch v5

If Mounir reviewed this, then that's enough for me.
Attachment #560727 - Flags: review?(jonas)
Thanks Mounir for clarifying about marking attachment obsolete. So Is this patch good to go? I don't see any review pending but there isn't any review done in current version of patch.
This patch is good to go, you can consider my f+ as a r+ per comment 51.
Though, I would like to push reflectInt() tests first and I prefer to push that patch after the branching that is going to come soon: next tuesday, mozilla-central will be Firefox 10 code instead of Firefox 9. If we push that patch for Firefox 10, users will have more time to report the regressions that might happen. The chance are low but I don't think we will win anything by urging the push.
I completely agree. We should give sufficient time to users to address any regressions (if any).
Keywords: dev-doc-needed
Target Milestone: --- → mozilla10
Atul, reflectInt() tests have been pushed. I think you will have to update it to remove some todo's this patch is fixing. You can easily find the tests that need to be changed by running in your objdir:
TEST_PATH=content/html/content/tests make mochitest-plain

(it will run our HTML tests and you will see some failures)
Working on it right now. I will create the patch of modified test separately than source change and will get it reviewed.
Attached patch Fixing test failures (obsolete) — Splinter Review
This patch only contains the test failures in the reflectInt.
Attachment #562223 - Flags: review?(mounir)
Comment on attachment 562223 [details] [diff] [review]
Fixing test failures

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

r=me with a double-check for the {++2,+-2,--2-+2} cases.

::: content/html/content/test/reflect.js
@@ +558,4 @@
>      } else if ((v == "++2" || v == "+-2" || v == "--2" || v == "-+2") && element[attr] != defaultValue)  {
>        //TBD: Bug: Should not be able to parse strings with multiple sign characters, should return defaultValue
>        todo_is(element[attr], intValue, "Bug: " + element.localName +
>          ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");

Are you sure this todo isn't fixed by your patch? I think it is.
Attachment #562223 - Flags: review?(mounir) → review+
This should be fixed but I got all the test passed without fixing this todo. Let me check what is the root cause here.
Attached patch Fixing tests (v2) (obsolete) — Splinter Review
Attachment #562223 - Attachment is obsolete: true
Both patches pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=dca6ca6c6543

Assuming the push is green, I think you should merge those two patches to prevent having a version of the tree that does not pass tests (in case of someone bisecting for example).

Also, when you update the patch, please add the f/r information so it will be ready to be pushed.
Attached patch Final source and test case patch (obsolete) — Splinter Review
Try server is a green. Ready for check-in once new branch is emerged.
Attachment #560727 - Attachment is obsolete: true
Attachment #562294 - Attachment is obsolete: true
Verified with diff (with previous patch) only bool changes are there in this patch.
Attachment #562310 - Attachment is obsolete: true
Comment on attachment 563963 [details] [diff] [review]
Rebase with the bool change

># HG changeset patch
># Parent 164fd1bbd06f7e1e0f501469043d53f703e3039b
># User Atul Aggarwal <atulagrwl@gmail.com>
># Date 1317477627 -19800
>Bug 673820 - Rewriting StringToInteger function to match behavior of Rules for parsing Integers specified in spec. r=jonas f=mounir,Ms2ger,dbaron
>   bool negate = false;
>   PRInt32 value = 0;
>+  PRInt32 pValue = 0; // Previous value, used to check integer overflow

I just noticed that these could be declared a bit later. I've fixed that locally and will push tomorrow.

Thanks again for taking this on!
https://hg.mozilla.org/mozilla-central/rev/423728a5c37a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
No longer blocks: 679672
Duplicate of this bug: 679672
This change is noted in Firefox 10 for developers.
You need to log in before you can comment on or make changes to this bug.