Bug 843780 - Fix SMS mockup for desktop testing #8246
Conversation
| sent.message.delivery = 'sent'; | ||
| MessageManager.onMessageSent(sent); | ||
|
|
||
| if (typeof callback==="function") { |
There was a problem hiding this comment.
nit: typeof callback==="function" => typeof callback === "function"
There was a problem hiding this comment.
Absolutely. I've re-written the commit to address this.
|
@jugglinmike I will review this during the morning. Meanwhile could you take a look to #8262 ? I would like to know your opinion as well. Thanks! Gracias! |
| }; | ||
| MessageManager.handleEvent.call(MessageManager, evt); | ||
| // the SMS DB is written after the callback |
There was a problem hiding this comment.
We should remove this comment due to there is no DB anymore.
|
@jugglinmike The code looks nice! I've left small comments. Meanwhile Im gonna test that everything works as expected. |
| }, 3000 * Math.random()); | ||
|
|
||
| // Wait between [1000, 2000] milliseconds to simulate network latency. | ||
| }, 1000 + 1000 * Math.random()); | ||
|
|
||
| if (simulateFail) |
There was a problem hiding this comment.
Could we get a simulateFail with a 1/2 of probability? For having all UI states with a well know pattern. Wdyt?
There was a problem hiding this comment.
Currently, messages fail based on their text content--specifically, if it contains the string "fail":
var simulateFail = /fail/i.test(text);This was how the mock behaved prior to this patch, and I feel it is preferable to random failures because it allows developers to control the outcome of the operation.
|
@jugglinmike I like to see all these mockups restored! Your code works as expected. Take a look to the comments and please change the commit name adding the reviewer. In this case would be something like "Bug 843780 - Fix SMS mockup for desktop testing r=borjasalguero" . Let me know when you have the code ready and I will test it again. Thanks a lot! |
|
@jugglinmike Once you have these small changes please squash your changes and change the commit as I told you before ok? The patch looks good so we could have it landed asap! |
| receiver: number, | ||
| delivery: 'sending', | ||
| body: text, | ||
| id: messagesHack.length, |
There was a problem hiding this comment.
I will also be changing this to use a global message counter--using the array length introduces race conditions.
This script was created to facilitate development on desktops using with FireFox. It has broken since being implemented. This commit restores the intended functionality and prevents race conditions in message creation.
|
Okay, amended the commit. How does it look now, @borjasalguero ? |
|
@jugglinmike Hi again! The code looks nice, it's ready for merging. However we have to make a small step before! Please go to the bug https://bugzilla.mozilla.org/show_bug.cgi?id=843780 and in the 'details' section of your attachment please add me as a 'reviewer'. |
|
Alrighty--all set! |
|
@jugglinmike r+! Thanks for restoring SMS mockup. |
|
No problem. I don't have write access to the repository, so I think you'll have to merge it on my behalf. |
|
@jugglinmike No prob. Merging...! |
Bug 843780 - Fix SMS mockup for desktop testing
|
Ah, great--thanks! |
Bug 843780 - Fix SMS mockup for desktop testing(cherry picked from commit 48e5aa9)
This script was created to facilitate development on desktops using with
FireFox. It has broken since being implemented. This commit restores the
intended functionality.