1027347 - downloadError on individual app should not always be an error object

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[James Lal [:lightsofapollo]]

I think the intent here should be .downloadError contains the last error for that particular app if there was an error downloading. The current logic https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#435 is always return a download error but the name is blank if there was no error.

Comment 1

[Jason Smith [:jsmith]]

James - Should this be marked as 2.0+ given that this blocks bug 1027935? If so, can you nom this for 2.0?

Comment 2

[James Lal [:lightsofapollo]]

Attachment: Only set downloadError to an error if it actually is an error and clear past errors after successful states

Comment 3

[James Lal [:lightsofapollo]]

Try:

https://tbpl.mozilla.org/?tree=Try&rev=7f68d1f91d65

I did a potentially sketchy thing which is update state on the child process side for the downloadError during our success events... This has a big upside which is we don't need to explicitly clear the error all over in Webapps.jsm (I still clear there but for the beginning of downloads)

Comment 4

[James Lal [:lightsofapollo]]

Attachment: Only set downloadError to an error if it actually is an error and clear past errors after successful states

Comment 5

[James Lal [:lightsofapollo]]

Updated to actually override error to null if .error was sent. Might be better to just always set .downloadError to null if .error is sent.

https://tbpl.mozilla.org/?tree=Try&rev=6b9b27fddeed

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8446335 [details] [diff] [review]
Only set downloadError to an error if it actually is an error and clear past errors after successful states

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

I just want to take another quick look before landing.

::: dom/apps/src/Webapps.js
@@ +589,5 @@
>          this[prop] = aMsg.app[prop];
>        }
>      }
>  
> +    if ('error' in aMsg) {

it's actually slower than the original.

@@ +648,5 @@
> +          // If we are in a successful state clear any past errors.
> +          if (
> +            aEventType === 'downloadapplied' ||
> +            aEventType === 'downloadsuccess'
> +          ) {

nit: the usual style is :
if (aEventType === 'downloadapplied' ||
    aEventType === 'downloadsuccess') {

@@ +649,5 @@
> +          if (
> +            aEventType === 'downloadapplied' ||
> +            aEventType === 'downloadsuccess'
> +          ) {
> +            dump('Webapps.js - clear error\n');

No unconditional dump() please.

Comment 7

[James Lal [:lightsofapollo]]

re the use of 'in' I am not trying to check for "is it truthy" I am trying to check for "does the property exist" so when I set it to null it will be picked up.

dump is a typo new patch in a sec.

Comment 8

[James Lal [:lightsofapollo]]

Attachment: Only set downloadError to an error if it actually is an error and clear past errors after successful states

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8446355 [details] [diff] [review]
Only set downloadError to an error if it actually is an error and clear past errors after successful states

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

::: dom/apps/src/Webapps.js
@@ +649,5 @@
> +          // If we are in a successful state clear any past errors.
> +          if (
> +            aEventType === 'downloadapplied' ||
> +            aEventType === 'downloadsuccess'
> +          ) {

you did not change the style there.

Comment 10

[James Lal [:lightsofapollo]]

Attachment: Only set downloadError to an error if it actually is an error and clear past errors after successful states

Comment 11

[James Lal [:lightsofapollo]]

Sorry not sure how I missed that comment...

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1836f9b10537

Comment 13

[James Lal [:lightsofapollo]]

I am greedy and want to be the assignee this this work is already done pending central landing... I will watch it until then.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1836f9b10537

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2016c180a540