Closed Bug 843163 Opened 12 years ago Closed 12 years ago

[SMS] Split 'sms.js' in some files for getting a better architecture

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:tef+, b2g18 verified, b2g18-v1.0.1 verified)

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
b2g18 --- verified
b2g18-v1.0.1 --- verified

People

(Reporter: borjasalguero, Assigned: borjasalguero)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch rebased
224 bytes, text/html
julienw
: review+
lsblakk
: approval-gaia-v1+
Details
Having all the code in one file makes the code hard to read and maintain.
Assignee: nobody → fbsc
3 files will be enough ;) The best would be to have MessageManager expose observer methods so that it doesn't call ThreadUI and ThreadListUI directly. This should be quite easy and wouold make MessageManager quite easy to test. Obviously ThreadUI and ThreadListUI are a lot interdependant now but I think we can try to find the places where each object call the other and try to reduce these cases a lot.
Im creating the structure now, I will come with a proposal asap! ;)
Attached file Pull Request (obsolete) —
Attachment #717008 - Flags: review?(felash)
Comment on attachment 717008 [details] Pull Request feedback+ of course. some other small changes needed and I think we'll be good. Just need to take care that you have all the changes that landed, maybe use a diff tool before landing to check that.
Attachment #717008 - Flags: review?(felash) → feedback+
Blocks: 844770
I've added your suggestions to the bug. Adding you as a reviewer!
Attachment #717008 - Flags: review?(felash)
Attached file Patch rebased —
Attachment #717008 - Attachment is obsolete: true
Attachment #717008 - Flags: review?(felash)
Attachment #718347 - Flags: review?(felash)
Comment on attachment 718347 [details] Patch rebased r=me we'll handle the discussion points in future bugs.
Attachment #718347 - Flags: review?(felash) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 718347 [details] Patch rebased NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): We have a not clear JS files structure. User impact if declined: Dev-Team needs to have a clear JS structure for changes coming and for debug current code. This change it's needed as well for adding performance stuff defined by bug 844770 Testing completed: Risk to taking this patch (and alternatives if risky): Low. We have move the code to several JS files only. String or UUID changes made by this patch:
Attachment #718347 - Flags: approval-gaia-v1?
Blocks: 845278
Do we have a way of knowing if any apps or calls to sms.js will be aware of this change and call the new file locations?
This change should not affect to other apps. Entry point it's the same (startup.js). It's only splitting the same code into several files. That's all! ;)
Lukas> If that's what worries you, we kept the same JS object names, the same activities, the same message handlers.
Comment on attachment 718347 [details] Patch rebased Thanks for clarifying, go ahead with uplift to v1-train.
Attachment #718347 - Flags: approval-gaia-v1? → approval-gaia-v1+
I was not able to uplift this bug to v1-train. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1-train git cherry-pick -x -m1 74cee2b2ca39fe9ba96c17f36664b4b8296cbaa8 <RESOLVE MERGE CONFLICTS> git commit
Hi John, I've checked that the problem comes from this one https://bugzilla.mozilla.org/show_bug.cgi?id=841353, and I dont know if it should be uplifted to v1-train because I was not the reviewer and that patch makes a lot of changes in several apps. On the other hand the conflict it's only in 'index.html', so it should be easy to fix.
it seems Bug 841353 should be uplifted but it has a conflict as well. I'll take a look today.
A chain of conflicts leads to Bug 837267, where I asked for approval.
John, the cherry-pick works after the uplift for Bug 842315 is done.
Required for uplifting SMS performance improvement (bug 844770)
blocking-b2g: --- → tef+
v1-train: 50f3262 Because bug 842315 is not yet on v1.0.1, this bug is waiting on that bug to be resolved before it can be landed on v1.0.1
status-b2g18: affected → fixed
The uplift of this broke v1-train, please don't uplift to v1.0.1 until I sort this out.
Seems like Bug 845278 is the culprit here :)
ok, this is fine to uplift along with Bug 845278. Should we wait for Bug 845278 to get tef+ or should we go ahead assuming it is automatic ?
Borja, could you help uplifting this to v1.0.1 please ? This is needed for Bug 844770 and I don't want to screw the app.
Flags: needinfo?(fbsc)
Borja, have you tried doing this uplift ?
Working on it now. I will push the commit asap.
Flags: needinfo?(fbsc)
I'm actually wondering if this is safe to do that now. I believe we're quite happy with the current performance for v1.0.1... My advice would be to keep Sms just as it is now in v1.0.1, and requalify all perf tef+ not yet landed to leo+. What do you think ?
Flags: needinfo?(mvines)
After checking I've realized that was uplifted to v1-train as well. Fixed.
status-b2g18-v1.0.1: affected → fixed
Has this really been fixed on v1.0.1? What's the commit?
Flags: needinfo?(mvines)
Can you please provide steps to verify this fix - as we will blackbox test from the UI? Thanks.
Hi. This fix has no impact in UI. For testing the whole patch, you should apply as well bugs 844770 & 845278 and you will see that the loading time is better than before ;)
The only thing to verify here is that the Sms app still works correctly.
(In reply to Julien Wajsberg [:julienw] from comment #34) > The only thing to verify here is that the Sms app still works correctly. Verified fixed the following: sms "works correctly" as far as the ability to send and receive text messages. Verified on the following Unagi v1 and v101 builds: Unagi Build ID: 20130327070202 Kernel Date: Dec 5 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/879324d41588 Gaia: e7e7c1a473b4069544f2dad779b1c9b1bab91663 Unagi Build ID: 20130327153857 Kernel Date: Dec 5 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4931ec89ebbe Gaia: d40dcdd112f12e2a5a0d1de46451670918fd4369
Status: RESOLVED → VERIFIED
status-b2g18: fixed → verified
status-b2g18-v1.0.1: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: