Closed Bug 1208532 Opened 4 years ago Closed 4 years ago

[Messages][Refactoring] Extract SystemMessageHandler from ActivityHandler

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: azasypkin, Assigned: azasypkin)

References

Details

(Whiteboard: [sms-sprint-FxOS-S8])

Attachments

(1 file)

In this we'd like to split ActivityHandler into ActivityHandler that will handle activity requests only and SystemMessageHandler that will handle "sms-received" and "notification" system messages.

Code that handles activity requests is quite different from one that handles other system messages, so it makes sense to split them into separate modules.
Will take it, hope it will make solution for bug 1192263 easier.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Whiteboard: [sms-sprint-FxOS-S8]
Working on the patch, discovered funny issue: if you use any l10n-id as the name of contact and receive the message from this contact - you'll see localized string as the sender name :D. E.g. name your contact as "discard-new-message" and enjoy receiving new message notifications from "Discard the message you’re composing and open the new message?" sender :)

It seems like an edgy case, but there is still chance to collide with shorter l10n ids.

I'll fix that.
Comment on attachment 8667921 [details] [review]
[gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master

Hey Steve,

Here is WIP PR for the separated SystemMessageHandler, it doesn't contain any haven't fixed unit tests yet) and it works only in non-split mode (though I haven't tested it heavily, almost solely relying on existing integration tests).

Would be glad to have your early feedback!

Thanks!
Attachment #8667921 - Flags: feedback?(schung)
Comment on attachment 8667921 [details] [review]
[gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master

It looks good and we want to refactor the message receiving part for a long time. It good to see we split the logic and split the huge message receiving logic into smaller pieces. I just have a small question: Would you plan to move the system message handling into service/shim once the system message is workable in shim?
Attachment #8667921 - Flags: feedback?(schung) → feedback+
Comment on attachment 8667921 [details] [review]
[gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master

Hey Steve,

I think it's now ready for the first round of review :)

(In reply to Steve Chung [:steveck] from comment #5)
> Comment on attachment 8667921 [details] [review]
> [gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master
> 
> It looks good and we want to refactor the message receiving part for a long
> time. It good to see we split the logic and split the huge message receiving
> logic into smaller pieces.

Thanks for the feedback!

> I just have a small question: Would you plan to move
> the system message handling into service/shim once the system message
> is workable in shim?

Yep, I think so. In this PR I used the chance to clear this code up a bit, should not be difficult to move it to service/shim later if we decide so.
Attachment #8667921 - Flags: review?(schung)
Comment on attachment 8667921 [details] [review]
[gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master

Okey I can't find critical issue in the patch. Only some trivial question and nits. Thanks for the splitting!
Attachment #8667921 - Flags: review?(schung) → review+
(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8667921 [details] [review]
> [gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master
> 
> Okey I can't find critical issue in the patch. Only some trivial question
> and nits. Thanks for the splitting!

Thanks for review!

Nits are fixed and questions are answered - please tell me if it's what you've asked for :)
Duplicate of this bug: 1153784
Treeherder is green (only one unrelated failure), landed!

Master: https://github.com/mozilla-b2g/gaia/commit/a9ee136440a7d57c3d47ef1587e72be8e9136074
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.