[Messages][Refactoring] Extract SystemMessageHandler from ActivityHandler

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: azasypkin, Assigned: azasypkin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

3 years ago
Will take it, hope it will make solution for bug 1192263 easier.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Whiteboard: [sms-sprint-FxOS-S8]
(Assignee)

Comment 2

3 years ago
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.
Created attachment 8667921 [details] [review]
[gaia] azasypkin:bug-1208532-system-message-handler > mozilla-b2g:master
(Assignee)

Comment 4

3 years ago
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+
(Assignee)

Comment 6

3 years ago
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+
(Assignee)

Comment 8

3 years ago
(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 :)
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.