Closed Bug 468880 Opened 14 years ago Closed 14 years ago

Change Browser.newTab to take a URI

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(2 files, 2 obsolete files)

There are plenty of situations where adding a new tab and loading it with content is needed. Also, it should allow background loads.
Attached patch patch (obsolete) — Splinter Review
Something like this. I want to wait until we have chrome mochitests before asking for review and landing.
Assignee: nobody → mark.finkle
Joel, didn't you get chrome mochitests running at one point or am I confused?
Mark's referring to Fennec-specific browser chrome test infra (with appropriate changes to our build system to stage the tests properly).
Ah I see.  yeah, those would require a bit of fennec specific tweaks
Blocks: 468739
Comment on attachment 352367 [details] [diff] [review]
patch

Fennec mochitests are taking longer than I want to wait for this patch. It is also potentially blocking bug 468739.
Attachment #352367 - Flags: review?(gavin.sharp)
Comment on attachment 352367 [details] [diff] [review]
patch

Don't you need to update the existing callers? Should we take this opportunity to move to using the tabbrowseresque addTab/selectedTab API?
(In reply to comment #7)
> (From update of attachment 352367 [details] [diff] [review])
> Don't you need to update the existing callers?

Whoops

> Should we take this opportunity
> to move to using the tabbrowseresque addTab/selectedTab API?

Yeah - and selectedBrowser

new patch coming up
Attached patch patch 2 - addresses comments (obsolete) — Splinter Review
Same as the first patch but:

* updates callers
* renames newTab -> addTab, selectTab -> selectedTab (property), currentTab -> (removed) & currentBrowser -> selectedBrowser

During startup we load "about:firstrun" (which should avoid going into edit mode), but we likely loadURI it again lower in the startup function. Not great, but that logic needs some love in a new bug.
Attachment #352367 - Attachment is obsolete: true
Attachment #355612 - Flags: review?(gavin.sharp)
Attachment #352367 - Flags: review?(gavin.sharp)
Builds on the last patch. Adds some code to stop executing other code during startup.

* adds firstTab check in Browser.selectedTab setter to skip the onlocationchange and onsecuritychange calls on the first tab.
Attachment #355612 - Attachment is obsolete: true
Attachment #355847 - Flags: review?(gavin.sharp)
Attachment #355612 - Flags: review?(gavin.sharp)
this change helps avoid loading about:blank
Blocks: 472745
Comment on attachment 355847 [details] [diff] [review]
patch 3 - reduce the "runs twice" issues

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

>+      if (whereURI)
>+        this.addTab(whereURI, true);

What happens if whereURI is null?

>+  get selectedBrowser() {
>+    return this._selectedTab.browser;

Similarly, what happens if _selectedTab is null when someone calls this?

Sounds like we should have a tab in place by default (like tabbrowser), to avoid having to worry about these corner cases. Might even be faster than the common case of always having to create one.

>-  newTab: function(bringFront) {
>+  addTab: function(uri, bringFront) {

We need to eventually change addTab to have the same signature as tabbrowser's, but we can do that separately I guess.

>+    if (!findbar.browser) {
>+      findbar.browser = this.selectedBrowser;
>     }

nit: remove the brackets

>-    currentBrowser = Browser.currentBrowser;

>+    let selectedBrowser = Browser.selectedBrowser;

This code is copied, iirc, so might be worth keeping the variable the name the same.

(Also reminds me that I have a patch from a couple of years ago rewriting this to do less dumb string manipulation that I never landed...)

>+  _createBrowser: function(uri) {

>+    browser.setAttribute("src", uri);

>+/*
>+    if (uri != "about:blank") {
>+      browser.stop();
>+      browser.loadURI(uri);
>+    }
>+*/

Why is this commented out? tabbrowser does it this way...

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>           <textbox id="urlbar-edit"

>+                   readonly="true"

Unrelated?
Attachment #355847 - Flags: review?(gavin.sharp) → review+
Comment on attachment 355847 [details] [diff] [review]
patch 3 - reduce the "runs twice" issues

;
>     document.getElementById("tabs").selectedItem = tab.content;
> 
>-    ws.panTo(0,0, true);
>+    if (!firstTab) {

moving the .selectedItem under (!firstTab) should give us another 30ms speedup
(In reply to comment #12)
> (From update of attachment 355847 [details] [diff] [review])
> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> 
> >+      if (whereURI)
> >+        this.addTab(whereURI, true);
> 
> What happens if whereURI is null?

We could revert back to the homepage. Currently, we'd be in a bad state.

> 
> >+  get selectedBrowser() {
> >+    return this._selectedTab.browser;
> 
> Similarly, what happens if _selectedTab is null when someone calls this?

Boom

> 
> Sounds like we should have a tab in place by default (like tabbrowser), to
> avoid having to worry about these corner cases. Might even be faster than the
> common case of always having to create one.

That's possible, as long as it's not a perf drag.

> 
> >-  newTab: function(bringFront) {
> >+  addTab: function(uri, bringFront) {
> 
> We need to eventually change addTab to have the same signature as tabbrowser's,
> but we can do that separately I guess.
> 

Yeah, I'll file a bug for more tabbrowser-ism

> >+    if (!findbar.browser) {
> >+      findbar.browser = this.selectedBrowser;
> >     }
> 
> nit: remove the brackets

Done

> 
> >-    currentBrowser = Browser.currentBrowser;
> 
> >+    let selectedBrowser = Browser.selectedBrowser;
> 
> This code is copied, iirc, so might be worth keeping the variable the name the
> same.
> 

The code we copied it from uses "var selectedBrowser = gBrowser.selectedBrowser" so we are good.

> >+/*
> >+    if (uri != "about:blank") {
> >+      browser.stop();
> >+      browser.loadURI(uri);
> >+    }
> >+*/
> 
> Why is this commented out? tabbrowser does it this way...

I am setting the browser src attribute to the URI explicitly, not using a .loadURI like tabbrowser does. That way we don't need to stop an about:blank load to reset the src to something else.

> >           <textbox id="urlbar-edit"
> >+                   readonly="true"
> 
> Unrelated?

I put this in cause it is the default for the textbox. We start in the "caption" mode.


I am also going to make Taras' two suggestions.
(In reply to comment #13)
> (From update of attachment 355847 [details] [diff] [review])
> ;
> >     document.getElementById("tabs").selectedItem = tab.content;
> > 
> >-    ws.panTo(0,0, true);
> >+    if (!firstTab) {
> 
> moving the .selectedItem under (!firstTab) should give us another 30ms speedup

Actually, I didn't do this part. It breaks the CSS styling and we want to replace the richlistbox anyway. When we replace the richlistbox, this line should get faster.
http://hg.mozilla.org/mobile-browser/rev/953171c26a87
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #14)
> I am setting the browser src attribute to the URI explicitly, not using a
> .loadURI like tabbrowser does. That way we don't need to stop an about:blank
> load to reset the src to something else.

Yeah, I was just wondering if there was a good reason to differ from tabbrowser on this. Assuming that it had a good reason to do it that way might be foolish, though!
You need to log in before you can comment on or make changes to this bug.