This seems to affect only clicks using the new context menu since that's the only path that sets afaict. Is that intentional?

There are other ways to open tabs in the bg from the urlbar. Rather than using , they rely on returning or as appropriate plus the existing logic in openLinkIn, which checks the browser.tabs.loadInBackground pref.

If it is intentional, I'd like to make that more explicit, for example by adding a comment at least. If it's not intentional, then I think this needs some more work to support those other cases.

Thank you very much, Drew!

In D299948#10423403, @adw wrote:

This seems to affect only clicks using the new context menu since that's the only path that sets afaict. Is that intentional?

There are other ways to open tabs in the bg from the urlbar. Rather than using , they rely on returning or as appropriate plus the existing logic in openLinkIn, which checks the browser.tabs.loadInBackground pref.

If it is intentional, I'd like to make that more explicit, for example by adding a comment at least. If it's not intentional, then I think this needs some more work to support those other cases.

Oh, sorry, yeah, you are right! I completely missed that. I will address it. Thanks!

The main updates:

• Since we don't know a place where opening url at unless opening actually via openLinkIn(), get to know it using . And, do closing processing in the callback.
• To avoid extra processing to load URL, added .
• Since the timing of view closing and urlbar blurring, changes a bit.

Thanks Daisuke. This is an interesting approach that I hadn't considered. There are two different aspects that are interesting to me:

1. The view is closed after the page load rather than before, if the load is in the foreground
2. is used to do it

I think point 1 is perfectly valid. The drawback is that it's a substantial change from how the urlbar currently works. The page-load process is already pretty complex and is spread across multiple parts of the codebase, not only urlbar. I'm worried that this change might interact with that process in surprising ways. You mentioned . Have you run this patch on try? If there aren't any significant failures, that would give me some confidence.

But I'd like to ask you to consider two other approaches that I think are simpler. Let me know what you think:

(1) The logic in that determines whether the load should happen in the background is a small number of lines: https://searchfox.org/firefox-main/rev/2bd78d82d0128abc12df9900b6cb264f92adb9d0/browser/modules/URILoadingHelper.sys.mjs#542,544,574-582,596-597

If that logic were made available to callers somehow, it would let you decide whether the view should be closed, right? There are a few ways to do that:

• Factor it out into a new helper function that both and would call
• Modify and all the similar open-link functions it so that they take an out-param that tells callers whether the load happened in the background
• Even just copy-paste the logic into a helper method that you could call

This would let us keep the logic substantially unchanged.

(2) Point 1 (closing the view after the load) is valid, to reiterate. Point 2 (using ) made me think about how the urlbar currently handles page loads.

already has a method that's called when a page is loaded in the foreground: . Rather than use to add yet another code path that runs on page load, it would be simpler to use the existing one in .

That's easier said than done of course. is called in cases other than page loads, like switching tabs. It has some params that tell you whether a page loaded.

There are other places in you'd need to modify besides . For example focuses the content browser, reverts the input, and closes the view, so you'd need to change that so it doesn't happen until is called.

I tried commenting out all those lines in and simply calling in , and it worked surprisingly well. For example something focused the content browser after the view closed even though I didn't add any lines to do it. That illustrates what I mean about the page-load process being complex. If we take this approach, it could be the start of a larger project of simplification and refactoring IMO.

Thank you very much for your review, Drew!

In D299948#10486766, @adw wrote:

Thanks Daisuke. This is an interesting approach that I hadn't considered. There are two different aspects that are interesting to me:

1. The view is closed after the page load rather than before, if the load is in the foreground
2. is used to do it

I think point 1 is perfectly valid. The drawback is that it's a substantial change from how the urlbar currently works. The page-load process is already pretty complex and is spread across multiple parts of the codebase, not only urlbar. I'm worried that this change might interact with that process in surprising ways. You mentioned . Have you run this patch on try? If there aren't any significant failures, that would give me some confidence.

Yes, it seems no issues.
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=dvm8o2WOSAKPt8ukcvCQbQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=5d832678691454a3a8103bbb6cd8e58522609c6d

But I'd like to ask you to consider two other approaches that I think are simpler. Let me know what you think:

