Adding inBackground parameter to openLinkIn feels kind of wrong, but I don't really see an alternative that doesn't involve duplicating the chromehidden check, which also doesn't seem great.

Comment on attachment 488742 [details] [diff] [review] patch >- openUILinkIn(url, where, >- { allowThirdPartyFixup: true, postData: postData }); >+ >+ openLinkIn(url, where, >+ { allowThirdPartyFixup: true, >+ postData: postData, >+ fromContent: false, Use openUILinkIn and omit fromContent?

I didn't really want to extend the signature change to openUILinkIn, particularly since it uses explicit arguments...

(In reply to comment #1) > Created attachment 488742 [details] [diff] [review] > patch > > Adding inBackground parameter to openLinkIn feels kind of wrong, but I don't > really see an alternative that doesn't involve duplicating the chromehidden > check, which also doesn't seem great. Because the tab would otherwise open in the background with browser.tabs.loadBookmarksInBackground = true?

(In reply to comment #3) > I didn't really want to extend the signature change to openUILinkIn, > particularly since it uses explicit arguments... The third argument is just passed through, it's not bound to the explicit arguments.

(In reply to comment #4) > Because the tab would otherwise open in the background with > browser.tabs.loadBookmarksInBackground = true? Right.

I'm not sure that matters... browser.tabs.loadBookmarksInBackground is already overloaded. If we add inBackground, it seems that it would be more consistent to assign it directly to loadInBackground.

(In reply to comment #7) > I'm not sure that matters... browser.tabs.loadBookmarksInBackground is already > overloaded. I don't think it ever makes sense to have Alt+Enter load a tab in the background, and I don't really think we should introduce a behavior change just because the implementation is slightly bothersome. > If we add inBackground, it seems that it would be more consistent to assign it > directly to loadInBackground. I thought of that, but it felt wrong that we'd end up reversing the passed-in value in the tabshifted case...

But reversing the behavior when the user hits Shift is what "tabshifted" exists for: 'same as "tab" but in background if default is to select new tabs, and vice versa'. Explicitly passing "tabshifted" and setting inBackground would be confusing, but it wouldn't make sense to call the function that way in the first place. "tabshifted" would usually come from whereToOpenLink and be transparent to the caller. Maybe the parameter name should indicate that it specifies default behavior. I wouldn't mind it being lengthy since it should probably be omitted in favor of the prefs in most cases.

Comment on attachment 488742 [details] [diff] [review] patch Normal Enter had better call openUILinkIn(url, "current") instead of loadURI. This will fix the overriding current pinned tab problem.

Make the logic consistent between gURLBar.handleCommand and searchbar.handleSearchCommand.

(In reply to comment #1) > Adding inBackground parameter to openLinkIn feels kind of wrong, but I don't > really see an alternative that doesn't involve duplicating the chromehidden > check, which also doesn't seem great. Add 'tabbackground' and 'tabforeground' for where?

(In reply to comment #10) > Normal Enter had better call openUILinkIn(url, "current") instead of loadURI. > This will fix the overriding current pinned tab problem. That's a separate issue, and one that I don't want to get into in this bug.

(In reply to comment #6) > (In reply to comment #4) > > Because the tab would otherwise open in the background with > > browser.tabs.loadBookmarksInBackground = true? > > Right. Alt+Enter in the search bar involves browser.tabs.loadBookmarksInBackground preference, so it may be not a problem.

I think I probably should write a test for this.

We really need better test coverage here, but they are a pain to write... I found a few other edge cases that my previous patch hadn't taken into consideration. I think the only remaining behavior change introduced by this patch is unconditionally calling preventDefault/stopPropagation for Alt+Enter events (seems like we should do that even if the current tab is empty and we end up taking the "current" path).

Is there a good reason that this blocks betaN? If not, it should be moved over to final+.

If this gets reviewed before we generate RC, it has implicit a=beltzner

Is this just waiting for a test?

Yes.

** PRODUCT DRIVERS PLEASE NOTE ** This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons: - it was marked as a soft blocking issue without a requirement for beta coverage

So what's up with this bug? Is there a reason it's on hold or was it simply forgotten.

(In reply to khagaroth from comment #24) > So what's up with this bug? Is there a reason it's on hold or was it simply > forgotten. This bug was held up because we wanted to write tests before landing this patch. Unfortunately, there aren't any existing tests for these code paths, which makes this task more time consuming. I actually started working on some tests for this last week because this bug blocks bug 598587, which we really want for the new home tab feature.

Here are some tests I've been working on to test the existing location bar behavior (Iwithout Gavin's patch applied). I still need to flesh out the tests for save and new window behavior, but I wanted to get feedback to make sure these are on the right track. I figure once we get test coverage for the existing behavior, it will be much easier to add new tests for new behavior.

Comment on attachment 561382 [details] [diff] [review] wip tests I'm making this better - ignore this patch.

I had to change the patch quite a bit because of all the logic changes from bug 658383 and friends. Gavin, maybe you want to change your patch at this point, since most of the code around it has changed? If you do want to change the patch, perhaps you could be super awesome and also account for bug 598587 in the rewrite.

Comment on attachment 563594 [details] [diff] [review] location bar tests >diff --git a/browser/base/content/test/browser_locationBarCommand.js b/browser/base/content/test/browser_locationBarCommand.js >+ { desc: "Ctrl/Cmd+Return keypress", >+ event: { meta: true }, >+ check: checkCurrent This looks wrong - shouldn't that be accelKey, and checkNewTab? Looks good otherwise, though there is that issue with some other test interfering with this that needs to be sorted out.

Thanks a lot for writing those tests, btw :)

I'd like to remove the preventDefault/stopPropagation stuff marked by the XXX comment, but I don't have easy access to windows/linux machines to test at the moment. I have a patch on top of this for bug 674161 which will simplify the urlbarBindings logic (and should fix bug 598587).

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30) > >+ { desc: "Ctrl/Cmd+Return keypress", > >+ event: { meta: true }, > >+ check: checkCurrent > > This looks wrong - shouldn't that be accelKey, and checkNewTab? Yeah, should be accelKey, but no, doing cmd+return opens the URI in the current tab. If that's not what you think should happen, we'll have to file a bug on that. > Looks good otherwise, though there is that issue with some other test > interfering with this that needs to be sorted out. New patch with that coming shortly.

I found that browser_bug647886.js ended with a page loaded, so that was breaking my tests, since they expect the urlbar to be empty when the test starts. Making browser_bug647886.js do it's stuff in a new tab fixed this problem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d161a15f9dec https://hg.mozilla.org/integration/mozilla-inbound/rev/4fce25f18e7a

https://hg.mozilla.org/mozilla-central/rev/4fce25f18e7a https://hg.mozilla.org/mozilla-central/rev/d161a15f9dec