595839 - Unknown protocol warning dialog appears more than once

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sudarsana Nagineni]

Sources:
http://hg.mozilla.org/mozilla-central/rev/508d0a50827c
http://hg.mozilla.org/mobile-browser/rev/467096c3ad41

pref browser.tabs.remote=true

In current trunk build, Unknown protocol warning dialog appears more than once, when unknown protocol is loaded.

Steps to reproduce:
1. Open Fennec
2. Type bad:// in the url bar and press enter

Result: 
The protocol warning dialog appears three times.

Reproducible: 
Always

Comment 1

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

Attachment: Patch

One of the prompt come from bug 594845, the other is because we're loading the URL twice for setting the remote attribute correctly.

My initial plan was first to remove setting the "src" attribute into the createBrowser method but this method is called into the Tab constructor, itself called by addTab which is used in many places in the source code. This leads me to think that the current behavior is good now, and so I've decided to add a new parameter to addTab to be able to specify the loading parameters and get rid of the need to call tab.browser.stop into the loadURI method.

Comment 2

[Matt Brubeck (:mbrubeck)]

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

This looks good to me.  I've tried out the patch and can't find any problems.  I'd like to get Mark's opinion on this change too.

>-function Tab(aURI) {
>+function Tab(aURI, aParams) {

Please add a comment for this function explaining what aParams may contain.

>+  create: function create(aURI, aParams) {
>     // Initialize a viewport state for BrowserView
>     this._browserViewportState = BrowserView.Util.createBrowserViewportState();
> 
>     this._chromeTab = document.getElementById("tabs").addTab();
>+    this._createBrowser(aURI, aParams);
>   },

Instead of threading this new argument through yet another call, let's combine create() and _createBrowser() into one function (especially since "create" is even shorter in mobile2).  If we get rid of _createBrowser, be sure to remove the references to it from various comments in browser.js.

Comment 3

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

Attachment: Patch v0.2

Addressed comments

Comment 4

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

http://hg.mozilla.org/mobile-browser/rev/13e0611f94a1

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Forgot to put the verified on notice:

verified FIXED on build:
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100916 Firefox/4.0b7pre Fennec/2.0b1pre