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

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

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

Comment 1

[Quentin Headen [:qheaden]]

Attachment: Diff to fix bug

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.

Comment 2

[Mounir Lamouri (:mounir)]

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.

Comment 3

[Mounir Lamouri (:mounir)]

Thank you for working on this BTW :)

Comment 4

[Quentin Headen [:qheaden]]

Attachment: Revision to the previous patch

Updated the patch with the suggestions given. It came down to becoming a one line change.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 549523 [details] [diff] [review]
Revision to the previous patch

Jonas knows this stuff better than I do.

Comment 6

[Quentin Headen [:qheaden]]

So what is the review status of this bug?

Comment 7

[Mounir Lamouri (:mounir)]

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.

Comment 8

[Quentin Headen [:qheaden]]

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

Comment 9

[Quentin Headen [:qheaden]]

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.

Comment 10

[Mounir Lamouri (:mounir)]

Quentin, any news about the patch? :)

Comment 11

[Quentin Headen [:qheaden]]

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? :)

Comment 12

[Mounir Lamouri (:mounir)]

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

Comment 13

[Quentin Headen [:qheaden]]

Ok. I've gotten my try server access a couple of days ago. So I am ready to start working back with this bug.

Comment 14

[Quentin Headen [:qheaden]]

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.

Comment 15

[Atul Aggarwal]

This bug looks interesting to me. Quentin, If you are not able to work on this bug, can I work on it?

Comment 16

[Quentin Headen [:qheaden]]

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?

Comment 17

[Atul Aggarwal]

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.

Comment 18

[Atul Aggarwal]

Attachment: Patch v1

Comment 19

[Mounir Lamouri (: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.

Comment 20

[Atul Aggarwal]

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.

Comment 21

[Mounir Lamouri (:mounir)]

Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=20c30575bbb2

Comment 22

[Atul Aggarwal]

I understand some of the failures and will try to work to fix them in next patch.

Comment 23

[Benjamin Smedberg]

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.

Comment 24

[Atul Aggarwal]

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.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sounds like the best approach to me.

Comment 26

[Mounir Lamouri (:mounir)]

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?

Comment 27

[Mounir Lamouri (:mounir)]

Quentin patch re-pushed to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ce14a41f9e18

Comment 28

[Mounir Lamouri (:mounir)]

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

Comment 29

[Mounir Lamouri (:mounir)]

Attachment: Fix tests

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

Comment 30

[(no longer active)]

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

Comment 31

[Atul Aggarwal]

Attachment: Patch v1 to fix attribute value problem.

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.

Comment 32

[Mounir Lamouri (:mounir)]

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

Comment 33

[Atul Aggarwal]

Attachment: Patch v2

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.

Comment 34

[Atul Aggarwal]

https://tbpl.mozilla.org/?tree=Try&rev=4e39b3f3f7db

Comment 35

[Mounir Lamouri (: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.

Comment 36

[Atul Aggarwal]

Attachment: Patch v3

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 37

[Ed Morley [:emorley]]

Comment on attachment 560680 [details] [diff] [review]
Patch v3

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=6d324df55204

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And please check if using mozilla::CheckedInt would make the code cleaner.

Comment 41

[Atul Aggarwal]

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

Comment 42

[Atul Aggarwal]

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

Comment 43

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 44

[Mounir Lamouri (:mounir)]

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

Comment 45

[Mounir Lamouri (:mounir)]

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

Comment 46

[Mounir Lamouri (:mounir)]

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

Comment 47

[Atul Aggarwal]

Attachment: Patch v4 (hopefully final version)

Changes suggested by Ms2ger and volkmar.

Comment 48

[Atul Aggarwal]

Attachment: Patch v5

Comment 49

[Mounir Lamouri (:mounir)]

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.

Comment 50

[David Baron :dbaron:]

Comment on attachment 560727 [details] [diff] [review]
Patch v5

r=dbaron on the change to layout/reftests/bugs/539880-1-ref.html

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 560727 [details] [diff] [review]
Patch v5

If Mounir reviewed this, then that's enough for me.

Comment 52

[Atul Aggarwal]

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.

Comment 53

[Mounir Lamouri (:mounir)]

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.

Comment 54

[Atul Aggarwal]

I completely agree. We should give sufficient time to users to address any regressions (if any).

Comment 55

[Mounir Lamouri (:mounir)]

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)

Comment 56

[Atul Aggarwal]

Working on it right now. I will create the patch of modified test separately than source change and will get it reviewed.

Comment 57

[Atul Aggarwal]

Attachment: Fixing test failures

This patch only contains the test failures in the reflectInt.

Comment 58

[Mounir Lamouri (: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.

Comment 59

[Atul Aggarwal]

This should be fixed but I got all the test passed without fixing this todo. Let me check what is the root cause here.

Comment 60

[Atul Aggarwal]

Attachment: Fixing tests (v2)

Comment 61

[Mounir Lamouri (:mounir)]

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.

Comment 62

[Atul Aggarwal]

Attachment: Final source and test case patch

Try server is a green. Ready for check-in once new branch is emerged.

Comment 63

[Atul Aggarwal]

Attachment: Rebase with the bool change

Verified with diff (with previous patch) only bool changes are there in this patch.

Comment 64

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 563963 [details] [diff] [review]
Rebase with the bool change

># HG changeset patch
># Parent 164fd1bbd06f7e1e0f501469043d53f703e3039b
># User Atul Aggarwal <[e-mail]>
># 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!

Comment 65

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/423728a5c37a

Comment 67

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

FTR, this fixed

http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.parse.em.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.parse.exp.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.parse.hex.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.setAttribute.em.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.setAttribute.exp.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/size.attributes.setAttribute.hex.html

Comment 68

[Eric Shepherd [:sheppy]]

This change is noted in Firefox 10 for developers.