this is about using the new API from bug 515932

I changed all the addTab and loadOneTab callers I could find. There were a few instances of callers just passing a URI with default (null/false) parameters for the rest which I stripped out.

If you don't set inBackground to false new tabs aren't made active. I incorrectly assumed that leaving this parameter out made it default to the expected behaviour.

Comment on attachment 401181 [details] [diff] [review] Set inBackground to false explicitely Looks good, thanks. >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -1794,17 +1794,17 @@ function BrowserOpenTab() > function BrowserOpenTab() > { > if (!gBrowser) { > // If there are no open browser windows, open a new one > window.openDialog("chrome://browser/content/", "_blank", > "chrome,all,dialog=no", "about:blank"); > return; > } >- gBrowser.loadOneTab("about:blank", null, null, null, false, false); >+ gBrowser.loadOneTab("about:blank",{inBackground: false}); nit: add a space after ,

Fixed coding style inconsistency.

Verified fixed on trunk based on code check-in.

Comment on attachment 401184 [details] [diff] [review] For check in Safe on its own, baking for most of a month and, while not exactly blocking bug 514327 (a P1 blocker), makes it substantially easier to land. a=me.

Comment on attachment 401184 [details] [diff] [review] For check in >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js [SNIP] > // If this is an external load, we need to load a blank tab first, > // because loadflags can't be passed to loadOneTab. > let loadBlankFirst = !aURI || isExternal; >- let tab = win.gBrowser.loadOneTab(loadBlankFirst ? "about:blank" : aURI.spec, >- referrer, null, null, loadInBackground, false); >+ let tab = win.gBrowser.loadOneTab(loadBlankFirst ? "about:blank" : aURI.spec, { >+ referrerURI: referrer, >+ inBackground: loadInBackground}); Looks like this chunk of this bug's patch didn't land on 1.9.2, as can be seen in the push links above. (the 1.9.2 one is missing this chunk) Alternately, compare the current up-to-date chunks of source: http://hg.mozilla.org/mozilla-central/annotate/46484930f4f3/browser/base/content/browser.js#l4556 vs http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/14fd499129bf/browser/base/content/browser.js#l4497 Is that a problem?

For convenience, here's a patch to apply the chunk that got left out of 1.9.2, as noted in comment 9. This is a subset of the already-posted patch (attachment 401184 [details] [diff] [review]), and it applies cleanly to current mozilla-1.9.2. (and un-applies cleanly on mozilla-central, with "patch -R") I'm not claiming to know that this chunk is appropriate for 1.9.2 -- I'm just assuming it is, based on the fact that it was in the original patch posted here. I leave any further decisions/action here up to John Morkel & Dao. (patch author & patch reviewer/lander)

Comment on attachment 427249 [details] [diff] [review] chunk that got left out of 1.9.2 Not sure how this got left out. Dao, any ideas?

Comment on attachment 427249 [details] [diff] [review] chunk that got left out of 1.9.2 It got left out because it didn't apply on 1.9.2 at that time. There's no point in landing it now.