Last Comment Bug 798166 - Intermittent defaults display problem after setting default depending on helper.exe speed
: Intermittent defaults display problem after setting default depending on help...
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: unspecified
: x86_64 Windows 7
-- normal (vote)
: Firefox 19
Assigned To: Brian R. Bondy [:bbondy]
: Robert Strong [:rstrong] (use needinfo to contact me)
Depends on:
Blocks: 791019
  Show dependency treegraph
Reported: 2012-10-04 18:23 PDT by Brian R. Bondy [:bbondy]
Modified: 2012-10-15 15:39 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (2.88 KB, patch)
2012-10-04 18:24 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2 (3.06 KB, patch)
2012-10-04 18:28 PDT, Brian R. Bondy [:bbondy]
felipc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Brian R. Bondy [:bbondy] 2012-10-04 18:23:34 PDT
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.
Comment 1 User image Brian R. Bondy [:bbondy] 2012-10-04 18:24:53 PDT
Created attachment 668269 [details] [diff] [review]
Patch v1
Comment 2 User image Brian R. Bondy [:bbondy] 2012-10-04 18:25:34 PDT
We'll request to get this in v17 once it is beta, but not v16.
Comment 3 User image Brian R. Bondy [:bbondy] 2012-10-04 18:28:09 PDT
Created attachment 668271 [details] [diff] [review]
Patch v2

Just synced the 2 files comments since the last patch
Comment 4 User image Brian R. Bondy [:bbondy] 2012-10-05 11:58:20 PDT
I tested this manually and pushed to try with a different patch and it all passes btw if that helps when reviewing it.
Comment 5 User image :Felipe Gomes (needinfo me!) 2012-10-10 13:11:47 PDT
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
Comment 7 User image Ed Morley [:emorley] 2012-10-11 12:08:49 PDT
Comment 8 User image Brian R. Bondy [:bbondy] 2012-10-14 06:12:31 PDT
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

Note You need to log in before you can comment on or make changes to this bug.