Webapp runtime download tests are failing

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

Trunk
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

No description provided.
Posted 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: 4 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.