1116769 - Webapp runtime download tests are failing

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Marco Castelluccio [:marco]]

This is a partial backout of one of the patches from bug 1108310.

Comment 3

[Marco Bonardo [:mak]]

looks like downloadItems should be a Map instead, and the iteration could go through .values()

Comment 4

[Marco Castelluccio [:marco]]

I'd be happy to take a patch that fixes bug 1108310 without introducing regressions, but unfortunately I don't have time to do it and test that the patch works, so this is just a straight backout to avoid the regression introduced by bug 1108310.

Comment 5

[Chris Peterson [:cpeterson]]

Sorry for breaking this test, Marco. I just landed your backout and I'm investigating a real fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/99dfdc6bdee8

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: downloadItems-map.patch

:marco, do you mind testing this patch? I can't run the webapprt-test-chrome tests locally due to OS X bug 1097243.

This patch changes downloadItems from an object to a Map (as suggested in comment 3) and extracts the repeated this.downloadItems.get() calls into two helper functions: _getDownloadItemForElement() and _getSelectedDownloadItem().

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99dfdc6bdee8

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 8544442 [details] [diff] [review]
downloadItems-map.patch

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

> Sorry for breaking this test, Marco.

No worries, the tests are not yet run automatically, so it's difficult to catch regressions!

I've run the tests and they're passing with your patch applied. It'd be great if you could also test this manually, since the tests' coverage isn't so high.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8544442 [details] [diff] [review]
downloadItems-map.patch

Change webapprt downloadItems to a Map and remove nonstandard for-each-in loops.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8544442 [details] [diff] [review]
downloadItems-map.patch

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

::: webapprt/content/downloads/downloads.js
@@ +577,5 @@
>  };
>  
>  let gDownloadList = {
>    downloadItemsMap: new Map(),
> +  downloadItems: new Map(),

To avoid confusion with downloadItemsMap, I'd suggest renaming downloadItems to idToDownloadItemMap

I'm sure with some refactoring it would also be possible to avoid the double map and the id attribute, but that's out of scope here.

Comment 11

[Chris Peterson [:cpeterson]]

Thanks. Landed with suggested name idToDownloadItemMap:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d40cc2407fa3

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d40cc2407fa3

Comment 13

[vkrishnamoorthy]

Marco is this bug fixed? if so can yo please close?