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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Stephanie Ouillon [:arroway] (needinfo me)]

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

Comment 1

[Gregor Wagner [:gwagner]]

Can we get UX input here as well?

Comment 2

[Gregor Wagner [:gwagner]]

Had a discussion during standup. We are not blocking the release on it.

Comment 3

[Kevin Grandon :kgrandon]

I'll look at this.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1138508_nfc_sharing_private_landing > mozilla-b2g:master

Comment 5

[Kevin Grandon :kgrandon]

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!

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

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?

Comment 7

[Ben Francis [:benfrancis]]

(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?

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

(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 9

[Krzysztof Mioduszewski[:tauzen]]

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!).

Comment 10

[Kevin Grandon :kgrandon]

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?

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

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

Looks good now, thanks!

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/52cc47708d378ba0a46ec518b7348ab2518a8e26

Comment 13

[Gregor Wagner [:gwagner]]

Should we uplift?

Comment 14

[Kevin Grandon :kgrandon]

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.

Comment 15

[Shine(Leave from Mozilla)]

Attachment: 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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/af2f4b83c187e0e190526f8c4a32ede4c470131e

Comment 17

[Shine(Leave from Mozilla)]

Attachment: 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