1277467 - "Showing offline version" snackbar appears outside of the tab in question

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Grisha Kruglov]

Attachment: Screenshot_20160601-181506.png

- Open some website, wait for it to load and make sure it's in the cache.
- Go into airplane mode
- Open that website in a new tab (say, from Top Sites. Or History panel)
- Once it finishes loading from cache, you'll see a blue snackbar indicating that offline version was loaded. But, we're not on that tab!
- For bonus points, I believe that if you manage to get into settings while a tab is loading, you'll see the snackbar there as well

Ideally, we will only show that snackbar if:
- user is on the tab in question and it finished loading
- user is not on tab in question, and switches to it after it finished loading

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

'Offline version' snackbar will be shown: if tab that was loaded offline is currently active,
or if tab was loaded in background, and then user selected it. Snackbar will only be shown once per tab.

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

Comment 2

[:Grisha Kruglov]

Attachment: Screenshot_20160602-184514.png

Comment 3

[:Grisha Kruglov]

Attached patch doesn't fix problem in Comment 2 (and its variations... while looking at a list of tabs you might see "offline version" snackbar, for example) , but does fix the problem in Comment 1.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Grisha Kruglov from comment #3)
> Attached patch doesn't fix problem in Comment 2 (and its variations... while
> looking at a list of tabs you might see "offline version" snackbar, for
> example) , but does fix the problem in Comment 1.

We had similar problems with prompts for "Add to home screen" etc. However Java knows more about the current state of the UI:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/promotion/AddToHomeScreenPromotion.java#70-88

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759444 [details]
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

https://reviewboard.mozilla.org/r/57428/#review55062

Queuing the snackbar seems like a good idea but I'm not sure how this is going to work with restoring tabs after they have been zombified (I'll flag JanH as a second reviewer. I guess he'll know best). I wonder if there are other edge cases where we queued the message but the tab state has changed in a way that it's not correct to show it anymore.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759444 [details]
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

@JanH: What's your opinion about the approach here? Especially in regards to zombifying/restoring tabs. If we are low on memory and zombify the tab then the next time the user selects the tab we'd reload the tab but also show the offline snackbar which might not be correct?

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8759444 [details]
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

https://reviewboard.mozilla.org/r/57428/#review55094

Good point. As far as I know and speaking empirically, restoring a zombified tab always prefers loading from the cache, even when we *do* have network connectivity, so in any case showing the message when restoring a zombifed tab in offline mode won't technically be wrong :-)

On the other hand, I suppose that tab zombification and session restoring are supposed to be as transparent as possible, i.e. ideally the browser should be in the same state as if that tab had never been zombified/the browser had never been closed and killed from memory. (Even though in practice it's probably not working quite as perfectly, plus the fact that those tabs will be visibly reloading is also a giveaway).

I think the following cases are relevant here:
1. A user loads a tab in offline mode but switches away before we can show the snackbar. The tab is then zombifed while in background. Finally, the user switches back to that tab and it is restored
a) while still in offline mode.
b) after having regained connectivity.
In practice, restoring a zombified tab will always reload it from the cache, so I think showing the notification in both cases is correct, since the original tab load occured while in offline mode. The current implementation shows a snackbar in both cases.

2. A user loads a tab in online mode and switches to a different tab. The original tab is zombified in background. Finally, the user enters offline mode and switches back to the original tab.
Since the tab was originally loaded in online mode and restoring a zombie tab means we try reloading the page from the cache no matter what, I'd tend to the opinion that we shouldn't show the snackbar here.
The current implementation shows a snackbar here during offline mode.

There's a possible slight edge case here, though, if the tab is zombified before it has fully loaded, although that's probably not too different from the alternative scenario where the tab isn't zombified but instead the network connection drops off before loading finished - in both cases you end up with an incompletely loaded tab.

2b). Firefox wasn't running and is starting up. If session restore is active, the last sessions gets restored automatically. Only the foreground tab is actually loaded, while all other tabs are created as zombie tabs which are only loaded on demand.
The session will be restored from the cache in any case, however currently the snackbar is only shown if we're actually offline at that time. On the same basis as above, I'd tend to say that we shouldn't show a snackbar here, too.

So basically when restoring a zombie tab, if there's already a snackbar queued show it, but otherwise don't trigger/queue any fresh offline mode snackbars. I have a feeling that this problem I'm seeing might actually be the opposite of what Sebastian had in mind?

In any case I've got no problem with moving the discussion about what we actually want to do in the above cases to a follow-up bug, though, so if you want to land this as it is, you can go ahead.

::: mobile/android/chrome/content/browser.js:7399
(Diff revision 1)
>          // Notify if we are loading a page from cache.
>          if (this._useCache) {
>            let targetDoc = aEvent.originalTarget;
> +          let tabForEvent = BrowserApp.getTabForBrowser(BrowserApp.getBrowserForDocument(targetDoc));
>            let isTopLevel = (targetDoc.defaultView.parent === targetDoc.defaultView);
>  

Maybe check somewhere between here and the point were we're showing/enqueueing the snackbar whether we're actually restoring a delay loaded/zombified tab and return early in that case.

::: mobile/android/chrome/content/browser.js:7413
(Diff revision 1)
> +
> +          // If we're currently on the tab in question, show the offline snackbar.
> +          if (tabForEvent.id === BrowserApp.selectedTab.id) {
> +            this.showOfflineSnackbar();
> +
> +          // Otherwise, remember to show the offline Snackbar when tab is selected later on.

Nit: In all the other comments, "snackbar" hasn't been capitalised.

Comment 8

[:Grisha Kruglov]

Attachment: Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

- Add a new Tab:LoadedFromCache event, and use it to inform Tabs that they were actively loaded from cache
- Move offline notification logic away from browser.js and into a delegate, which displays notifications when
tab in question is user-visible

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

Comment 9

[:Grisha Kruglov]

Comment on attachment 8759444 [details]
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

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

Comment 10

[:Grisha Kruglov]

I've re-implemented notification display logic in a BrowserDelegate-extending class and removed it all from browser.js. Couldn't figure out another way to get around notifications being displayed on top of the tabs tray. 

The idea is still the same - display notification if tab is user-visible, and queue it up for later otherwise.

There is currently one oddity with this. When we're on the Tabs tray, and user swipes away currently selected tab, we select a new tab and send it SELECTED event. However, when we then click through into that tab, tab doesn't receive any additional events. I would expect SELECTED to be sent again, but alas that doesn't happen.
So currently, there's a bug:
- if you're offline, and open up a bunch of tabs in the background
- then switch to the latest tab, you will see "Showing offline version" message. Great!
- open up tabs tray. Swipe away currently active tab. Tab which was right above it gets selected. Open it (call it TAB A)
- No offline notification is displayed :(
- Go into some other tabs (notification is displayed correctly).
- Go back to TAB A - offline notification is displayed correctly (since it was queued up, and waiting for display).

I feel that this edge case should be addressed separately, since the fix would be to ensure we always send a SELECTED event when tab is opened. (unless there are some good reasons we don't do this already...).

Comment 11

[:Grisha Kruglov]

Comment on attachment 8759444 [details]
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

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

Comment 12

[:Grisha Kruglov]

Comment on attachment 8761304 [details]
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

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

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/57428/#review55474

::: mobile/android/base/java/org/mozilla/gecko/delegates/ForegroundAwareDelegate.java:13
(Diff revision 3)
> +import android.os.Bundle;
> +
> +import org.mozilla.gecko.BrowserApp;
> +import org.mozilla.gecko.tabs.TabsPanel;
> +
> +public abstract class ForegroundAwareDelegate extends BrowserAppDelegate {

Technically this delegate should implement onResume() and onPause() too and set isInForeground. 

The classes using this right now solve this by registering /unregistering and strictly do not need this. But the name implies that this delegate trakcs the foreground state and right now doesn't always.

Also: Should we make it more clear that this delegate is about the foreground state of the tab? Either by naming the delegate differently or adding javadoc.

::: mobile/android/base/java/org/mozilla/gecko/delegates/ForegroundAwareDelegate.java:14
(Diff revision 3)
> +
> +import org.mozilla.gecko.BrowserApp;
> +import org.mozilla.gecko.tabs.TabsPanel;
> +
> +public abstract class ForegroundAwareDelegate extends BrowserAppDelegate {
> +    protected boolean isInForeground;

Should we make this private and add a getter? Just to make it clear that this value is for reading only.

::: mobile/android/base/java/org/mozilla/gecko/delegates/ForegroundAwareDelegate.java:17
(Diff revision 3)
> +
> +public abstract class ForegroundAwareDelegate extends BrowserAppDelegate {
> +    protected boolean isInForeground;
> +
> +    @Override
> +    public void onCreate(BrowserApp browserApp, Bundle savedInstanceState) {

With @CallSuper you can instruct the IDE/developer that any overriding methods should invoke this method too:

https://developer.android.com/reference/android/support/annotation/CallSuper.html

::: mobile/android/base/java/org/mozilla/gecko/delegates/ForegroundAwareDelegate.java:22
(Diff revision 3)
> +    public void onCreate(BrowserApp browserApp, Bundle savedInstanceState) {
> +        isInForeground = true;
> +    }
> +
> +    @Override
> +    public void onTabsTrayShown(BrowserApp browserApp, TabsPanel tabsPanel) {

@CallSuper

::: mobile/android/base/java/org/mozilla/gecko/delegates/ForegroundAwareDelegate.java:27
(Diff revision 3)
> +    public void onTabsTrayShown(BrowserApp browserApp, TabsPanel tabsPanel) {
> +        isInForeground = false;
> +    }
> +
> +    @Override
> +    public void onTabsTrayHidden(BrowserApp browserApp, TabsPanel tabsPanel) {

@CallSuper

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8761304 [details]
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

https://reviewboard.mozilla.org/r/58544/#review55478

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:628
(Diff revision 2)
>          VIEWPORT_CHANGE,
>          RECORDING_CHANGE,
>          BOOKMARK_ADDED,
>          BOOKMARK_REMOVED,
>          AUDIO_PLAYING_CHANGE,
> +        LOADED_FROM_CACHE,

See below: I think this should be something like PAGE_SHOW.

::: mobile/android/base/java/org/mozilla/gecko/delegates/OfflineTabStatusDelegate.java:28
(Diff revision 2)
> +
> +/**
> + * Displays "Showing offline version" message when tabs are loaded from cache while offline.
> + */
> +public class OfflineTabStatusDelegate extends ForegroundAwareDelegate implements Tabs.OnTabsChangedListener {
> +    private WeakReference<Activity> activityReference;

We recently added BrowserAppDelegateWithReference for this. Theoretically this class needs ForegroundAwareWithReferenceDelegate :-)
This kind of composing is really not nice in Java.

::: mobile/android/base/java/org/mozilla/gecko/delegates/OfflineTabStatusDelegate.java:29
(Diff revision 2)
> +/**
> + * Displays "Showing offline version" message when tabs are loaded from cache while offline.
> + */
> +public class OfflineTabStatusDelegate extends ForegroundAwareDelegate implements Tabs.OnTabsChangedListener {
> +    private WeakReference<Activity> activityReference;
> +    private ArrayList<Integer> tabIdQueueToShowOfflineSnackbar = new ArrayList<>();

As we use this list to only lookup and remove values: Isn't this a job for a map? The list will never be very long so I guess it doesn't really matter performance wise.

However how do we handle tabs that get closed without getting selected before? For exmaple: I'm offline, open the browser and it restores 10 tabs. I close the tabs and continue to browse in a new tab: Do we still have the 10 tab ids in this list?

How about we use the tab here and just keep a weak reference: If the tab disappears the queued offline snackbar disappears too.

Something like:

WeakHashMap<Tab, Void> offlineLoadedTabs ...
... offlineLoadedTabs.put(tab, null);
... offlineLoadedTabs.remove(tab);
... offlineLoadedTabs.containsKey(tab)

::: mobile/android/base/java/org/mozilla/gecko/delegates/OfflineTabStatusDelegate.java:57
(Diff revision 2)
> +        // Ignore "about:" tabs
> +        if (shouldIgnoreOfflineStatusOfTab(tab)) {

Ultra nitpicking: This comment is about an implementation detail of shouldIgnoreOfflineStatusOfTab(). It gets pretty easily outdated when the behavior of shouldIgnoreOfflineStatusOfTab() changes. Either do not comment or do the actual check you are commenting about. :-)

::: mobile/android/base/java/org/mozilla/gecko/delegates/OfflineTabStatusDelegate.java:100
(Diff revision 2)
> +     * Don't show offline version notifications for "about:*" URLs.
> +     *
> +     * @param tab Tab for which we check the URL
> +     */
> +    private static boolean shouldIgnoreOfflineStatusOfTab(final Tab tab) {
> +        return tab.getURL().startsWith("about:");

You can find such helper methods in AboutPages.

::: mobile/android/chrome/content/browser.js:7333
(Diff revision 2)
>  
>  var Tabs = {
>    _enableTabExpiration: false,
>    _useCache: false,
>    _domains: new Set(),
> +  _tabIdQueueToShowOfflineSnackbar: [],

This shouldn't be needed anymore, or?

::: mobile/android/chrome/content/browser.js:7406
(Diff revision 2)
> +        Messaging.sendRequest({
> +          type: "Tab:LoadedFromCache",
> +          tabID: tabForEvent.id,
> +          fromCache: this._useCache
> +        });

It looks like we send this event every time and it's independent from whether we used the cache or not. Following the naming of the event here, shouldn't this be "Tab:PageShow"?

Comment 15

[:Grisha Kruglov]

Comment on attachment 8759444 [details]
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible

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

Comment 16

[:Grisha Kruglov]

Comment on attachment 8761304 [details]
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

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

Comment 17

[:Grisha Kruglov]

Comment on attachment 8761304 [details]
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

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

Comment 18

[:Grisha Kruglov]

https://reviewboard.mozilla.org/r/57428/#review55474

> Technically this delegate should implement onResume() and onPause() too and set isInForeground. 
> 
> The classes using this right now solve this by registering /unregistering and strictly do not need this. But the name implies that this delegate trakcs the foreground state and right now doesn't always.
> 
> Also: Should we make it more clear that this delegate is about the foreground state of the tab? Either by naming the delegate differently or adding javadoc.

I've renamed it to not imply that it's doing more than it really is - keeping track of TabsTray state.

Comment 19

[:Grisha Kruglov]

https://reviewboard.mozilla.org/r/58544/#review55478

> As we use this list to only lookup and remove values: Isn't this a job for a map? The list will never be very long so I guess it doesn't really matter performance wise.
> 
> However how do we handle tabs that get closed without getting selected before? For exmaple: I'm offline, open the browser and it restores 10 tabs. I close the tabs and continue to browse in a new tab: Do we still have the 10 tab ids in this list?
> 
> How about we use the tab here and just keep a weak reference: If the tab disappears the queued offline snackbar disappears too.
> 
> Something like:
> 
> WeakHashMap<Tab, Void> offlineLoadedTabs ...
> ... offlineLoadedTabs.put(tab, null);
> ... offlineLoadedTabs.remove(tab);
> ... offlineLoadedTabs.containsKey(tab)

WeakHashMaps' concurrency model is interesting, it reminds me of a mine field :) In this case it's a simple enough logic to reason about though.

> It looks like we send this event every time and it's independent from whether we used the cache or not. Following the naming of the event here, shouldn't this be "Tab:PageShow"?

Removed this entirely, and instead augmented the PageShow event itself to include "fromCache" boolean.

Comment 20

[:Grisha Kruglov]

Addressed comments and cleaned things up.

A slightly off-topic thought about this bug... It's kind of a meaningless message, isn't it? The way it's worded, "offline version", is implying that somehow when we're loading a page with a data connection, it is "online" - what does that even mean? The page surely isn't being kept up-to-date by the browser (although JS logic might do just that). We're really talking about "age" here. Any tab which was loaded in a background, left for a few hours and somehow wasn't zombified is, by this definition, "offline". What we should be saying is something akin to "Showing a version that's 2 days old" - that's much more meaningful.

I'm not sure we have easy access to how old cached content is, haven't done my research yet - although that must be available somehow.

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Grisha Kruglov from comment #20)
> Addressed comments and cleaned things up.
> 
> A slightly off-topic thought about this bug... It's kind of a meaningless
> message, isn't it? The way it's worded, "offline version", is implying that
> somehow when we're loading a page with a data connection, it is "online" -
> what does that even mean? The page surely isn't being kept up-to-date by the
> browser (although JS logic might do just that). We're really talking about
> "age" here. Any tab which was loaded in a background, left for a few hours
> and somehow wasn't zombified is, by this definition, "offline". What we
> should be saying is something akin to "Showing a version that's 2 days old"
> - that's much more meaningful.
> 
> I'm not sure we have easy access to how old cached content is, haven't done
> my research yet - although that must be available somehow.

Out of scope of this bug. 

But, yes the copy could be improved. It is one of the things on the list as we work out the high level goals of this feature.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8761304 [details]
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

https://reviewboard.mozilla.org/r/58544/#review57388

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:625
(Diff revision 4)
>          DESKTOP_MODE_CHANGE,
>          VIEWPORT_CHANGE,
>          RECORDING_CHANGE,
>          BOOKMARK_ADDED,
>          BOOKMARK_REMOVED,
> -        AUDIO_PLAYING_CHANGE,
> +        AUDIO_PLAYING_CHANGE

nit: Unrelated change?

::: mobile/android/chrome/content/browser.js:7476
(Diff revision 4)
>    dump: function(aPrefix) {
>      let tabs = BrowserApp.tabs;
>      for (let i = 0; i < tabs.length; i++) {
>        dump(aPrefix + " | " + "Tab [" + tabs[i].browser.contentWindow.location.href + "]: lastTouchedAt:" + tabs[i].lastTouchedAt + ", zombie:" + tabs[i].browser.__SS_restore);
>      }
> -  },
> +  }

nit: Unrelated?

Comment 23

[:Grisha Kruglov]

Comment on attachment 8761304 [details]
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate

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

Comment 24

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/9ef1ffed83af812c84c76ee620959b3313065e8b
Bug 1277467 - Pre: Add ForegroundAwareDelegate to help track if Gecko tabs are user-visible r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/7d94f5bdb7ce8527eabe513022dd2cf611a50b9f
Bug 1277467 - Add OfflineTabStatusDelegate for displaying tab-is-offline notifiations when appropriate r=sebastian

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ef1ffed83af
https://hg.mozilla.org/mozilla-central/rev/7d94f5bdb7ce

Comment 26

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 2016-07-14;
Device: Nexus 5 (Android 6.0.1).