Don't initiate NFC sharing when on the browser private window start page

VERIFIED FIXED in Firefox OS v2.2

Status

VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: arroway, Assigned: kgrandon)

Tracking

({polish})

unspecified
2.2 S7 (6mar)
x86_64
Linux
polish

Firefox Tracking Flags

(b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Gaia-Rev        77609916ca5ab721150fab2b7bc5c37f43ee3a5a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/27ab8aa34201
Build-ID        20150302002504
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150302.042655
FW-Date         Mon Mar  2 04:27:05 EST 2015
Bootloader      L1TC100118D0


STR:
1. launch the Settings app on two phones
2. Turn on NFC on both phones
3. Launch the Browser on phone A
4. Open a private window and stay on the home page (empty by default)
4. Tap the 2 phones

Expected result:
The private window homepage page should not show shrinking UI because it could not be shared

Actual result:
The private window homepage  page show shrinking UI

See also bug 1000678
(Reporter)

Updated

4 years ago
See Also: → bug 1126249
Whiteboard: [systemsfe]
blocking-b2g: --- → 2.2?
Can we get UX input here as well?
Flags: needinfo?(firefoxos-ux-bugzilla)
Had a discussion during standup. We are not blocking the release on it.
blocking-b2g: 2.2? → ---
Flags: needinfo?(firefoxos-ux-bugzilla)
Keywords: polish
(Assignee)

Comment 3

4 years ago
I'll look at this.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Created attachment 8573674 [details] [review]
[gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master
(Assignee)

Comment 5

4 years ago
Comment on attachment 8573674 [details] [review]
[gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master

Krzysztof - Any chance you'd be able to review this small NFC patch for me? Thanks!
Attachment #8573674 - Flags: review?(kmioduszewski)
The patch looks ok (cool that it prevents not needed IPC), but are we absolutely sure this cannot be fixed on the browser side? I think that like in Bug 1000678, the browser should not be listening for onpeerready if there is no actual content which can be shared. 

Ben since you worked on Bug 1000678, is it possible to stop NFC sharing on private browsing landing page? Is this complicated?
Flags: needinfo?(bfrancis)
(In reply to Krzysztof Mioduszewski[:tauzen] from comment #6)
> Ben since you worked on Bug 1000678, is it possible to stop NFC sharing on
> private browsing landing page? Is this complicated?

That was the old browser app, in the new system browser it looks like the conditional is:

  if (currentApp && currentApp.isBrowser() && currentApp.config.url)

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handler.js#L24

The private browser start page (an HTML page in the system app) is implemented differently to the normal browser start page (the search app) so I guess this same conditional doesn't work. Maybe it just needs tweaking?
Flags: needinfo?(bfrancis)
(In reply to Ben Francis [:benfrancis] from comment #7)
> The private browser start page (an HTML page in the system app) is
> implemented differently to the normal browser start page (the search app) so
> I guess this same conditional doesn't work. Maybe it just needs tweaking?

Ok, I wasn't aware that normal browser start page is a different app. So this explains why it's not shrinking. So actually we're not able to tweak anything here, since |NfcHandler| code is fired after shrinking ui is shown and accepted. The patch proposed by Kevin is the best solution.
Comment on attachment 8573674 [details] [review]
[gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master

Test needs some fixing. Left comments on github, by mistake directly on commit not in pull request view (sorry for that!).
Attachment #8573674 - Flags: review?(kmioduszewski)
(Assignee)

Comment 10

4 years ago
Comment on attachment 8573674 [details] [review]
[gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master

Hey Krzysztof - thanks for the review. Those test assertions were totally wrong so nice catch.

I've addressed the github comments - could you take another look?
Attachment #8573674 - Flags: review?(kmioduszewski)
Comment on attachment 8573674 [details] [review]
[gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master

Looks good now, thanks!
Attachment #8573674 - Flags: review?(kmioduszewski) → review+
(Assignee)

Updated

4 years ago
Component: Gaia::Browser → Gaia::System
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
Should we uplift?
(Assignee)

Comment 14

4 years ago
Comment on attachment 8573674 [details] [review]
[gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master

(In reply to Gregor Wagner [:gwagner] from comment #13)
> Should we uplift?

It's a small/simple enough patch that I don't see why not.


[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Private browser implementation.
[User impact] if declined: If two FirefoxOS phones happen to meet in the wild, and they are *both* using NFC *and* one happens to have the private browsing landing page opened *and* the phones touch, one could be given a UI which is not appropriate.
[Testing completed]: Manual and unit tests.
[Risk to taking this patch] (and alternatives if risky): Low risk, fairy small and contained patch.
[String changes made]: None.
Attachment #8573674 - Flags: approval-gaia-v2.2?(bbajaj)

Updated

4 years ago
Attachment #8573674 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+

Updated

4 years ago
Keywords: verifyme
Created attachment 8577926 [details]
video

1. This problem is verified pass on latest build of Flame 3.0.
Verify result: The private window homepage page cannot be shared, it will not show shrinking UI.
2. I have also verified bug 1000678, the verify result is Pass. The Browser Homepage page cannot be shared, it will not show shrinking UI.
See attachment: Verify_video.MP4
Rate: 0/10

Flame 3.0 build: (Pass)
Build ID               20150315160203
Gaia Revision          d4177902b04b8fedcb7df9a30ae6e9677e03d2d4
Gaia Date              2015-03-13 15:58:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/af68c9c0e903
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.192711
Firmware Date          Sun Mar 15 19:27:22 EDT 2015
Bootloader             L1TC000118D0
QA Whiteboard: [MGSEI-Triage+]
status-b2g-master: --- → verified
Keywords: verifyme
v2.2: https://github.com/mozilla-b2g/gaia/commit/af2f4b83c187e0e190526f8c4a32ede4c470131e
status-b2g-v2.2: --- → fixed
Target Milestone: 2.2 S8 (20mar) → 2.2 S7 (6mar)
Created attachment 8629805 [details]
Verify_Pass_2.2.3gp

This problem is verified as "pass" on latest build of Flame 2.2 and N5 2.2.
Actual result: The Browser Homepage page and private window homepage page cannot be shared, it will not show shrinking UI.
See attachment: Verify_Pass_2.2.3gp
Rate: 0/5

Device information:
Flame 2.2 build (Pass)
Build ID               20150705002505
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0214b4c1ea0
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150705.040440
Firmware Date          Sun Jul  5 04:04:51 EDT 2015
Bootloader             L1TC000118D0

N5 2.2 build (Pass)
Build ID               20150705002505
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0214b4c1ea0
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150705.041046
Firmware Date          Sun Jul  5 04:11:05 EDT 2015
Bootloader             HHZ12f
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified
You need to log in before you can comment on or make changes to this bug.