869219 - [SMS] Resend handler is bound unsafely

Status: RESOLVED FIXED

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

Description

[Mike Pennisi [:jugglinmike]]

Currently, a unique "resend" handler is bound to each message element individually on an as-needed basis. This approach is prone to runtime bugs such as multiple handlers being bound at once, or handlers not being removed when the message state changes.

To avoid bugs like these (and to make the code more performant/maintainable), re-factor the "resend" handler to be bound to the Thread's container element and delegate to each message.

Comment 1

[Mike Pennisi [:jugglinmike]]

Making a note here that while the "resend" handler is currently bound to the top-level message element, this is not desirable. The message element spans the entire width of the application, which can be surprising for a user who is attempting to scroll through the thread.

The patch for this bug should instead bind the "resend" handler to the message "bubble" element whose boundaries are well-defined in the UI.

Comment 2

[Mike Pennisi [:jugglinmike]]

Attachment: Use event delegation for message resend handler

Comment 3

[Corey Frang [:gnarf]]

Comment on attachment 750085 [details] [diff] [review]
Use event delegation for message resend handler

Review of attachment 750085 [details] [diff] [review]:
-----------------------------------------------------------------

quick stuff here I think

message_manager_tst.js should be message_manager_test.js

::: apps/sms/js/thread_ui.js
@@ +123,5 @@
>      );
>  
>      // Delegate to |this.handleEvent|
>      this.container.addEventListener(
> +      'click', this.handleEvent.bind(this)

Let's bind the rest of these as well

@@ +981,5 @@
>    handleEvent: function thui_handleEvent(evt) {
>      switch (evt.type) {
>        case 'click':
>          if (window.location.hash !== '#edit') {
> +          this.handleMessageClick(evt);

There are a few refs to ThreadUI below, lets convert them to this

@@ +1183,5 @@
> +        removeFromDOM();
> +        // We resend again
> +        this.sendMessage(message.body);
> +      }.bind(this));
> +    }.bind(this);

Lint complains about this, which is a shame, but if you put () around the function it works fine.

Comment 4

[Mike Pennisi [:jugglinmike]]

Attachment: Use event delegation for message resend handler

This version implements Corey's feedback in comment 3. Namely:

- Fix a linting error
- More consistent binding of event handlers

Comment 5

[Corey Frang [:gnarf]]

master:9cce95ca788f653e8915c71d41c81f831edcd62c

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 5c95d71

Comment 7

[Deepa Subramanian]

Can you please provide steps to verify this fix - as we can perform blackbox testing from the UI?

Comment 8

[Corey Frang [:gnarf]]

The fact that you can still resend is all that should be tested here.  The bug was basically improving the method to bind the event