Closed Bug 1112959 Opened 10 years ago Closed 10 years ago

[FFOS2.0][Woodduck][SMS]The message list isn't refresh after save a draft in other module.

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Type:
defect

Tracking

(blocking-b2g:2.0M+, b2g-v2.0M affected)

Status:
RESOLVED DUPLICATE of bug 1058459
Project Flags:
blocking-b2g 2.0M+
Tracking Flags:
Tracking Status
b2g-v2.0M --- affected

People

(Reporter: sync-1, Unassigned)

References

Details

Attachments

(3 files)

patch.diff
28.62 KB, patch
Details | Diff | Splinter Review
1112959_20141226
7.81 KB, application/octet-stream
Details
1112959_20141226
7.81 KB, application/octet-stream
Details 
DEFECT DESCRIPTION:
   The message list isn't refresh after save a draft in other module.
 
 REPRODUCING PROCEDURES:
 (Enter the message list first.)
 
 1.Enter camera/Music/Gallery/Video/File manager/Browser->select a file/bookmark->share via Messages,before switch to new message editing screen,MS displays message list screen several seconds--KO1
 
 2.After MS go to new message editing screen->click "X" on the left top of the editing screen->save as draft,back to idle
 
 3.Go to message list->check the saved draft,no draft displays--KO2
 
 PS: Press home key --> clear the SMS app --> Enter SMS app again, will display the saved draft.
 
  EXPECTED BEHAVIOUR:
 KO1:should not display this screen
 KO2:should refresh the message list and display the saved draft.
KO1 is known and we're currently alleviating it (without fixing the root cause in bug 1013738 because it's a lot of work) in bug 1109745.

