700153 - opening page in new tab opens it in background, but url bar contents in foreground

Status: VERIFIED FIXED

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

Description

[Aakash Desai [:aakashd]]

build: 11/6 birch build

steps to reproduce:
1. go to sports.yahoo.com/nfl/scoreboard
2. long tap on any link
3. select "open page in new tab".

actual results:
the page loads in the background, but the contents of the url bar is updated to the psge title and favicon for the new loaded page.

expected results:
the page loads in the foreground.

Comment 1

[Wesley Johnston (:wesj)]

I'll take this since it was my fault.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This adds a parameter to our javascript addTab method, which takes an object which may have a "selected" parameter. The selected value gets passed to Java, and that in turn lets us decide whether or not to select the new tab.

Comment 3

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

Comment on attachment 572593 [details] [diff] [review]
Patch v1

># HG changeset patch
># Parent cd32f4fce7ef765b41607f4f7ddfe056fcaeb2fe
>
>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -770,7 +770,8 @@
>                 Log.i(LOG_NAME, "Created a new tab");
>                 int tabId = message.getInt("tabID");
>                 String uri = message.getString("uri");
>-                handleAddTab(tabId, uri);
>+                Boolean selected = message.getBoolean("selected");

We used "bringToFront" in XUL Fennec. I guess "selected" is ok, just seems odd at first read. Maybe it will grow on me.

>     void handleAddTab(final int tabId, final String uri) {
>+        handleAddTab(tabId, uri, true);
>+    }

Why keep this one at all? Remove please. I didn't see any other callers.

>+
>+    void handleAddTab(final int tabId, final String uri, final boolean selected) {
>         Tab tab = Tabs.getInstance().addTab(tabId, uri);
>+        if (selected) {
>+            Tabs.getInstance().selectTab(tabId);
>+        }
>         mMainHandler.post(new Runnable() { 

Add blank line

>             public void run() {
>-                onTabsChanged();
>                 mBrowserToolbar.updateTabs(Tabs.getInstance().getCount());
>-                mDoorHanger.updateForTab(tabId);
>+                if (selected) {

This is not good enough. By the time the runnable is called, the current tab may not be the selected tab anymore. Use something like

  if (selected && Tabs.getInstance().isSelectedTab(tab)) {

(and make 'tab' final so it can be used in the runnable)

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js


>-                 BrowserApp.addTab(url);
>+                 BrowserApp.addTab(url, {selected: false});

                   BrowserApp.addTab(url, { selected: false });

>       gecko: {
>         type: "Tab:Added",
>         tabID: this.id,
>-        uri: aURL
>+        uri: aURL,
>+        selected: aParams ? aParams.selected : true

This is a bit fragile. We should also check to see if | "selected" in aParams |

Maybe do | let aParams = aParams | { selected: true }; |  above this

and then 

          selected: ("selected" in aParams) ? aParams.selected : true

?


r+ but fix the nits

>       }
>     };
>     sendMessageToJava(message);

Comment 4

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/projects/birch/rev/cde2fa77ea5d

Comment 5

[Wesley Johnston (:wesj)]

These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Comment 7

[Aaron Train [:aaronmt]]

20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)