Mounir Lamouri (:mounir) Reporter
	Description • 13 years ago

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

	Quentin Headen [:qheaden]
	Comment 1 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 2 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 3 • 13 years ago

Thank you for working on this BTW :)

	Quentin Headen [:qheaden]
	Comment 4 • 13 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 13 years ago

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

Jonas knows this stuff better than I do.

	Quentin Headen [:qheaden]
	Comment 6 • 13 years ago

So what is the review status of this bug?

	Mounir Lamouri (:mounir) Reporter
	Comment 7 • 13 years ago

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.

	Quentin Headen [:qheaden]
	Comment 8 • 13 years ago

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

	Quentin Headen [:qheaden]
	Comment 9 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 10 • 13 years ago

Quentin, any news about the patch? :)

	Quentin Headen [:qheaden]
	Comment 11 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 12 • 13 years ago

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

	Quentin Headen [:qheaden]
	Comment 13 • 13 years ago

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

	Quentin Headen [:qheaden]
	Comment 14 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 15 • 13 years ago

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

	Quentin Headen [:qheaden]
	Comment 16 • 13 years ago

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?

	Atul Aggarwal Assignee
	Comment 17 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 19 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 20 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 21 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 22 • 13 years ago

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

	Benjamin Smedberg
	Comment 23 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 24 • 13 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 25 • 13 years ago

Sounds like the best approach to me.

	Mounir Lamouri (:mounir) Reporter
	Comment 26 • 13 years ago

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?

	Mounir Lamouri (:mounir) Reporter
	Comment 27 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 28 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 29 • 13 years ago

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

	(no longer active)
	Comment 30 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 31 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 32 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 33 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 34 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 35 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 36 • 13 years ago

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.

	Ed Morley [:emorley]
	Comment 37 • 13 years ago

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

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 38 • 13 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 39 • 13 years ago

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?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 40 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 41 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 42 • 13 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 43 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 44 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 45 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 46 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 47 • 13 years ago

Changes suggested by Ms2ger and volkmar.

	Mounir Lamouri (:mounir) Reporter
	Comment 49 • 13 years ago

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.

	David Baron :dbaron:
	Comment 50 • 13 years ago

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

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 51 • 13 years ago

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

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

	Atul Aggarwal Assignee
	Comment 52 • 13 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 53 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 54 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 55 • 13 years ago

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)

	Atul Aggarwal Assignee
	Comment 56 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 57 • 13 years ago

This patch only contains the test failures in the reflectInt.

	Mounir Lamouri (:mounir) Reporter
	Comment 58 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 59 • 13 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 61 • 13 years ago

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.

	Atul Aggarwal Assignee
	Comment 62 • 13 years ago

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

	Atul Aggarwal Assignee
	Comment 63 • 13 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 64 • 13 years ago

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!

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 65 • 13 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 67 • 13 years ago

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

	Eric Shepherd [:sheppy]
	Comment 68 • 13 years ago

This change is noted in Firefox 10 for developers.