729463 - Implement switch-to-tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Peter Retzer (:pretzer)]

Fennec should list currently loaded URIs in autocomplete results on awesome screen and allow switching to the tab, just like desktop Firefox (bug 480350).

Comment 1

[Peter Retzer (:pretzer)]

Attachment: Mockup (awesome bar autocomplete list)

I created two quick mockups of how this feature could look like, one for the autocomplete list on the awesome bar, and one for the top sites list. 
This is obviously not pixel-perfect and just a proposal based on the desktop Firefox implementation, which is intended to help evaluate this feature for mobile.

I created this for the UX team to comment on, wether this is wanted or not. 
The correct way would be, to request UI-Review from the UX-Account, I think, but I don't seem to have the rights to do this. So I just requested feedback from Madhava, I hope that's okay.

Comment 2

[Peter Retzer (:pretzer)]

Attachment: Mockup (top sites)

Comment 3

[Peter Retzer (:pretzer)]

Comment on attachment 600664 [details]
Mockup (awesome bar autocomplete list)

Requesting feedback from Ian, since Madhava is at MWC

Comment 4

[Ian Barlow (:ibarlow)]

Hi pretzer, thanks for mocking this up! We do want this as a feature, but we're still iterating on the design of it. 

Your mockup is indeed consistent with desktop today, but we're looking at tweaking the designa bit across mobile and desktop. I'd like to wait on implementing this until we settle on that design.

Comment 5

[Peter Retzer (:pretzer)]

Thanks for the response, it's good to hear UX is already working on this! We can leave this open until the design is final.

Comment 6

[Oliver Henshaw]

Could this be extended to include remote tabs, or is that worthy of a separate bug?

Comment 7

[Oliver Henshaw]

(In reply to Oliver Henshaw from comment #6)
> Could this be extended to include remote tabs, or is that worthy of a
> separate bug?

Bug 776881 so that this doesn't get lost.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This implements this in a pretty simple fashion. Just changes the title text if Tabs.getInstance().isOpen() (which I think I'll rename to findForUrl()?). I also refactored some code to be a little more shareable so that this works across all our awesomebar screens. Made a default mdpi image (that gets used on all resolutions), but need some better ones.

Build at:
http://people.mozilla.com/~wjohnston/switchToTab.apk

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Screnshot

Screenshot. Ian, mfinkle and I were talking about whether we should add back "Open in new tab" here as a context menu action, or whether we can just.. not implement that. Ideas?

Comment 10

[Ian Barlow (:ibarlow)]

Sorry, not sure I follow you... switch to tab is a bit of a different use case than wanting to open something fresh in a new tab. Can you clarify a bit?

Comment 11

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

(In reply to Ian Barlow (:ibarlow) from comment #10)
> Sorry, not sure I follow you... switch to tab is a bit of a different use
> case than wanting to open something fresh in a new tab. Can you clarify a
> bit?

Desktop gives you an option (CTRL+Click) to open the URL in a new tab even if it is already open in an existing tab. On Android, we don't have the CTRL+Click modifier to allow this. So, if you really wanted to open the URL in a new tab, you can't. Tapping will just switch to the existing tab.

Comment 12

[Aaron Train [:aaronmt]]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Created attachment 733130 [details]
> Screnshot
> 
> Screenshot. Ian, mfinkle and I were talking about whether we should add back
> "Open in new tab" here as a context menu action, or whether we can just..
> not implement that. Ideas?


I'd love to have that functionality back. I think the steps required currently to get back to bookmarks and back the listing (I have lots of bookmarks) currently requires many steps when all I want to do is open a few in a row in as few steps as possible. Currently Chrome keeps the bookmark listing open on tab open and offers a context menu to open in either private or regular tabs.

Comment 13

[Ian Barlow (:ibarlow)]

(In reply to Aaron Train [:aaronmt] from comment #12)

> I'd love to have that functionality back. I think the steps required
> currently to get back to bookmarks and back the listing (I have lots of
> bookmarks) currently requires many steps when all I want to do is open a few
> in a row in as few steps as possible. Currently Chrome keeps the bookmark
> listing open on tab open and offers a context menu to open in either private
> or regular tabs.

Arg. I don't know. I actually do seeing that as a useful feature, particularly when we redo about:home... Being able to throw a bunch of my favorite sites open in a list of tabs would be nice. 

I guess we can add it back.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Implements switch to tab. I added a static HashMap that we lazily create to make searches a bit faster. New images from ian as well.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Open in New Tab

This adds back Open in New Tab. It shows New Tab if you're in normal mode "Open in Private Tab" if you're in private mode. They're only shown on entries that are switch to tab.

Comment 16

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

Comment on attachment 734046 [details] [diff] [review]
Patch 1/2

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>     public interface OnUrlOpenListener {
>         public void onUrlOpen(String url, String title);
>         public void onSearch(String engine, String text);
>         public void onEditSuggestion(String suggestion);
>+        public void switchToTab(final int tabId);

OCD: We use the onXxx pattern for the rest of the methods. It's arguable that we should not, but until we change them all, could you use "onSwitchToTab" for consistency.

>diff --git a/mobile/android/base/awesomebar/AwesomeBarTab.java b/mobile/android/base/awesomebar/AwesomeBarTab.java
>+    public static HashMap<String, Integer> sOpenTabs;

Does this need to be static? You clear it in destroy, so I am wondering what the benefit is.

r+ with the nits fixed and removing the static if you can.

Comment 17

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

Comment on attachment 734047 [details] [diff] [review]
Patch 2/2 - Open in New Tab

>diff --git a/mobile/android/base/awesomebar/AwesomeBarTab.java b/mobile/android/base/awesomebar/AwesomeBarTab.java

>+    protected void setupMenu(ContextMenu menu, AwesomeBar.ContextMenuSubject subject) {

>+        boolean isOpen = getOpenTabs().get(subject.url) != null;
>+        boolean isPrivate = false;
>+        Tab tab = Tabs.getInstance().getSelectedTab();
>+        if (tab != null) {
>+            isPrivate = tab.isPrivate();
>+        }

Maybe I am over thinking it, but you could skip getting the tab and checking it's private state if isOpen is false.

Also, we need to get Ian to OK only showing the menu for "Switch to tab" rows. He might just want to add them back to all rows. I know Aaron does :)

Comment 18

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #17)

> 
> Also, we need to get Ian to OK only showing the menu for "Switch to tab"
> rows. He might just want to add them back to all rows. I know Aaron does :)

If we're going to add it, it might as well be everywhere. There's no value that I can see to only showing it for already open tabs.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This shows the "Open Tab" for whatever mode you're in on all rows.

Comment 20

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

Comment on attachment 734831 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/awesomebar/AwesomeBarTab.java b/mobile/android/base/awesomebar/AwesomeBarTab.java

>-    private HashMap<String, Integer> getOpenTabs() {
>+    protected HashMap<String, Integer> getOpenTabs() {

Still needed?

>diff --git a/mobile/android/base/widget/TopSitesView.java b/mobile/android/base/widget/TopSitesView.java

Revert all the changes in this file

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48da1752622f

Comment 22

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d901d33dd142

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/180eaa7f6519

https://tbpl.mozilla.org/php/getParsedLog.php?id=21572609&tree=Mozilla-Inbound

22 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
Exception caught during test!
junit.framework.AssertionFailedError: 22 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec.FennecNativeActions$GeckoEventExpecter.blockForEvent(FennecNativeActions.java:135)
	at org.mozilla.fennec.FennecNativeActions$GeckoEventExpecter.blockForEvent(FennecNativeActions.java:118)
	at org.mozilla.fennec.tests.testAllPagesTab.testClick(testAllPagesTab.java:144)
	at org.mozilla.fennec.tests.testAllPagesTab.testAllPagesTab(testAllPagesTab.java:47)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:125)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
23 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | Exception caught - junit.framework.AssertionFailedError: 22 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
24 INFO TEST-END | testAllPagesTab | finished in 125393ms
25 INFO TEST-START | Shutdown

0 INFO SimpleTest START
1 INFO TEST-START | testHistoryTab
2 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_big_link.html should equal http://mochi.test:8888/tests/robocop/robocop_big_link.html
3 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_blank_01.html should equal http://mochi.test:8888/tests/robocop/robocop_blank_01.html
waitForText timeout on Settings
4 INFO TEST-PASS | testHistoryTab | bookmark added successfully - true should equal true
5 INFO TEST-PASS | testHistoryTab | checking that history list exists and has 3 children - android.widget.ExpandableListView@488d0ef8 should not equal null
6 INFO TEST-PASS | testHistoryTab | TextView is filled in - Big Link
7 INFO TEST-PASS | testHistoryTab | TextView is filled in - http://mochi.test:8888/tests/robocop/robocop_big_link.html
8 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 1 should equal 1
9 INFO TEST-PASS | testHistoryTab | TextView is filled in - Browser Blank Page 01
10 INFO TEST-PASS | testHistoryTab | TextView is filled in - Switch to tab
11 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Correct number of ImageViews visible - got 2, expected 1
Exception caught during test!
junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Correct number of ImageViews visible - got 2, expected 1
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec.FennecMochitestAssert.is(FennecMochitestAssert.java:142)
	at org.mozilla.fennec.tests.testHistoryTab.testList(testHistoryTab.java:143)
	at org.mozilla.fennec.tests.testHistoryTab.testHistoryTab(testHistoryTab.java:87)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:125)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
12 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Correct number of ImageViews visible - got 2, expected 1
13 INFO TEST-END | testHistoryTab | finished in 54099ms
14 INFO TEST-START | Shutdown

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Tests Patch v1

This fixes some tests. Also helps with bug 817586 (locally). I fixed up some wiatForText calls to not expect things in the UI that aren't necessarily there any more (url's replaces with switch-to-tab), fixed clicking on rows that now switch to tab rather than opening. I also ran into the problem again where we are trying to tap on the awesomebar before the awesomescreen has finished animating. I added a big LaunchAwesomeBarTest (BooleanTest) to try and fix that, but it doesn't. Right now that's fixed with a mSolo.sleep(2000); that I hate.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d73659623654

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 735479 [details] [diff] [review]
Tests Patch v1

Waiting for try, but if you wanted to look at this first geoff, I'd appreciate.

Comment 26

[Geoff Brown [:gbrown]]

Comment on attachment 735479 [details] [diff] [review]
Tests Patch v1

Review of attachment 735479 [details] [diff] [review]:
-----------------------------------------------------------------

This sort of change (Switch to Tab) is bound to cause lots of trouble for the UI tests -- you have my sympathy!

I don't understand some of the changes here...hopefully we just need to discuss.

::: mobile/android/base/tests/BaseTest.java.in
@@ +186,5 @@
>      /**
>       * Click on the awesome bar element and return the resulting activity.
>       * @return The created activity, or null if the awesome bar cannot be clicked.
>       */
>      protected final Activity clickOnAwesomeBar() {

I haven't noticed much trouble with clickOnAwesomeBar, and it is used quite often...what kind of trouble did you run into here?

@@ +407,5 @@
> +
> +        @Override
> +        public boolean test() {
> +            // If we're returning from the awesomebar, there are animations running that may prevent opening the awesomescreen again
> +            mSolo.sleep(2000);

If the problem is code like:
clickOnAwesomeBar
sendKey(BACK)
clickOnAwesomeBar

I think I would prefer to handle that in the test code:
clickOnAwesomeBar
sendKey(BACK)
sleep(...)
clickOnAwesomeBar

Also 2 s seems too long: People can tap faster than that.

@@ +640,5 @@
>  
>          // If the OS is not Gingerbread the vkb will be opened so we need to close it in order to press the "Cancel" button
>          if (!(mDevice.version.equals("2.x"))) {
> +           // mSolo.hideSoftKeyboard(); // not shipping this robotium yet
> +           InputMethodManager inputMethodManager = (InputMethodManager) mActivity.getSystemService(Activity.INPUT_METHOD_SERVICE);

Does BACK no longer work? That seems unrelated to Switch to Tab.

@@ +648,4 @@
>          }
>  
>          // Check if the new text was added
> +        mSolo.sleep(2000);

Why? Again, this seems excessive.

@@ +652,3 @@
>          if (mSolo.searchText(addedText)) {
>              mSolo.clickOnText("Cancel");
> +            waitForText("Bookmarks");

I think the intent here was to wait for the list to be populated, not just the tab. Is there some list item text that we can wait for still?

::: mobile/android/base/tests/testAllPagesTab.java.in
@@ +129,5 @@
>          ListView list = getAllPagesList(bookmarks[0], 5);
>  
>          Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
> +
> +        // Avoid clicking the first item. Its the first item because its in an open tab, and will just switch to it

nit: it's or it is (x2)

::: mobile/android/base/tests/testHistoryTab.java.in
@@ +218,5 @@
>          mFirstChild = null;
>          boolean success = waitForTest(new BooleanTest() {
>              @Override
>              public boolean test() {
> +                // Avoid clicking on the first entry, which is already open and will just switch to the tab

Good comments throughout - thanks!

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Test fix

Sorry for the delay. This fixes the issues I was seeing. Looks good on try (except Android 4.0, which seems to just constantly randomly fail?)

Comment 28

[Geoff Brown [:gbrown]]

Comment on attachment 737572 [details] [diff] [review]
Test fix

Review of attachment 737572 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good now - thanks. 

I checked your try run at https://tbpl.mozilla.org/?tree=Try&rev=24bc71d69d04 and re-triggered some tests. The Panda results are troubling. The panda rc suite is unreliable currently (sorry about that -- working on it!) but there are specific tests that fail intermittently. I have not previously seen the consistent testAllPagesTab and testHistoryTab failures seen in your try runs. Also, I cannot find any outstanding reports of the one failure in Android 2.2 testBookmark. 

Reserving r+ until we get a reliable try run.

Ping me if you would like some help tracking down the cause(s) of the test failures here.

::: mobile/android/base/tests/testAllPagesTab.java.in
@@ +43,5 @@
>          String url = getAbsoluteUrl("/robocop/robocop_big_link.html");
>          loadUrl(url);
>  
>          testList(url);
> +        mSolo.sleep(500);

If the need for these pauses is understood, add a comment explaining; if not, add a TODO comment calling out need for investigation.

Comment 29

[Wesley Johnston (:wesj)]

Still fighting pandas here:

https://tbpl.mozilla.org/?tree=Try&rev=461d78b62c3b

I changed the fix to try a little harder to open the awesomescreen. That seems to have made things happy without the sleeps. Its mostly these verifyUrl tests that are killing me now. They go through the same code path, but seem to still have problems (on panda's, pass fine locally).

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Test fixes

This is a bit more complex. I'm trying to make clickOnAwesomeBar actually check that the activity is visible before returning. I wrote a test to do that that clicks it and waits for the text "History". That fixes most of my problems except for those using verifyUrl. For some reason I need a bigger sleep there?

Seems good on try:
https://tbpl.mozilla.org/?tree=Try&rev=7cf3687c1a59

Comment 31

[Geoff Brown [:gbrown]]

Comment on attachment 746697 [details] [diff] [review]
Test fixes

Review of attachment 746697 [details] [diff] [review]:
-----------------------------------------------------------------

Your most recent try run looks good to me.

::: mobile/android/base/AwesomeBar.java
@@ +34,5 @@
>  import android.view.ContextMenu.ContextMenuInfo;
>  import android.view.KeyEvent;
>  import android.view.LayoutInflater;
>  import android.view.MenuItem;
> +import android.view.MenuInflater;

What's this doing here?

::: mobile/android/base/tests/BaseTest.java.in
@@ +641,5 @@
>          mSolo.clickOnText("Edit");
>          waitForText("Edit Bookmark");
>  
>          // If the OS is not Gingerbread the vkb will be opened so we need to close it in order to press the "Cancel" button
> +        // XXX: This is not true on all devices, but is true on our current automation test machines...

Do you know what the test should be?

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Favicons fix

Realized as I was landing, because it does it on a delay for only visible rows, the top sites page is using the TextView to determine what favicon to show. That means it breaks here because we set that row to show "Switch to Tab".

This fixes it to show the correct favicon by saving the url in the ViewHolder itself.

Comment 33

[:Margaret Leibovic]

Comment on attachment 747528 [details] [diff] [review]
Favicons fix

Review of attachment 747528 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +938,5 @@
> +
> +        Cursor cursor = adapter.getCursor();
> +        if (cursor == null)
> +            return;
> +

Why do we need these new null checks?

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ +60,5 @@
>      }
>  
>      protected class AwesomeEntryViewHolder {
>          public TextView titleView;
> +        public String url;

It seems odd for AwesomeEntryViewHolder to hold a string, since it looks like it was designed to just be holding views, but this doesn't really hurt anything, and we can revisit this during the about:home rewrite.

@@ +175,2 @@
>      }
> +    

Nit: whitespace.

@@ +175,3 @@
>      }
> +    
> +    protected void updateUrl(AwesomeEntryViewHolder holder, String url) {

I always feel like overloaded methods are more confusing than useful, but I suppose I'm not reviewing that change here...

::: mobile/android/base/awesomebar/HistoryTab.java
@@ +185,5 @@
>              String title = (String) historyItem.get(URLColumns.TITLE);
>              String url = (String) historyItem.get(URLColumns.URL);
>  
>              updateTitle(viewHolder.titleView, title, url);
> +            updateUrl(viewHolder, url);

So holder.url won't get updated from here... is that okay?

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Favicon fix

I'm not sure if you wanted me to fix things or were just asking questions? I addressed some?

(In reply to :Margaret Leibovic from comment #33)
> Why do we need these new null checks?

Actually, not for the adapter. Its possible for adapter.getCursor() to return null, so I think we should keep that check in here (this code is pretty heavily threaded... and I don't trust the cursor to always be around)

> It seems odd for AwesomeEntryViewHolder to hold a string, since it looks
> like it was designed to just be holding views, but this doesn't really hurt
> anything, and we can revisit this during the about:home rewrite.

Yeah. I didn't love it either. But the other solutions seem pretty ugly too. These views should probably just be real Views and we can kill this whole viewholder pattern.

> So holder.url won't get updated from here... is that okay?

Yes, but good idea. I moved this into the second function.

Comment 35

[:Margaret Leibovic]

Comment on attachment 747622 [details] [diff] [review]
Favicon fix

I just wanted answers to my questions before deciding whether to r+/r-. I guess I could have just given it an r-, since all patches are r- before being proven r+ :)

Comment 36

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e358c97f4294
https://hg.mozilla.org/integration/mozilla-inbound/rev/d437af6786eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7de4b81d211
https://hg.mozilla.org/integration/mozilla-inbound/rev/2834210d62bf

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2834210d62bf
https://hg.mozilla.org/mozilla-central/rev/b7de4b81d211
https://hg.mozilla.org/mozilla-central/rev/d437af6786eb
https://hg.mozilla.org/mozilla-central/rev/e358c97f4294

Comment 38

[u421692]

Test Case added in Moztrap:
https://moztrap.mozilla.org/manage/case/8083/

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/mobile/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 40

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 734046 [details] [diff] [review]
Patch 1/2

I have problems with this patch!
1. Why is awesomebar holding a HashMap for the open-tabs? Should it be the responsibility of Tabs to expose a "findTabWithUrl()" method and hold the hashmap?
2. Why is sOpenTabs a static variable? Once the first set of tabs is cached by awesomebar, when new tabs are opened, they won't be updated by Awesomebar until everything is GC-ed. Note: this is a static variable!

Comment 41

[Wesley Johnston (:wesj)]

1.) We can. I don't think anyone else cared, so it didn't seem great to maintain a full-time hashmap of urls in memory that we don't always need. i.e. this is an optimization to make url look up faster for this one use case.

2.) The static variable isn't held in AwesomeBar. Its held in the AwesomebarTab object, and is destroyed when the tabs are destroyed (i.e. when the ViewPager is destroyed).

Comment 42

[Gabriela [:gaby2300]]

I and cuantocarlos(a Mozilla Hispano QA contributor) can confirm the bug is fixed using me a Samsung Galaxy S2 phone and he a Samsung Galaxy S3 phone, both with the latest beta.