468880 - Change Browser.newTab to take a URI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

There are plenty of situations where adding a new tab and loading it with content is needed. Also, it should allow background loads.

Comment 1

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

Attachment: patch

Something like this. I want to wait until we have chrome mochitests before asking for review and landing.

Comment 2

[cmtalbert]

Joel, didn't you get chrome mochitests running at one point or am I confused?

Comment 3

[Joel Maher ( :jmaher ) (UTC -0800)]

yes: https://wiki.mozilla.org/Mobile/Fennec_Automation

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Mark's referring to Fennec-specific browser chrome test infra (with appropriate changes to our build system to stage the tests properly).

Comment 5

[cmtalbert]

Ah I see.  yeah, those would require a bit of fennec specific tweaks

Comment 6

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

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.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 8

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

(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

Comment 9

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

Attachment: patch 2 - addresses comments

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.

Comment 10

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

Attachment: patch 3 - reduce the "runs twice" issues

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.

Comment 11

[(dormant account)]

Attachment: this should also go into the final patch

this change helps avoid loading about:blank

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 13

[(dormant account)]

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

Comment 14

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

(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.

Comment 15

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

(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.

Comment 16

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

http://hg.mozilla.org/mobile-browser/rev/953171c26a87

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(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!