Closed
Bug 1184865
Opened 8 years ago
Closed 8 years ago
[Messages][NG] Replace some methods in MessageManager with messaging service in conversation view
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.2 S5 (6feb)
People
(Reporter: steveck, Assigned: steveck)
References
Details
(Whiteboard: [p(FxOS-S5)=1][p(FxOS-S4)=2][sms-sprint-FxOS-S5])
Attachments
(1 file)
Since message service and message API shim is ready, we can do some experiment to figure out how to make it work in current code. We might need to introduce some utils method for iframe creation and connection setup between service/shim context.
Assignee | ||
Comment 1•8 years ago
|
||
WIP: https://github.com/steveck-chung/gaia/tree/new-message-messaging-service First step is to use BridgeServiceMixin in MozMobileMessageShim
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master Hi, It's still a WIP about migrate retrieveMMS to messaging service/shim(because settings service/shim are not ready yet and message sending need it). The concept of the shim and iframe was aligned with Oleg's in conversation service patch. MMS retrieval could work under this architecture, but with some unneglectable flaws: - retrieveMMS mozMobileMessage API was executed and retrieve mms correctly, but the bridge will return reject with no response error. - It only works for the first time. when I tried to retrieve another mms, message service method was no executed. I'll spend more time to dig out the problem, and it would be great if you could find out any problem or bug in the patch. ni? Wilson since he might know the root cause of these issues. Hi Wilson, do you have any idea that might related to these issues? The call path from view to API would be: messaging-client(window context) -> messageing-service(sharedworker context) -> message-shim(iframe context) And inside the message-shim, we'll return the result of retrieveMMS API since it should be the then-able DOMrequest.
Flags: needinfo?(wilsonpage)
Attachment #8638545 -
Flags: feedback?(felash)
Attachment #8638545 -
Flags: feedback?(azasypkin)
Comment 4•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master Looks good to me! Left some comments at GitHub. I've tried to reproduce "no-response" in Fx Desktop, I noticed it once, enabled debugging in bridge, and could not able to reproduce after that, I'll try later once again, if you don't figure out the reason earlier :) Thanks!
Attachment #8638545 -
Flags: feedback?(azasypkin) → feedback+
Comment 5•8 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #3) > Hi Wilson, do you have any idea that might related to these issues? The call > path from view to API would be: Are you using BroadcastChannel for the connection you're having issues with? If so I have recently landed a patch that might improve the connection handshake in some corner-cases. Try running with the latest threads.js master build and let me know if this improves things. If not can you give me a reduced test-case or some STRs with 'expected' and 'actual' outcomes, cheese :)
Flags: needinfo?(wilsonpage)
Comment 6•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master left some feedback but overall this looks good
Attachment #8638545 -
Flags: feedback?(felash) → feedback+
Updated•8 years ago
|
Blocks: sms-sprint-FxOS-S4
Whiteboard: [p=2]
Assignee | ||
Comment 7•8 years ago
|
||
Updated with the latest threads, but the 2 main issues mentioned above still existed :/ Sorry I didn't have the reduced test-case yet, but the STRs would be: 1) Enter the message settings panel from settings or message app, and turn off the MMS auto retrieve 2) Send the MMS from other devices to target device, and you'll receive a message with download button. Click download. Expected: Original message removed and mms message downloaded later Actual: Original message will turn into the error status briefly since it will return no response error first, but the retrieval is success actually and it'll be replaced new message later. And after this action completed, if you repeat the same steps again, the service will not work with this error: I/Gecko ( 208): IPDL protocol error: Handler for PostMessages returned error code I/Gecko ( 208): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x880001,name=PMessagePort::Msg_PostMessages) Processing error: message was deserialized, but the handler returned false (indicating failure) Please let me know if there is any other way that could provide more debugging information for you, thanks!
Flags: needinfo?(wilsonpage)
Comment 8•8 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #7) > Sorry I didn't have the reduced test-case yet, but the STRs would be: > 1) Enter the message settings panel from settings or message app, and turn > off the MMS auto retrieve > 2) Send the MMS from other devices to target device, and you'll receive a > message with download button. Click download. Is there any way to mock this without actually having to send real MMS?
Flags: needinfo?(wilsonpage)
Updated•8 years ago
|
Flags: needinfo?(schung)
Comment 9•8 years ago
|
||
Hey Wilson, I'm not 100% sure, but you can try to reproduce this in Fx Desktop: 1. Checkout PR and run "DEBUG=1 make"; 2. Open this profile in Nightly (/path_to_nightly/firefox -profile /path_to_gaia/profile-debug/); 3. Navigate to "app://sms.gaiamobile.org"; 4. Enter conversation from "Tom O'Hare" and find message with "Donwload" link; 5. When you tap on that link we retrieve MMS using mozMobileMessage mock, but "bridge" machinery is the same as on device. So you can try several times to go back to Inbox, then enter the "Tom O'Hare" conversation and tap on Download link again - I noticed "no response" after few tries, but didn't have time to confirm that it's the same issue Steve is suffering from.
Assignee | ||
Comment 10•8 years ago
|
||
I updated the patch to make the desktop mock runnable, and I found one issue is we'll need to make a clone for the returning message from API, otherwise service side could not be resolved successfully. Wondering why there is no dataClone error for this... Anyway if return the cloned message from shim, the no response error will be resolved. Still trying to figure out why the second service call could not work.
Flags: needinfo?(schung)
Assignee | ||
Comment 11•8 years ago
|
||
Ok I think the latest commit addressed/workaround the dataclone error and sharedworker reference problem. One last question is about the no response timeout, since mms retrieval will need more time than default 1 sec timeout, is there an alternative way to change the timeout duration for some specific case?
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master Hey, Patch with the updated unit test is ready(hopefully), There's also update for bridge lib included for some changes for timeout feature and endpoint changes(that we can provide the sharedworker directly as endpoint). Please ping me if it didn't work on your device/desktop or seeing any performance regression, thanks!
Attachment #8638545 -
Flags: review?(felash)
Attachment #8638545 -
Flags: review?(azasypkin)
Assignee | ||
Comment 13•8 years ago
|
||
Clear ni? since Wilson already addressed the timeout issue in the latest master.
Flags: needinfo?(wilsonpage)
Comment 14•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master I left some comments on github. This will need rebasing after bug 1162030 but the rebase is not very difficult (I managed to do it :) ). I tested on the phone a MMS retrieval and it works fine, except we have the timeout.
Attachment #8638545 -
Flags: review?(felash)
Comment 15•8 years ago
|
||
I realize I also got this message once: 08-10 18:49:32.677 6817 6817 E Messages: [JavaScript Error: "DataCloneError: The object could not be cloned." {file: "app://sms.gaiamobile.org/lib/bridge/bridge.js" line: 17}] but I don't know when it happened exactly... :/
Comment 16•8 years ago
|
||
Depends on https://github.com/gaia-components/threads/issues/73 (which is p=1 for this sprint as well)
Blocks: sms-sprint-FxOS-S5
Whiteboard: [p=2] → [p(FxOS-S5)=1][p(FxOS-S4)=2]
Comment 17•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master Please add r?=me once again when Bridge related PR is landed :) Thanks!
Attachment #8638545 -
Flags: review?(azasypkin)
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master Hi, patch updated with timeout disabled feature(still in review). There should be no timeout for "no response" error but you can give it a try on your side.
Attachment #8638545 -
Flags: review?(felash)
Attachment #8638545 -
Flags: review?(azasypkin)
Comment 19•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master r=me I left some comments thought, so please fix them before landing: * you forgot the desktop shim mock in your patch * add the the iframe to views/inbox * some style nits Also I agree with Oleg's comments :) Of course please wait for a green try before merging :)
Attachment #8638545 -
Flags: review?(felash) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8638545 [details] [review] [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master Looks good! Just left several nits at GitHub. Thanks!
Attachment #8638545 -
Flags: review?(azasypkin) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Updated with the latest bridge library and the tree is green. Thanks for all the review! In master: https://github.com/mozilla-b2g/gaia/commit/7afe8bd5d34b70a053620a2480df05c8f827bcec
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [p(FxOS-S5)=1][p(FxOS-S4)=2] → [p(FxOS-S5)=1][p(FxOS-S4)=2][sms-sprint-FxOS-S5]
Target Milestone: --- → 2.2 S5 (6feb)
You need to log in
before you can comment on or make changes to this bug.
Description
•