Closed Bug 1389377 Opened 7 years ago Closed 7 years ago

Make default in about:preferences should update default browser pane

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1389377 - Click make default button in preferences should update default browser pane
59 bytes, text/x-review-board-request
jaws
: review+
Details 
[Steps to reproduce]:
1. Open about:preferences (Make sure your default browser isn't Firefox)
2. Click "Make Default..." button in General > Startup panel
3. Click "Use Firefox" in popup dialog (Assuming you're using macOS, but it depends on your OS)

[Expected result]:
- Messenge will be updated immediately to "Smile face" + "Firefox is currently your default browser" 

[Actual result]:
- Messenge stay in "Sad face" + "Firefox is not your default browser"
Flags: qe-verify+
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
Here is my investigation about the implementation behind triggering "Make Default" button.

After "Make Default" button click, button will trigger shellSvc.setDefaultBrowser [1] which is implemented in [2]. Unfortunately, I found that [1] always throw error as below:


[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIShellService.setDefaultBrowser]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/preferences/in-content-new/main.js :: setDefaultBrowser :: line 868"  data: no]

as a result, the UI won't be updated correctly [3] as expected.

Jared, do you have any idea about that?

[1] http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/main.js#868
[2] http://searchfox.org/mozilla-central/source/browser/components/shell/nsGNOMEShellService.cpp#250-328
[3] http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/main.js#874-875
Flags: needinfo?(jaws)
For macOS, the shellSvc.setDefaultBrowser implementation should be at [1].

[1]
This issue has been verified that only happen on macOS but it's unable to reproduce on Windows and Linux.

It seems like the root cause might be in nsMacShellService.cpp implementation [1]. This API call should be synchronized and block main thread until user respond the popup dialog.

On the other hand, changing default browser setting to a non-firefox browser on Windows and Linux will update the default browser pane instantly as well. But there is nothing happening on macOS.

[1] http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#63-98
Would we seriously want to do this? "This API call should be synchronized and block main thread until user respond the popup dialog."

It sounds like we just need to do something similar to http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/browser/components/preferences/in-content-new/main.js#155-180 for macOS.
Flags: needinfo?(jaws)
The solution you mentioned on comment 6 works for me on macOS. Submitted patch for review. Thanks
I noticed that the default browser doesn't update instantly if Firefox is no longer to be default browser until refresh the preferences page. I'd like to change the patch to remove the platform condition to make sure every platform takes the same effect and poll the latest default browser info constantly.
Comment on attachment 8897265 [details]
Bug 1389377 - Click make default button in preferences should update default browser pane

https://reviewboard.mozilla.org/r/168548/#review174126

These calls to get the default browser from the shell service can get quite expensive.

Can you implement a simple exponential backoff on the checking here? This exponential backoff can be reset if the user clicks the "Make default" button.

To do this in a simple fashion:

1. Create an array of backoff amounts: let backoffTimes = [1000, 1000, 1000, 1000, 2000, 2000, 2000, 5000, 5000, 10000]
2. Each time the idle callback is run, call setTimeout with the next amount from the backoffTimes array.
3. If the user clicks the "make default" button, reset the backoffTimes index to 0.
4. Limit the backoffTimes index to backoffTimes.length - 1.

After 20 seconds, we will only check the default browser every 10 seconds.
The times would be:
1 second
2 seconds
3 seconds
4 seconds
6 seconds
8 seconds
10 seconds
15 seconds
20 seconds
30 seconds
40 seconds
50 seconds
...

The main idea here is to slow down our checking of the default browser by an order of magnitude from what we are doing right now. Users would on average change the default browser half-way through an interval, so a 10 second interval only translates to a 5 second delay of updating the visual state of the preferences (on average).
Attachment #8897265 - Flags: review?(jaws) → review-
Comment on attachment 8897265 [details]
Bug 1389377 - Click make default button in preferences should update default browser pane

https://reviewboard.mozilla.org/r/168548/#review174432

::: browser/components/preferences/in-content-new/main.js:171
(Diff revision 8)
> -          if ((uri == "about:preferences" || uri == "about:preferences#general") &&
> +        if ((uri == "about:preferences" || uri == "about:preferences#general") &&
> -              document.visibilityState == "visible") {
> +            document.visibilityState == "visible") {
> -            this.updateSetDefaultBrowser();
> +          this.updateSetDefaultBrowser();
> -          }
> +        }
>  
> -          // approximately a "requestIdleInterval"
> +        // approximately a "requestIdleInterval

the trailing quote got accidentally removed

::: browser/components/preferences/in-content-new/main.js:175
(Diff revision 8)
>  
> -          // approximately a "requestIdleInterval"
> +        // approximately a "requestIdleInterval
> -          window.setTimeout(() => {
> +        window.setTimeout(() => {
> -            window.requestIdleCallback(pollForDefaultBrowser);
> +          window.requestIdleCallback(pollForDefaultBrowser);
> -          }, 1000);
> +        }, backoffTimes[this._backoffIndex + 1 < backoffTimes.length ?
> +                        ++this._backoffIndex : backoffTimes.length - 1]);

this._backoffIndex++ : backoffTimes.length - 1

otherwise we will never use index=0
Attachment #8897265 - Flags: review?(jaws) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f478a5b88b8f
Click make default button in preferences should update default browser pane r=jaws
https://hg.mozilla.org/mozilla-central/rev/f478a5b88b8f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Build ID: 20170817100132
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: