697063 - Tabs.getSelectedTab returns null until Gecko starts and will cause crashes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We have a lot of code that gets the selected tab and preformsa an action:

Tab tab = Tabs.getInstance().getSelectedTab();
// do something to tab

Currently, there are no "tabs" until gecko starts, so trying to perform one of those actions too early during startup will result in a crash. We could add null checks to all the code spots, or we could create a dummy tab right away.

I really don't know which is better. I suppose a dummy tab _could_ make some actions seem to work, like "Share", but others would be useless, like "Reload".

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch makes sure that the awesomebar is shown without crashing when gecko is loading.
When gecko is loading, the url is not shown though.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 569417 [details] [diff] [review]
Patch

What about all the other failure modes? Like "Share" and "Reload"

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Reload, Bookmarks and Share have been take care of in this patch.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 569425 [details] [diff] [review]
Patch


>            case R.id.bookmarks:
>                Intent intent = new Intent(this, GeckoBookmarks.class);
>                tab = Tabs.getInstance().getSelectedTab();
>+               if (tab == null) {
>+                   startActivity(intent);
>+                   return true;
>+               }
>+
>                he = tab.getLastHistoryEntry();
>                if (he != null) {
>                    intent.setData(android.net.Uri.parse(he.mUri));
>                    intent.putExtra("title", he.mTitle);
>                    startActivity(intent);
>                }
>                return true;

We need to convert this to "Add/Remove Bookmark" toggle. Showing the list is now part of Awesombar. In a new bug I guess.

>         if (aType != AwesomeBar.Type.ADD) {
>             // if we're not adding a new tab, show the old url
>             Tab tab = Tabs.getInstance().getSelectedTab();
>+            if (tab == null) {
>+                startActivityForResult(intent, AWESOMEBAR_REQUEST);
>+                return true;
>+            }
>+
>             if (!tab.getHistory().empty()) {
>                 intent.putExtra(AwesomeBar.CURRENT_URL_KEY, tab.getHistory().peek().mUri);
>             }
>         }
>         startActivityForResult(intent, AWESOMEBAR_REQUEST);
>         return true;

I don't like having two "startActivityForResult". Wouldn't this work?

if (tab != null && !tab.getHistory().empty()) {
...
}


>diff --git a/embedding/android/GeckoBookmarks.java b/embedding/android/GeckoBookmarks.java

fine for now, but we should probably remove/refactor GeckoBookmarks.java since the list is now in Awesombar

r- for the startActivityForResult change

Comment 5

[Sriram Ramasubramanian [:sriram]]

startActitivyForResult is needed so that whatever user types in the AwesomeBar gets loaded. startActivity does not get the value from AwesomeBar. The whole purpose of faking the user to type the url when gecko is loading would fail if we do that.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Refactored the code as requested.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed https://hg.mozilla.org/projects/birch/rev/4c926c4ac01b