Closed Bug 1109511 Opened 5 years ago Closed 5 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, minor)

defect
Not set
minor
Points:
1

Tracking

(firefox35 verified, firefox36 verified, firefox37 verified)

VERIFIED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: jesup, Assigned: mikedeboer)

References

Details

Attachments

(1 file)

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.
Blocks: 1092954
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
OS: Linux → All
Hardware: x86_64 → All
Flags: needinfo?(mmucci)
Added to IT 37.2
Flags: needinfo?(mmucci)
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+
(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.
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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 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+
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)
(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)
(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.