Closed Bug 1116769 Opened 6 years ago Closed 6 years ago

Webapp runtime download tests are failing

Categories

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

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(2 files)

No description provided.
Attached patch PatchSplinter Review
Attachment #8542975 - Flags: review?(myk)
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
This is a partial backout of one of the patches from bug 1108310.
looks like downloadItems should be a Map instead, and the iteration could go through .values()
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.
Attachment #8542975 - Flags: review?(myk) → review+
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
Blocks: 1108310
Whiteboard: [leave open]
: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().
Attachment #8544442 - Flags: feedback?(mar.castelluccio)
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.
Attachment #8544442 - Flags: feedback?(mar.castelluccio) → feedback+
No longer blocks: 899707
Comment on attachment 8544442 [details] [diff] [review]
downloadItems-map.patch

Change webapprt downloadItems to a Map and remove nonstandard for-each-in loops.
Attachment #8544442 - Flags: review?(mak77)
Priority: -- → P1
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.
Attachment #8544442 - Flags: review?(mak77) → review+
Thanks. Landed with suggested name idToDownloadItemMap:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d40cc2407fa3
Marco is this bug fixed? if so can yo please close?
Flags: needinfo?(mar.castelluccio)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(mar.castelluccio)
Resolution: --- → FIXED
Whiteboard: [leave open]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.