I think KO2 is fixed in bug 1058459 (only in v2.2 then).
Should we dupe to bug 1058459?
(In reply to comment #1)
 > Comment from Mozilla:KO1 is known and we're currently alleviating it (without
 > fixing the root cause in bug 1013738 because it's a lot of work) in bug
 > 1109745.
 > I think KO2 is fixed in bug 1058459 (only in v2.2 then).
 
 Could you provide the patch of KO2 to us? I want to do a test.
You can get the patch here: https://github.com/mozilla-b2g/gaia/commit/33a481e5418a57ab8eef57dd21902b102d2f649c.patch

It's quite a big patch, with low-medium risk (because it's mostly new files and it's only modifying existing files in a small amount).
(In reply to comment #4)
 > Comment from Mozilla:You can get the patch here:
 > https://github.com/mozilla-b2g/gaia/commit/33a481e5418a57ab8eef57dd21902b102d2f649c.patch
 > It's quite a big patch, with low-medium risk (because it's mostly new files and
 > it's only modifying existing files in a small amount).
 
 Will you landed this patch on 2.0?
(In reply to sync-1 from comment #5)
> (In reply to comment #4)
>  > Comment from Mozilla:You can get the patch here:
>  >
> https://github.com/mozilla-b2g/gaia/commit/
> 33a481e5418a57ab8eef57dd21902b102d2f649c.patch
>  > It's quite a big patch, with low-medium risk (because it's mostly new
> files and
>  > it's only modifying existing files in a small amount).
>  
>  Will you landed this patch on 2.0?

Please let me know if you need help to rebase this patch on v2.0m (I haven't checked if it can be easily applied). 

If we decide to land it on v2.0m we'll need your help to thoroughly test entire draft functionality on v2.0m with this patch because there is still a low-medium risk to break something.
(In reply to comment #6)
 > Comment from Mozilla:(In reply to sync-1 from comment #5)
 > > (In reply to comment #4)
 > >  > Comment from Mozilla:You can get the patch here:
 > >  >
 > > https://github.com/mozilla-b2g/gaia/commit/
 > > 33a481e5418a57ab8eef57dd21902b102d2f649c.patch
 > >  > It's quite a big patch, with low-medium risk (because it's mostly new
 > > files and
 > >  > it's only modifying existing files in a small amount).
 > >  
 > >  Will you landed this patch on 2.0?
 > Please let me know if you need help to rebase this patch on v2.0m (I haven't
 > checked if it can be easily applied). 
 
 Could you send the chaned files to me.
(In reply to sync-1 from comment #7)
> (In reply to comment #6)
>  > Comment from Mozilla:(In reply to sync-1 from comment #5)
>  > > (In reply to comment #4)
>  > >  > Comment from Mozilla:You can get the patch here:
>  > >  >
>  > > https://github.com/mozilla-b2g/gaia/commit/
>  > > 33a481e5418a57ab8eef57dd21902b102d2f649c.patch
>  > >  > It's quite a big patch, with low-medium risk (because it's mostly new
>  > > files and
>  > >  > it's only modifying existing files in a small amount).
>  > >  
>  > >  Will you landed this patch on 2.0?
>  > Please let me know if you need help to rebase this patch on v2.0m (I
> haven't
>  > checked if it can be easily applied). 
>  
>  Could you send the chaned files to me.

I've just rebased master patch on v2.0m (fix for KO2), so I can provide you with the link: bug 1058459 attachment 8540126 [details] [review]
(In reply to comment #8)
 > Comment from Mozilla:(In reply to sync-1 from comment #7)
 > > (In reply to comment #6)
 > >  > Comment from Mozilla:(In reply to sync-1 from comment #5)
 > >  > > (In reply to comment #4)
 > >  > >  > Comment from Mozilla:You can get the patch here:
 > >  > >  >
 > >  > > https://github.com/mozilla-b2g/gaia/commit/
 > >  > > 33a481e5418a57ab8eef57dd21902b102d2f649c.patch
 > >  > >  > It's quite a big patch, with low-medium risk (because it's mostly new
 > >  > > files and
 > >  > >  > it's only modifying existing files in a small amount).
 > >  > >  
 > >  > >  Will you landed this patch on 2.0?
 > >  > Please let me know if you need help to rebase this patch on v2.0m (I
 > > haven't
 > >  > checked if it can be easily applied). 
 > >  
 > >  Could you send the chaned files to me.
 > I've just rebased master patch on v2.0m (fix for KO2), so I can provide you
 > with the link: bug 1058459 attachment 8540126 [details] [review]
 
 I use this patch and do a test. It can't add the file to the MMS.
 
 1. Enter Browser-->select a bookmark-->share via Messages-->save as draft-->go to message list check the draft-->OK
 
 2. Enter camera/Music/Gallery/Video/File manager-->select a file-->share via Messages-->KO (can't add the file into the MMS)
Blocks: Woodduck
Flags: needinfo?(azasypkin)
(In reply to Julien Wajsberg [:julienw] from comment #2)
> Should we dupe to bug 1058459?

Hi Julien,
Let's wait and see whether partner try patch result per comment 8 and 9. Thanks!
Per comment #9 point 2, it can't add the file into the MMS. 
 (It can add the file into the MMS without this patch.)
(In reply to sync-1 from comment #11)
> Per comment #9 point 2, it can't add the file into the MMS. 
>  (It can add the file into the MMS without this patch.)

Mmm, I can't reproduce with the steps from point #2, I use Flame, v2.0 pvt build + full gaia from v2.0m with the patch.

Could you please attach logs?
Flags: needinfo?(azasypkin) → needinfo?(sync-1)
(In reply to comment #12)
 > Comment from Mozilla:(In reply to sync-1 from comment #11)
 > > Per comment #9 point 2, it can't add the file into the MMS. 
 > >  (It can add the file into the MMS without this patch.)
 > Mmm, I can't reproduce with the steps from point #2, I use Flame, v2.0 pvt
 > build + full gaia from v2.0m with the patch.
 > Could you please attach logs?
 
 The logs and video on baidu pan:
 
     http://pan.baidu.com/s/1i3mVMG9
 
     5gyb
(NI Oleg so that he doesn't miss this when coming back from holidays)
Flags: needinfo?(azasypkin)
Don't see anything suspicions in logs and still can't reproduce, but video looks strange (attachment size indicator above MMS label, never seen something like this)

Hey Josh, I'm on holidays right now, can we get any help from Device Team to investigate the issue from comment 9?

Keeping ni=me for now.

Thanks!
Flags: needinfo?(jocheng)
Thank you very much for the help and wish you a great holiday, Oleg!

Hi Gary,
Could you help resume investigating work from Oleg with patch from comment 8?
Thank you!
Flags: needinfo?(sync-1)
Flags: needinfo?(jocheng)
Flags: needinfo?(gchen)
Attached patch patch.diff — — Splinter Review 
Hi Sync-1,

I can reproduce KO2 bug at description.
After merging Oleg's patch at comment 8, KO2 bug is resolved.

May I know how you guys merge the patch?

The step that how I merge the patch is for your reference:
git checkout -b azasypkin-v2.0m-bug-1058459-share-drafts v2.0m
git pull https://github.com/azasypkin/gaia.git v2.0m-bug-1058459-share-drafts

The attachment is the patch generated after merging Oleg's patch.
parent sha-1: 05e603027cc2ce0508a7c58efe45bd7f2c65a076

Thank you
Flags: needinfo?(gchen) → needinfo?(sync-1)
(In reply to comment #17)
 > Comment from Mozilla:Hi Sync-1,
 > I can reproduce KO2 bug at description.
 > After merging Oleg's patch at comment 8, KO2 bug is resolved.
 > May I know how you guys merge the patch?
 
 I just merge the files which not under the test directory. Is there any problems?
 
 > The step that how I merge the patch is for your reference:
 > git checkout -b azasypkin-v2.0m-bug-1058459-share-drafts v2.0m
 > git pull https://github.com/azasypkin/gaia.git v2.0m-bug-1058459-share-drafts
 > The attachment is the patch generated after merging Oleg's patch.
 > parent sha-1: 05e603027cc2ce0508a7c58efe45bd7f2c65a076
 > Thank you
 
 Thanks, I will try this patch again and check whether is our codebase problem.
Hi Reporter,
Thanks! I will close the bug for now and please reopen if you still observe problem in the future.
Status: NEW → RESOLVED
blocking-b2g: --- → 2.0M+
Closed: 10 years ago
status-b2g-v2.0M: --- → affected
Flags: needinfo?(sync-1)
Flags: needinfo?(azasypkin)
Resolution: --- → DUPLICATE
Dear Sean & Oleg,
 
 Per comment #9 point #2 is our codebase problem and we had remove the code.
 
 After that, I try the patch like attachment 8541417 [details] [diff] [review] and found it can't create new message. The changes of the patch in this two files "message_manager.js" and "startup.js" cause some code error which add by us for some customization.
 
 So, about this two files, the only changes is:
 
 -------------------------------------------------------------------
 diff --git a/apps/sms/js/startup.js b/apps/sms/js/startup.js
 index fe6023c..d7d846f 100644
 --- a/apps/sms/js/startup.js
 +++ b/apps/sms/js/startup.js
 @@ -5,7 +5,9 @@
  
  /*global Utils, ActivityHandler, ThreadUI, ThreadListUI, MessageManager,
           Settings, LazyLoader, TimeHeaders, Information, SilentSms,
 -         PerformanceTestingHelper, App, Navigation, EventDispatcher */
 +         PerformanceTestingHelper, App, Navigation, EventDispatcher,
 +         InterInstanceEventDispatcher
 +*/
  
  navigator.mozL10n.ready(function localized() {
    // This will be called during startup, and any time the languange is changed
 @@ -92,6 +94,8 @@ var Startup = {
  
    _lazyLoadInit: function() {
      LazyLoader.load(this._lazyLoadScripts, function() {
 +      InterInstanceEventDispatcher.connect();
 +
        // dispatch moz-content-interactive when all the modules initialized
        SilentSms.init();
        ActivityHandler.init();
 -------------------------------------------------------------------
 
 I had test it, and it works.
 
 Is there any problems? Could you review it. Thanks!
Flags: needinfo?(selee)
Flags: needinfo?(jocheng)
Hi Reporter,

Could you provide more specific description for this patch?

If the patch at comment 20 is not for this bug, I will suggest that the new case should be created as a new bug/feature with your purpose in the bug's description.
Its purpose could be fixing a bug or adding a new feature which depends on what kind of issue that you want to fix.

So that the reviewer will have enough information to give you the comments with correct direction.

Thank you.
Flags: needinfo?(selee)
Flags: needinfo?(jocheng)
Created an attachment (id=1090140)
 1112959_20141226
 
 (In reply to Sean Lee [:seanlee] from comment #21)
 > Hi Reporter,
 > 
 > Could you provide more specific description for this patch?
 > 
 > If the patch at comment 20 is not for this bug, I will suggest that the new
 > case should be created as a new bug/feature with your purpose in the bug's
 > description.
 > Its purpose could be fixing a bug or adding a new feature which depends on
 > what kind of issue that you want to fix.
 > 
 > So that the reviewer will have enough information to give you the comments
 > with correct direction.
 > 
 > Thank you.
 
 The patch is base on attachment 8541417 [details] [diff] [review], the difference is what I merged in this two files "message_manager.js", "startup.js".  I want make sure this difference will not cause other error.
 
 From the attachment 1112959_20141226 you can see our local code in the file "message_manager.js".
Created an attachment (id=1090140)
 1112959_20141226
 
 (In reply to Sean Lee [:seanlee] from comment #21)
 > Hi Reporter,
 > 
 > Could you provide more specific description for this patch?
 > 
 > If the patch at comment 20 is not for this bug, I will suggest that the new
 > case should be created as a new bug/feature with your purpose in the bug's
 > description.
 > Its purpose could be fixing a bug or adding a new feature which depends on
 > what kind of issue that you want to fix.
 > 
 > So that the reviewer will have enough information to give you the comments
 > with correct direction.
 > 
 > Thank you.
 
 The patch is base on attachment 8541417 [details] [diff] [review], the difference is what I merged in this two files "message_manager.js", "startup.js".  I want make sure this difference will not cause other error.
 
 From the attachment 1112959_20141226 you can see our local code in the file "message_manager.js".
Attached file 1112959_20141226 — 
Created an attachment (id=1090140)
 1112959_20141226
 
 (In reply to Sean Lee [:seanlee] from comment #21)
 > Hi Reporter,
 > 
 > Could you provide more specific description for this patch?
 > 
 > If the patch at comment 20 is not for this bug, I will suggest that the new
 > case should be created as a new bug/feature with your purpose in the bug's
 > description.
 > Its purpose could be fixing a bug or adding a new feature which depends on
 > what kind of issue that you want to fix.
 > 
 > So that the reviewer will have enough information to give you the comments
 > with correct direction.
 > 
 > Thank you.
 
 The patch is base on attachment 8541417 [details] [diff] [review], the difference is what I merged in this two files "message_manager.js", "startup.js".  I want make sure this difference will not cause other error.
 
 From the attachment 1112959_20141226 you can see our local code in the file "message_manager.js".
Created an attachment (id=1090140)
 1112959_20141226
 
 (In reply to Sean Lee [:seanlee] from comment #21)
 > Hi Reporter,
 > 
 > Could you provide more specific description for this patch?
 > 
 > If the patch at comment 20 is not for this bug, I will suggest that the new
 > case should be created as a new bug/feature with your purpose in the bug's
 > description.
 > Its purpose could be fixing a bug or adding a new feature which depends on
 > what kind of issue that you want to fix.
 > 
 > So that the reviewer will have enough information to give you the comments
 > with correct direction.
 > 
 > Thank you.
 
 The patch is base on attachment 8541417 [details] [diff] [review], the difference is what I merged in this two files "message_manager.js", "startup.js".  I want make sure this difference will not cause other error.
 
 From the attachment 1112959_20141226 you can see our local code in the file "message_manager.js".
Created an attachment (id=1090140)
 1112959_20141226
 
 (In reply to Sean Lee [:seanlee] from comment #21)
 > Hi Reporter,
 > 
 > Could you provide more specific description for this patch?
 > 
 > If the patch at comment 20 is not for this bug, I will suggest that the new
 > case should be created as a new bug/feature with your purpose in the bug's
 > description.
 > Its purpose could be fixing a bug or adding a new feature which depends on
 > what kind of issue that you want to fix.
 > 
 > So that the reviewer will have enough information to give you the comments
 > with correct direction.
 > 
 > Thank you.
 
 The patch is base on attachment 8541417 [details] [diff] [review], the difference is what I merged in this two files "message_manager.js", "startup.js".  I want make sure this difference will not cause other error.
 
 From the attachment 1112959_20141226 you can see our local code in the file "message_manager.js".
Attached file 1112959_20141226 — 
Created an attachment (id=1090140)
 1112959_20141226
 
 (In reply to Sean Lee [:seanlee] from comment #21)
 > Hi Reporter,
 > 
 > Could you provide more specific description for this patch?
 > 
 > If the patch at comment 20 is not for this bug, I will suggest that the new
 > case should be created as a new bug/feature with your purpose in the bug's
 > description.
 > Its purpose could be fixing a bug or adding a new feature which depends on
 > what kind of issue that you want to fix.
 > 
 > So that the reviewer will have enough information to give you the comments
 > with correct direction.
 > 
 > Thank you.
 
 The patch is base on attachment 8541417 [details] [diff] [review], the difference is what I merged in this two files "message_manager.js", "startup.js".  I want make sure this difference will not cause other error.
 
 From the attachment 1112959_20141226 you can see our local code in the file "message_manager.js".
> The changes of the patch in this two files "message_manager.js" and "startup.js" 
> cause some code error which add by us for some customization.

Can we know your changes? Then maybe we could help you.
(In reply to comment #23)
 > Comment from Mozilla:> The changes of the patch in this two files
 > "message_manager.js" and "startup.js" 
 > > cause some code error which add by us for some customization.
 > Can we know your changes? Then maybe we could help you.
 
 Our local code in the file "message_manager.js" line 42~60, line 335~374.
 
 Only use your patch, the "req" is undefined ("message_manager.js" line 360)
(In reply to comment #24)
 > (In reply to comment #23)
 > > Comment from Mozilla:> The changes of the patch in this two files
 > > "message_manager.js" and "startup.js" 
 > > > cause some code error which add by us for some customization.
 > > Can we know your changes? Then maybe we could help you.
 > Our local code in the file "message_manager.js" line 42~60, line 335~374.
 > Only use your patch, the "req" is undefined ("message_manager.js" line 360)
 
 Attachment 1112959_20141226 was provided the file "message_manager.js" which had our local code. thanks!
My only guess is that some conflicts were resolved incorrectly, can you please provide us with the "message_manager.js" that you got after you applied patch and resolved conflicts?
Flags: needinfo?(sync-1)
(In reply to comment #31)
 > Comment from Mozilla:My only guess is that some conflicts were resolved
 > incorrectly, can you please provide us with the "message_manager.js" that you
 > got after you applied patch and resolved conflicts?
 
 The finally patch what I applied at comment #27 attachment 8541664 [details] ("1112959-patch.diff").
 
 Only use your patch, the "req" is undefined ("message_manager.js" line 360).
 In order to resolved conflicts, I didn't applied any code into "message_manager.js" from your patch. And only applied a part of code of the file "startup.js"(comment #20) from you patch.
(In reply to sync-1 from comment #32)
> (In reply to comment #31)
>  > Comment from Mozilla:My only guess is that some conflicts were resolved
>  > incorrectly, can you please provide us with the "message_manager.js" that
> you
>  > got after you applied patch and resolved conflicts?
>  
>  The finally patch what I applied at comment #27 attachment 8541664 [details]
> [details] ("1112959-patch.diff").
>  
>  Only use your patch, the "req" is undefined ("message_manager.js" line 360).
>  In order to resolved conflicts, I didn't applied any code into
> "message_manager.js" from your patch. And only applied a part of code of the
> file "startup.js"(comment #20) from you patch.

Sorry just to confirm that I understood you correctly:

* Once you applied full patch from comment 8, you got conflicts (in message_manager.js) that lead to some errors inside your custom code;

* To avoid these conflicts you applied patch partially (everything except for the message_manager.js changes, few related startup.js changes and unit tests) and everything worked correctly, issue with drafts was fixed.

Is that correct?

If so I can confirm that your partial patch from attachment 8541664 [details] should be enough to fix the issue.

My only concern is that your custom branch won't have appropriate unit tests and it will be more difficult to apply v2.0m patches to you branch in the future since they diverge,  so if it's fine for you I'm ok with that too :)
(In reply to comment #28)
 > Comment from Mozilla:(In reply to sync-1 from comment #32)
 > > (In reply to comment #31)
 > >  > Comment from Mozilla:My only guess is that some conflicts were resolved
 > >  > incorrectly, can you please provide us with the "message_manager.js" that
 > > you
 > >  > got after you applied patch and resolved conflicts?
 > >  
 > >  The finally patch what I applied at comment #27 attachment 8541664 [details]
 > > [details] ("1112959-patch.diff").
 > >  
 > >  Only use your patch, the "req" is undefined ("message_manager.js" line 360).
 > >  In order to resolved conflicts, I didn't applied any code into
 > > "message_manager.js" from your patch. And only applied a part of code of the
 > > file "startup.js"(comment #20) from you patch.
 > Sorry just to confirm that I understood you correctly:
 > * Once you applied full patch from comment 8, you got conflicts (in
 > message_manager.js) that lead to some errors inside your custom code;
 > * To avoid these conflicts you applied patch partially (everything except for
 > the message_manager.js changes, few related startup.js changes and unit tests)
 > and everything worked correctly, issue with drafts was fixed.
 > Is that correct?
 
 Correct.
 
 > If so I can confirm that your partial patch from attachment 8541664 [details] should be
 > enough to fix the issue.
 > My only concern is that your custom branch won't have appropriate unit tests
 > and it will be more difficult to apply v2.0m patches to you branch in the
 > future since they diverge,  so if it's fine for you I'm ok with that too :)
 
 This is not what we expected. We will try to modify our local code. Could you give me some prompt about the reason of "undefined"(comment #29)? Thanks!
(In reply to sync-1 from comment #34)
>  
>  This is not what we expected. We will try to modify our local code. Could
> you give me some prompt about the reason of "undefined"(comment #29)? Thanks!

Sorry, the only idea I have is the one from comment 31 (conflict of removed "callback" with your custom code that wasn't resolved correctly)
If we can share with us the complete source code for SMS (in ZIP form), this could help.

If you prefer to send to Oleg or I using private email instead of this bug, or possibly in a separare restricted bug (because this one is resolved already), please do.

Thanks!
Discussed possible solutions via email, removing ni?
Flags: needinfo?(sync-1)
Dear Dengwei,
 
   原PR已经验证通过并关闭，请关掉此PR，谢谢！
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: