594827 - Page not shown when trying to open from Bookmarks or History

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ayan Shah[:ashah]]

Build: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b6pre)Gecko/20100909
Firefox/4.0b6pre Fennec /2.0b1pre

Steps:
1. Open fennec
2. Tap on the awesome bar
3. Tap on the Bookmarks button
4. Tap on any available bookmark

Actual result:
It is shown on the URL bar that the URL is loading(spinning wheel) but the page is not displayed. The user remains on the Bookmarks window.

Expected result:
The bookmarked page should be shown to the user. 

Note:
If you press the back arrow, then it takes you to the webpage you just clicked. This means that the page does load but it is not shown to the user, because the bookmarks window does not go away. 


Same is the case with pages in History.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This is a regression from bug 594426 I've landed this morning.

I'm kind of tired of these regressions (which are completely my bad) and so I guess I need to do more tests.

Comment 3

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

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

more tests would be good! especially with all the new awesomebar behavior

Should we just set BrowserUI.activePanel = null in BrowserUI.goToURI? It seems like a lot of trouble to remember to close the awesomescreen in lots of places. Can we centralize it any better?

Comment 4

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

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

r+ for this, but it could become a bigger problem.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> Comment on attachment 473616 [details] [diff] [review]
> Patch
> 
> more tests would be good! especially with all the new awesomebar behavior
> 
> Should we just set BrowserUI.activePanel = null in BrowserUI.goToURI? It seems
> like a lot of trouble to remember to close the awesomescreen in lots of places.
> Can we centralize it any better?

In theory we are not supposed to add it everywhere, the activePanel state should be reset when we close the autocomplete panel which is called into goToURI (closeAutocomplete set activePanel to null) but there is probably a race condition here that turn the popup state to close before we're calling closeAutocomplete which leads in returning early because of the isAutocompleteOpen check.

I will land the patch like that but I would like to do a better system for that. 
I remember having doubt when I have introduced the "activePanel" setter into browserUI since we already have a "dialog" system. I would like to find a nice way to merge them and clean up all this regression prone code.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/1162af72b8ff

I've open bug 595118 for adding more tests.

Comment 8

[Ayan Shah[:ashah]]

Verified on 

Build:
Mozilla/5.0(Android;Linux armv7l;rv2.0b7pre)Gecko/20100916/4.0b7pre
Fennec/2.0b1pre

AND

Mozilla/5.0(Maemo;Linux armv7l;rv2.0b7pre)Gecko/20100916/4.0b7pre
Fennec/2.0b1pre

Comment 9

[Dão Gottwald [:dao]]

bugspam