Closed Bug 798166 Opened 8 years ago Closed 8 years ago

Intermittent defaults display problem after setting default depending on helper.exe speed

Categories

(Firefox :: Shell Integration, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 19
Tracking Status
firefox16 --- wontfix
firefox17 --- fixed
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(1 file, 1 obsolete file)

After clicking set default browser in preferences < win8, it will sometimes not update the UI to indicate you have the default browser.

This change:
-    document.getElementById("setDefaultPane").selectedIndex = 1;
+    let selectedIndex =
+      shellSvc.isDefaultBrowser(false, true) ? 1 : 0;
+    document.getElementById("setDefaultPane").selectedIndex = selectedIndex;

Caused an intermittent display problem on Windows when setting the default browser.
The real problem is inside windows shell code, where we return success before the launched helper.exe finishes.
But I don't think introducing a wait for helper.exe process is a good thing to do.

The best way to fix this is to just enable the timer always, not just on Windows 8.
The user can go into control panel while the window is open anyway and set defaults that way.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #668269 - Flags: review?(felipc)
We'll request to get this in v17 once it is beta, but not v16.
Attached patch Patch v2Splinter Review
Just synced the 2 files comments since the last patch
Attachment #668269 - Attachment is obsolete: true
Attachment #668269 - Flags: review?(felipc)
Attachment #668271 - Flags: review?(felipc)
I tested this manually and pushed to try with a different patch and it all passes btw if that helps when reviewing it.
Comment on attachment 668271 [details] [diff] [review]
Patch v2

Review of attachment 668271 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/advanced.js
@@ +34,1 @@
>      if (!shellSvc.isDefaultBrowser(false, true)) {

nit discussed on irc: we'll remove this condition and always set up the timer because the user might change this through another channel while the pref window is open
Attachment #668271 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/2091f0330efd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 668271 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 791019
User impact if declined: In preferences, after clicking set default browser on Win7 and below, it will intermittently lead to the button not showing that you are the default.  The user would think they didn't set the default, but it would actually be set.  It is more of a polish regression.  It looks bad that the UI doesn't react as the user expects.
Testing completed (on m-c, etc.): I tested on m-c in Win7 and Win8 and it was working correctly.
Risk to taking this patch (and alternatives if risky): Very low 
String or UUID changes made by this patch: None
Attachment #668271 - Flags: approval-mozilla-beta?
Attachment #668271 - Flags: approval-mozilla-aurora?
Attachment #668271 - Flags: approval-mozilla-beta?
Attachment #668271 - Flags: approval-mozilla-beta+
Attachment #668271 - Flags: approval-mozilla-aurora?
Attachment #668271 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.