Closed Bug 468880 Opened 17 years ago Closed 16 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.
Status: NEW → RESOLVED
Closed: 16 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.

Attachment

General

Created:
Updated:
Size: