[NFC] Unable to share image from gallery, if this image was once previewed through notification

VERIFIED FIXED in Firefox 41

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: wesley_huang, Assigned: allstars.chh)

Tracking

unspecified
2.2 S13 (29may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(10 attachments)

12.60 KB, patch
dimi
: review+
Details | Diff | Splinter Review
11.42 KB, patch
dimi
: review+
Details | Diff | Splinter Review
4.24 KB, patch
dimi
: review+
Details | Diff | Splinter Review
7.57 KB, patch
dimi
: review+
Details | Diff | Splinter Review
8.36 KB, patch
dimi
: review+
Details | Diff | Splinter Review
12.28 KB, patch
Details | Diff | Splinter Review
10.99 KB, patch
Details | Diff | Splinter Review
3.87 KB, patch
Details | Diff | Splinter Review
7.22 KB, patch
Details | Diff | Splinter Review
7.85 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1162461 +++
// we made bug 1162461 invalid because STR was incomplete to reproduce
// modified former step 1 (Run gallery in the background) into 1-1&1-2 to the STR 

Build ID               20150506162500
Gaia Revision          c6a6996841860ab335bf46b273477dc4bef19c95
Gaia Date              2015-05-06 17:01:57
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c7fafa53b4e7
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150506.195956
Firmware Date          Wed May  6 20:00:07 EDT 2015
Bootloader             L1TC100118D0

STR:
1-1. Run gallery
1-2. Open an arbitrary image, and then tap 'back' icon to jump back to grid view
2. Receive an image, and open it from notification
3. Exit image preview
4. Run gallery in the foreground, and select that received image
5. Tap device with others, swipe shrinking UI to share the selected image

Expected result:
Share image successfully

Actual result:
Unable to share image until relaunch gallery app, please refer: https://youtu.be/w-d3mrwM9xg  
Note that the video didn't record step 1-2 but it was essential to repro.
Carrying over the blocking status from bug 1162461
blocking-b2g: --- → 2.2+
(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #0)
> Actual result:
> Unable to share image until relaunch gallery app, please refer:
> https://youtu.be/w-d3mrwM9xg  
> Note that the video didn't record step 1-2 but it was essential to repro.
rephrase, The step 1-2 is essential to reproduce.
Summary: [NFC] Unable to share a received image from gallery, if this image was once previewed through notification → [NFC] Unable to share image from gallery, if this image was once previewed through notification
Assignee: nobody → allstars.chh

Updated

4 years ago
QA Whiteboard: [COM=NFC]
When the notification pops up, it will have two seperate windows running the same appId on the same process. But the original code doesn't assume this, it assumes only 1 window will be running on the same process. So I refactored all code related to window in NFC DOM part.

However these patches depend on Bug 1166595, since we need System app to update the foreground app information after the notification window is closed.

But I think these patches won't fix all the cases around notification, specially for the legacy code onpeerready.

1. when setting onpeerready it will send appId information into parent process, however when there are two windows having the same appId at the same time, so information will be overridden.

2. These patches should fix v2.2 and master, however back in v2.1 we don't have such thing as 'setFocusApp', so when System app calls notifyUserAccepted. The argument of that API is manifestURL, which should be updated as well, otherwise Gecko can't tell which window is actually going to share data if both windows are nfc-sharable. I'll see if we need to support this since it requires lots of change including Gaia.
Attachment #8608453 - Flags: review?(dlee) → review+
Attachment #8608454 - Flags: review?(dlee) → review+
Attachment #8608456 - Flags: review?(dlee) → review+
Attachment #8608457 - Flags: review?(dlee) → review+
Comment on attachment 8608458 [details] [diff] [review]
Part 5: remove listener when window is destroyed.

Review of attachment 8608458 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!
Attachment #8608458 - Flags: review?(dlee) → review+
I'll uplift this to v2.2 once Bug 1166595 is also uplift to v2.2

Comment 13

4 years ago
Verified on master: 

Build ID               20150525160205
Gaia Revision          5bcc08a732163087999251b523e3643db397412c
Gaia Date              2015-05-24 14:44:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/b6623a27fa64
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150525.192755
Firmware Date          Mon May 25 19:28:07 EDT 2015
Bootloader             L1TC100118D0
Please request b2g37 approval on these patches when you get a chance.
Flags: needinfo?(allstars.chh)
Comment on attachment 8613928 [details] [diff] [review]
(v2.2) Part 1: add window into nsINfcEventListener

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 #): 
Bug 933136

User impact if declined: 
User cannot share image by NFC after viewing notification

Testing completed: 
Manually

Risk to taking this patch (and alternatives if risky):
This bug causes regression in Bug 1168292, will need to uplift those patches in Bug 1168292 as well.

String or UUID changes made by this patch:
only changed UUID on internal IDL (nsINfcContentHelper.idl)
Attachment #8613928 - Flags: approval-mozilla-b2g37?
Comment on attachment 8613929 [details] [diff] [review]
(v2.2)  Part 2: move window-related code to nsNfc.js


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 933136

User impact if declined: 
User cannot share image by NFC after viewing notification

Testing completed: 
Manually

Risk to taking this patch (and alternatives if risky):
This bug causes regression in Bug 1168292, will need to uplift those patches in Bug 1168292 as well.

String or UUID changes made by this patch:
only changed UUID on internal IDL (nsINfcContentHelper.idl)
Attachment #8613929 - Flags: approval-mozilla-b2g37?
Comment on attachment 8613930 [details] [diff] [review]
(v2.2) Part 3: remove _rfState and init from NfcContentHelper.js


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 933136

User impact if declined: 
User cannot share image by NFC after viewing notification

Testing completed: 
Manually

Risk to taking this patch (and alternatives if risky):
This bug causes regression in Bug 1168292, will need to uplift those patches in Bug 1168292 as well.

String or UUID changes made by this patch:
only changed UUID on internal IDL (nsINfcContentHelper.idl)
Attachment #8613930 - Flags: approval-mozilla-b2g37?
Comment on attachment 8613931 [details] [diff] [review]
(v2.2) Part 4: using eventListeners.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 933136

User impact if declined: 
User cannot share image by NFC after viewing notification

Testing completed: 
Manually

Risk to taking this patch (and alternatives if risky):
This bug causes regression in Bug 1168292, will need to uplift those patches in Bug 1168292 as well.

String or UUID changes made by this patch:
only changed UUID on internal IDL (nsINfcContentHelper.idl)
Attachment #8613931 - Flags: approval-mozilla-b2g37?
Comment on attachment 8613932 [details] [diff] [review]
(v2.2) Part 5: remove listener when window is destroyed.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 933136

User impact if declined: 
User cannot share image by NFC after viewing notification

Testing completed: 
Manually

Risk to taking this patch (and alternatives if risky):
This bug causes regression in Bug 1168292, will need to uplift those patches in Bug 1168292 as well.

String or UUID changes made by this patch:
only changed UUID on internal IDL (nsINfcContentHelper.idl)
Attachment #8613932 - Flags: approval-mozilla-b2g37?

Updated

4 years ago
Attachment #8613928 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

4 years ago
Attachment #8613929 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

4 years ago
Attachment #8613930 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

4 years ago
Attachment #8613931 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

4 years ago
Attachment #8613932 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Comment 26

4 years ago
Verified on v2.2:

Build ID               20150607162503
Gaia Revision          8fc797527a3eca7665bc1d1828848f2fb77ca99f
Gaia Date              2015-06-04 07:46:11
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d213237e11e9
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150607.201513
Firmware Date          Sun Jun  7 20:15:24 EDT 2015
Bootloader             L1TC100118D0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.