Closed
Bug 468880
Opened 16 years ago
Closed 15 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•16 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•16 years ago
|
||
yes: https://wiki.mozilla.org/Mobile/Fennec_Automation
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•15 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•15 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•15 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•15 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•15 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•15 years ago
|
||
this change helps avoid loading about:blank
Comment 12•15 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•15 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•15 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•15 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•15 years ago
|
||
http://hg.mozilla.org/mobile-browser/rev/953171c26a87
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 17•15 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
•