1009793 - [Window Management] In TaskManager, use appWindow's instanceID not origin to lookup cards

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Sam Foster [:sfoster] (he/him)]

The TaskManager class implemented in bug 935260 uses app origin as a key to lookup cards (e.g. taskManager.cardsByOrigin). It should use the instanceID on the AppWindow instead.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Should be mostly trivial, though some tests will need to be reworked as well, so p=2

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: Use appWindow.instanceID instead of origin to associate app with card

Pretty trivial. Unit tests passing locally, tbpl/gaia-try pending. I want to rebase 1045784 onto this rather than conflate these 2 bugs.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8471004 [details] [review]
Use appWindow.instanceID instead of origin to associate app with card

nit: cardsByAppID is redundant. I think cards is enough. But I am not strongly against it. 

And I am against using dataset here. My intention is using card object and this.cards to do anything you want in taskManager but not traversing the dom tree every time to collect the whole cards. This could be another followup.

Comment 4

[Sam Foster [:sfoster] (he/him)]

this.cards is just an array of Card instances that parallels this.stack. We do need an id or dataset property that lets us retrieve the app from the card element without having to iterate either this.cards or this.stack. The picture changes though in the new task switcher where we deal with appWindows directly not cards so I'll land this as-is and hopefully we'll get the new stuff in good enough shape that we can turn off or even remove the old cards view and this code in this release.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Correction, we don't even have a this.cards right now - my faulty memory  - we just have cardsByAppId, stack and the cardList.childNodes. I agree with trying to get away from using dataset. To do that fully though we'll need to rewrite the python ui tests which rely on data-origin selector matches. There are probably other cases where we have to resolve an app from an element quickly.  The tests will need update regardless as we switch to using the appWindows. I've fixed the nits you pointed out and another element.dataset use - lots of this is legacy code and I try as a rule to only fix what needs fixing in any given patch to reduce the surface area for regression and new bugs. I'll carry the r+ and land as soon as tbpl is green (or as green as it ever gets these days)

Comment 6

[Sam Foster [:sfoster] (he/him)]

Landed, merge: 0ff9451520a8c43e5efa82081f4b39930b1bc044
cset: 9ad1c4919d66910c8c60e244a7e79b8b2997b1eb

I also fixed up the logic in getCardAtIndex and a test or 2 that used it so that they use the appById lookup rather than the roundabout route of going to the element.dataset.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Landing link https://github.com/mozilla-b2g/gaia/commit/0ff9451520a8c43e5efa82081f4b39930b1bc044