1353857 - Fix handling of entering/leaving Web App

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), enhancement, P2)

Description

[Jan Henning [:JanH]]

At the moment, the message passed by Gecko (Website:AppEntered/Left) doesn't even include the tab ID, which means that the address bar can be shown when it shouldn't be or vice versa.

To be done after bug 1352997, though, so we can do it properly...

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

We only want to process the AppEntered/Left message if it actually concerns our currently displayed tab.

Review commit: https://reviewboard.mozilla.org/r/130846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130846/

Comment 3

[Jan Henning [:JanH]]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130846/diff/1-2/

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

https://reviewboard.mozilla.org/r/130846/#review135714

Minor nit, otherwise looks great, cheers

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:113
(Diff revision 2)
>      }
>  
>      @Override
>      public void handleMessage(final String event, final GeckoBundle message,
>                                final EventCallback callback) {
> +        if (message.containsKey("tabId") && message.getInt("tabId") == mLastSelectedTabId) {

I am not 100% clear on our or Java's coding style preferences, but early returns always seem cleaner for this 

   if (!message.containsKey("tabId") || message.getInt("tabid") != mLastSelectedTabId) { 
       return;
   }
   
Feel free to ignore especially if it goes against our standard

Comment 5

[Julian Chu [:walkingice]]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

https://reviewboard.mozilla.org/r/130846/#review135718

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

https://reviewboard.mozilla.org/r/130846/#review135714

> I am not 100% clear on our or Java's coding style preferences, but early returns always seem cleaner for this 
> 
>    if (!message.containsKey("tabId") || message.getInt("tabid") != mLastSelectedTabId) { 
>        return;
>    }
>    
> Feel free to ignore especially if it goes against our standard

As things currently stand, you're right. I was just thinking that eventually we might need to handle other events with possibly different pre-conditions which might not be compatible with early returns, but I suppose we can worry about that situation if we actually run into it.

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130846/diff/2-3/

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130846/diff/3-4/

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130846/diff/4-5/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ce0883ffd62
Include the tab ID when notifying about leaving/entering a web app's scope. r=daleharvey,walkingice

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out because the dependent bug failed to land: https://hg.mozilla.org/integration/autoland/rev/483340ef5362

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1219abce0035
Include the tab ID when notifying about leaving/entering a web app's scope. r=daleharvey,walkingice

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1219abce0035

Comment 14

[Oana Horvath [:ohorvath]]

Build: Nightly 2017-05-04
Devices: HTC 10 (Android 6) & Prestigio Grace X5 (Android 4.4.2)
On Twitter & Pokedex.org web apps, when you open an external link, it's opened in a new browser tab, not inside the web app.

Comment 15

[Dale Harvey (:daleharvey)]

Twitter for me opens the link in a new browser tab, which I believe is a change on twitter side so it now behaves the same as chrome

Pokedex behaves the same for me and navigates with the header

Comment 16

[Jan Henning [:JanH]]

- That's not caused by this bug, so if we wanted to change this, this belongs in a separate bug.
- Yup, those links are opening and selecting a new (normal) tab, so because of bug 1351739 this'll trigger an activity switch.
- If we don't want this, this (https://dxr.mozilla.org/mozilla-central/rev/4a6a71f4aa22e4dc3961884ce505ce34bdd799a2/mobile/android/chrome/content/browser.js#3404) is the place to control this behaviour (whether to open a new tab or not), but that's a matter of taste (and if Chrome does it as well, that's one less reason to change our behaviour)
- Re testing with Pokedex, the link at the bottom of the page stays within the tab, but the "About" link in the sidebar menu opens a new tab.

Comment 17

[Dale Harvey (:daleharvey)]

Ah thats how it works, yeh I have been meaning to look into why twitter links were previously opened in place but hadnt got to it. So basically 1351739 fixed _blank links, which is perfect :)

Comment 18

[Oana Horvath [:ohorvath]]

Yup, you're right, it behaves the same with Chrome. I'll mark this as fixed, thanks!