Closed Bug 595839 Opened 9 years ago Closed 9 years ago

Unknown protocol warning dialog appears more than once

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: naginenis, Assigned: vingtetun)

References

()

Details

Attachments

(2 files)

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
Attached patch PatchSplinter Review
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.
Assignee: nobody → 21
Attachment #474828 - Flags: review?(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.
Attachment #474828 - Flags: review?(mbrubeck)
Attachment #474828 - Flags: review?(mark.finkle)
Attachment #474828 - Flags: review+
OS: Linux → All
Hardware: x86 → All
Attached patch Patch v0.2Splinter Review
Addressed comments
Attachment #475029 - Flags: review?(mark.finkle)
Attachment #475029 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/13e0611f94a1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.