Julien Wajsberg [:julienw] Assignee
	Description • 12 years ago

In the App Install Manager, we use the following code to make a progress node indeterminate :

progressNode.value = undefined;

In Firefox Nightly (21) this is now forbidden and it fails with :

     TypeError: Floating-point value is not finite.
      at ai_handleProgress (http://system.gaiamobile.org:8080/js/app_install_manager.js:308)

This is not clear how this should be done instead. This is most probably related to the issue https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 so I'm CCing Mounir on this.

Note also that I find very strange that to know that a progress bar is indeterminate, we ask for (node.position === -1) but to set a progress bar indeterminate we have to fiddle with the value attribute.

	Julien Wajsberg [:julienw] Assignee
	Comment 1 • 12 years ago

Note: this is not a problem now because we use a gecko18 branch but this will be as soon as we change the branch. For example, merely running the unit tests in Fx nightly triggers this.

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

Looks like a regression from bug 839033. We should probably make those unrestricted for now... Mounir?

	Mounir Lamouri (:mounir)
	Comment 3 • 12 years ago

(In reply to Julien Wajsberg [:julienw] from comment #0)
> In the App Install Manager, we use the following code to make a progress
> node indeterminate :
> 
> progressNode.value = undefined;

Are you doing that instead of progressNode.removeAttribute('value'); because it is easier to write?

> This is not clear how this should be done instead. This is most probably
> related to the issue https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 so

What's your opinion on that?

> Note also that I find very strange that to know that a progress bar is
> indeterminate, we ask for (node.position === -1) but to set a progress bar
> indeterminate we have to fiddle with the value attribute.

Would you prefer a 'indeterminate' attribute?

	Mounir Lamouri (:mounir)
	Comment 4 • 12 years ago

(In reply to :Ms2ger from comment #2)
> Looks like a regression from bug 839033. We should probably make those
> unrestricted for now... Mounir?

They can simply call .removeAttribute('value') to work around this issue. I am not sure if we should change our behaviour now and hope that the specification will follow. In https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 Hixie didn't express a lot of interest in that (the fact that .undefined isn't allowed is highly related with that spec bug).

	Julien Wajsberg [:julienw] Assignee
	Comment 5 • 12 years ago

(In reply to Mounir Lamouri (:mounir) from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > In the App Install Manager, we use the following code to make a progress
> > node indeterminate :
> > 
> > progressNode.value = undefined;
> 
> Are you doing that instead of progressNode.removeAttribute('value'); because
> it is easier to write?

Because I felt awkward to modify the DOM whereas I wanted to change the object's state. Plus I thought it was equivalent (but the spec is really not clear on how DOM attribute values or absence translates to IDL).

> 
> > This is not clear how this should be done instead. This is most probably
> > related to the issue https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 so
> 
> What's your opinion on that?

The bug does not really deal with setting value from JavaScript. About handling invalid value specified in markup, I don't mind either way. But it should be valid to set "undefined" to the IDL-reflected property, which would mean as if the attribute is removed in the DOM. Maybe the rules [1] should be changed to have the following as first rule: "if 'undefined' is set to the property, remove the attribute".

> 
> > Note also that I find very strange that to know that a progress bar is
> > indeterminate, we ask for (node.position === -1) but to set a progress bar
> > indeterminate we have to fiddle with the value attribute.
> 
> Would you prefer a 'indeterminate' attribute?

As a read attribute it could be nice but (position === "-1") is good too. But it would also be consistent that getting the value would return |undefined|. Although I don't mind as soon as this is clearly specified, and I believe getting values is correctly specified now, only setting values is not clear and/or logical.

[1] http://www.w3.org/html/wg/drafts/html/master/infrastructure.html#best-representation-of-the-number-as-a-floating-point-number

	Julien Wajsberg [:julienw] Assignee
	Comment 6 • 12 years ago

- use removeAttribute instead of assigning undefined
---
 apps/system/js/app_install_manager.js |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The tests are passing again in nightly. I tried installing various apps, packaged or not, with or without a size, and this works correctly. Thus asking for approval too because otherwise tests will fail on current nightly, and this file is otherwise very well tested.

	Julien Wajsberg [:julienw] Assignee
	Comment 7 • 12 years ago

master: https://github.com/mozilla-b2g/gaia/commit/95d47f725af83e701aefa694d1a90ae7fe8a5d95

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 8 • 12 years ago

Uplifted commit 95d47f725af83e701aefa694d1a90ae7fe8a5d95 as:
v1-train: 26f9f0aac31dba983dfde1e94a7be18cd6887de8