(1) The logic in that determines whether the load should happen in the background is a small number of lines: https://searchfox.org/firefox-main/rev/2bd78d82d0128abc12df9900b6cb264f92adb9d0/browser/modules/URILoadingHelper.sys.mjs#542,544,574-582,596-597

If that logic were made available to callers somehow, it would let you decide whether the view should be closed, right? There are a few ways to do that:

• Factor it out into a new helper function that both and would call
• Modify and all the similar open-link functions it so that they take an out-param that tells callers whether the load happened in the background
• Even just copy-paste the logic into a helper method that you could call

This would let us keep the logic substantially unchanged.

I thought this way at first though, even if we make a shared function, there is possibility to change the state in the middle, I wanted to believe the result.

(2) Point 1 (closing the view after the load) is valid, to reiterate. Point 2 (using ) made me think about how the urlbar currently handles page loads.

already has a method that's called when a page is loaded in the foreground: . Rather than use to add yet another code path that runs on page load, it would be simpler to use the existing one in .

That's easier said than done of course. is called in cases other than page loads, like switching tabs. It has some params that tell you whether a page loaded.

There are other places in you'd need to modify besides . For example focuses the content browser, reverts the input, and closes the view, so you'd need to change that so it doesn't happen until is called.

I tried commenting out all those lines in and simply calling in , and it worked surprisingly well. For example something focused the content browser after the view closed even though I didn't add any lines to do it. That illustrates what I mean about the page-load process being complex. If we take this approach, it could be the start of a larger project of simplification and refactoring IMO.

Yeah, super interesting. I want to try this one, thanks!

There are some failures in the try run.
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=G_j7g5YCQgu_EogKPvgBoA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=0fd6836039b05dc9772acd49660f61b9fa006f40
I will investigate them. I suspect one of the causes is the component. With this patch, the focus behavior of will change (e.g. Shift+Alt+Click on the button). I'd like your opinion on whether we should apply the same focus logic to all components as well. @adw

In D299948#10487589, @daisuke wrote:

There are some failures in the try run.
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=G_j7g5YCQgu_EogKPvgBoA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=0fd6836039b05dc9772acd49660f61b9fa006f40
I will investigate them. I suspect one of the causes is the component. With this patch, the focus behavior of will change (e.g. Shift+Alt+Click on the button). I'd like your opinion on whether we should apply the same focus logic to all components as well. @adw

Hmm, doing it in seems like it might be more complex than we expected. We need to add special handling for the following cases, I think.

1. For blank page. We can detect it in the function though. (e.g. about:newtab expects to keep the focus on the urlbar)
2. For since the focus behavior after clicking differs from choosing urlbar result. However, we can't know how the setURI() was called in there, it might be difficult and might need a flag in the class (but a bit naive I think).
3. There are other ways to open uri such as browser.fixupAndLoadURIString(). It seems like this expects keeping the focus on the urlbar even if opening a actual url. I am not sure we can change the focus behavior now..

So, we might be better to revert to the previous revision.. What do you think?

OK, thanks for trying.

I thought this way at first though, even if we make a shared function, there is possibility to change the state in the middle, I wanted to believe the result.

Could you elaborate on this? To me, if we could keep the behavior and timings the same as they are now (aside from keeping the view open in these new cases), that would be the better outcome at this point. Much less risk for regressions and unintended consequences, although it's good that your try push didn't have any failures.

Thank you very much, Drew!
Okay, I will make a common function and use it.

The changes are new since I looked at this, so I'm tagging firefox-desktop-core-reviewers for an additional review.

Thank you very much, @mossop!
I will address your comments.

https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=d2TfIiLiSDuuwHtCm8QSUg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=aac561f0c55129809e36c1f3ae20251b10222f51

Thanks for all your work on this. I'll r+ for urlbar but the other changes lgtm too.

Other than the question about ignoring the pref this looks fine

Thank you very much for the review!

I will try to land this patch once I've confirmed the try run is green.
https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=11e4b844b5b60a6fbab0a52c227ae38f5d913825

https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=b22eefcd8476e4788e45017699d80ca2717dc6b0