bpo-35308: Fix regression where BROWSER env var is not respected

[zmwangx]

https://bugs.python.org/issue35308

[serhiy-storchaka]

I think the default value for preferred should be negated as well. And perhaps tests need to be updated.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

[zmwangx]

@serhiy-storchaka Thanks for the quick review. Actually, register is called 28 times in webbrowser.py, mostly with an implicit preferred=False, so changing the default value will be quite impactful. Of course, it is arguable that a default value of True may be more intuitive when exposed in the public API, but here comes another problem: since webbrowser.register is public API, is changing the default value of a kwarg allowed in a patch release? Or should I not worry about backporting and let committers figure out that part?

I'll look into adding/updating tests a bit later.

[serhiy-storchaka]

I meant the default value in _synthesize().

[zmwangx]

Oh okay that makes more sense.

[zmwangx]

Wait, are you saying the default value in _synthesize() should be False? Since _synthesize() appears to only be used in situations where the browser is user-supplied, default False seems counterintuitive.

[serhiy-storchaka]

Before e3ce695 both register() and _synthesize() had an undocumented parameter update_tryorder with the default value 1 which meant "add as least preferred". In e3ce695 update_tryorder=1 in register() was replaced with preferred=False, and in 25b804a update_tryorder=1 in _synthesize() was incorrectly replaced with preferred=True. It is my mistake.

[zmwangx]

Sorry for the delay here. Default value of preferred to _synthesize() has been flipped, and a test has been added to make sure BROWSER always takes precedence.

To bot: I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

[miss-islington]

Thanks @zmwangx for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

[bedevere-bot]

GH-10729 is a backport of this pull request to the 3.7 branch.