Closed
Bug 1109511
Opened 9 years ago
Closed 9 years ago
Deleting a room while in-use leaves the Hello icon blue until the next time it's clicked
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(firefox35 verified, firefox36 verified, firefox37 verified)
People
(Reporter: jesup, Assigned: mikedeboer)
References
Details
Attachments
(1 file)
2.17 KB,
patch
|
NiKo
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
When deleting a room that's in-use leaves the Hello icon in the toolbar blue until the next time it's clicked. Minor consistency bug.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mmucci)
Assignee | ||
Comment 2•9 years ago
|
||
Niko, another review just for you; to welcome you to the team ;)
Attachment #8534967 -
Flags: review?(nperriault)
Comment on attachment 8534967 [details] [diff] [review] Patch v1: listen for the 'delete' event to update the toolbar icon when a room is deleted Review of attachment 8534967 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and the patch works fine. ::: browser/components/loop/LoopRooms.jsm @@ +131,5 @@ > /** > * @var {Number} participantsCount The total amount of participants currently > * inside all rooms. > */ > get participantsCount() { Nit, unrelated to this very patch: As a newcomer to this code, I think it's worth adding a comment that this value is used in browser/base/content/browser-loop.js and/or LoopUI#updateToolbarState. ::: browser/components/loop/MozLoopService.jsm @@ +1067,5 @@ > > // The Loop toolbar button should change icon when the room participant count > // changes from 0 to something. > + const onRoomsChange = (e) => { > + MozLoopServiceInternal.notifyStatusChanged("room-" + e); Nit: Could you please just confirm this string is passed as a reason for logging purpose only? If so, that's probably worth a comment.
Attachment #8534967 -
Flags: review?(nperriault) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #3) > Nit, unrelated to this very patch: As a newcomer to this code, I think it's > worth adding a comment that this value is used in > browser/base/content/browser-loop.js and/or LoopUI#updateToolbarState. I'm not sure how that would help, because that type of comment would add the burden of keeping it up-to-date when we use it somewhere else... I'm pretty sure that you figured it out too with a code search (a la `ack` or Sublime Text internal) scoped to the browser/components/loop/ directory, which takes ~2secs. > Nit: Could you please just confirm this string is passed as a reason for > logging purpose only? If so, that's probably worth a comment. Logging purposes only, I'll add a comment.
Assignee | ||
Comment 5•9 years ago
|
||
Thanks for the review! Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/e4e65a1c0dd6
https://hg.mozilla.org/mozilla-central/rev/e4e65a1c0dd6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8534967 [details] [diff] [review] Patch v1: listen for the 'delete' event to update the toolbar icon when a room is deleted Approval Request Comment [Feature/regressing bug #]: Rooms [User impact if declined]: Minor inconsistency in UI [Describe test coverage new/current, TBPL]: on m-c momentarily. manual test (easy) [Risks and why]: Very low risk patch. We don't have to take it for beta, but given the low risk I'm nominating it. [String/UUID change made/needed]: none
Attachment #8534967 -
Flags: approval-mozilla-beta?
Attachment #8534967 -
Flags: approval-mozilla-aurora?
Comment 8•9 years ago
|
||
Comment on attachment 8534967 [details] [diff] [review] Patch v1: listen for the 'delete' event to update the toolbar icon when a room is deleted low risk and since it's still early, we can take it on this week's first beta.
Attachment #8534967 -
Flags: approval-mozilla-beta?
Attachment #8534967 -
Flags: approval-mozilla-beta+
Attachment #8534967 -
Flags: approval-mozilla-aurora?
Attachment #8534967 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0891d8dfe594
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/fbed62f86166
Reporter | ||
Updated•9 years ago
|
Comment 11•9 years ago
|
||
Reproduced with Nightly 2014-12-10 on Windows 7 64-bit. Verified as fixed with Aurora 36.0a2 (Build ID: 20141218004002), Nightly 37.0a1 (Build ID: 20141217030202) and Firefox 35 beta 4 (Build ID: 20141216120925) on Ubuntu 14.04 32-bit, Windows 7 64-bit and Mac OS X 10.9.5. During some additional exploratory testing, encountered that during a call between FxA Accounts, the Loop button isn't turning blue. Tried with Fx 35 beta 4 and 35 beta 3 on 2 different Windows machines and Mac OS X, reproducible with both builds. Any ideas?
Status: RESOLVED → VERIFIED
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #11) > During some additional exploratory testing, encountered that during a call > between FxA Accounts, the Loop button isn't turning blue. Tried with Fx 35 > beta 4 and 35 beta 3 on 2 different Windows machines and Mac OS X, > reproducible with both builds. Any ideas? I'm not sure if that's supposed to happen, but can you file a bug about this? This way we can discuss, prioritize and perhaps land a fix.
Flags: needinfo?(mdeboer) → needinfo?(alexandra.lucinet)
Comment 13•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #12) > (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #11) > > During some additional exploratory testing, encountered that during a call > > between FxA Accounts, the Loop button isn't turning blue. Tried with Fx 35 > > beta 4 and 35 beta 3 on 2 different Windows machines and Mac OS X, > > reproducible with both builds. Any ideas? > > I'm not sure if that's supposed to happen, but can you file a bug about > this? This way we can discuss, prioritize and perhaps land a fix. Thanks for your input! Logged bug 1113673.
Flags: needinfo?(alexandra.lucinet)
You need to log in
before you can comment on or make changes to this bug.
Description
•