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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mfinkle, Assigned: mfinkle)
References
Details
Attachments
(2 files, 2 obsolete files)
24.42 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
619 bytes,
patch
|
Details | Diff | Splinter Review |
There are plenty of situations where adding a new tab and loading it with content is needed. Also, it should allow background loads.
Assignee | ||
Comment 1•17 years ago
|
||
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?
Comment 3•17 years ago
|
||
Comment 4•16 years ago
|
||
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
Assignee | ||
Comment 6•16 years ago
|
||
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 7•16 years ago
|
||
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?
Assignee | ||
Comment 8•16 years ago
|
||
(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
Assignee | ||
Comment 9•16 years ago
|
||
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)
Assignee | ||
Comment 10•16 years ago
|
||
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)
Comment 11•16 years ago
|
||
this change helps avoid loading about:blank
Comment 12•16 years ago
|
||
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 13•16 years ago
|
||
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
Assignee | ||
Comment 14•16 years ago
|
||
(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.
Assignee | ||
Comment 15•16 years ago
|
||
(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.
Assignee | ||
Comment 16•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 17•16 years ago
|
||
(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.
Description
•