1273584 - Displayed device list pref grows forever

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I noticed recently that we are always appending the default displayed devices to the array, whether or not they are in there.

This means the array is always growing in size, with more and more duplicates.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1273584 - Stop growing device list forever. r=gl

Review commit: https://reviewboard.mozilla.org/r/53268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53268/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This version should work, but we may actually want a data structure that deduplicates.  At the moment, those of us who work on the tool would still have large pref data even with this change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8769e03479ab

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/53268/#review50052

::: devtools/client/responsive.html/devices.js:37
(Diff revision 1)
> -      if (newDevice.displayed) {
> +      if (newDevice.displayed && !deviceList.includes(newDevice.name)) {
>          deviceList.push(newDevice.name);

Switch this to a JavaScript Set and you'll get your deduplicating and won't have to add this condition.

You can then use Array.from(deviceSet) to get an array out of it for any external APIs.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/53268/#review50052

> Switch this to a JavaScript Set and you'll get your deduplicating and won't have to add this condition.
> 
> You can then use Array.from(deviceSet) to get an array out of it for any external APIs.

Yep, a Set had ocurred to me too!  I couldn't remember if you can iterate in insertion order, but it looks like you can.  I'll give it a try.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8753434 [details]
MozReview Request: Bug 1273584 - Stop growing device list forever. r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53268/diff/1-2/

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8034421601fe

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8753434 [details]
MozReview Request: Bug 1273584 - Stop growing device list forever. r=gl

https://reviewboard.mozilla.org/r/53268/#review50564

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a3eb9950fa4b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for WinXP browser_responsiveui_touch.js failures.
https://hg.mozilla.org/integration/fx-team/rev/2013e1255bf8

https://treeherder.mozilla.org/logviewer.html#?job_id=9436369&repo=fx-team#L2930

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6de3dcf485

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems to be fine in my WinXP try run above, let's try landing again.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3aa793fa2d0b

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3aa793fa2d0b