Remember registry hash for later use on Win8+

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
Firefox 42
Unspecified
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40+ verified, firefox41 fixed, firefox42 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
We have to use the default apps UI on Win8+ because associations are protected by a cryptographic hash and we can't tell how to calculate the hash value from the scratch.
Once the hash value is calculated, however, we can remember the value to bypass the default apps UI from then onward. In particular, if Firefox was the default browser before upgrading from Windows 8.1, the user can take back the default browser from Microsoft Edge without being bothered by the boring UI.
Even if the hash value is changed, Firefox will fallback to the current method (i.e. launch the default apps UI). So this is a strict UX improvement.
Attachment #8634685 - Flags: review?(jmathies)
Summary: Remember registry hash for later use on Win8 → Remember registry hash for later use on Win8+
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
What is the hash based on?
Just copying values calculated by the default apps UI. Therefore this patch can't bypass the dialog at first.
For file extensions, values are taken from HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\FIleExts\<ext>\UserChoice. For URL protocols, values are taken from HKCU\Software\Microsoft\Windows\Shell\Associations\UrlAssociations\<protocol>\UserChoice.
(In reply to Masatoshi Kimura [:emk] from comment #2)
> Just copying values calculated by the default apps UI. Therefore this patch
> can't bypass the dialog at first.
> For file extensions, values are taken from
> HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\FIleExts\<ext>\UserCh
> oice. For URL protocols, values are taken from
> HKCU\Software\Microsoft\Windows\Shell\Associations\UrlAssociations\<protocol>
> \UserChoice.

I'm working on getting my win8 laptop updated and built so I can test this. I'll come back to it later today.

Brian you worked with these hashes back when we were doing win8 default browser updates. Any thoughts on the approach here - trying to preserve these values after they have been set?
Flags: needinfo?(netzen)
Maybe a bit intrusive but something like that works for me. I remember testing out an application for fast metro status sharing back when that was a thing with win8.  During install time it did the same thing of asking you to set the default for each first so it could copy out the value.  I think we have registry wrappers already, seems like overkill to make new ones.
Flags: needinfo?(netzen)
None of this patch applies to tip cleanly, mind updating?
Flags: needinfo?(VYV03354)
Posted patch patch v2 (obsolete) — Splinter Review
* Rebased to tip.
* Used nsIWindowsRegKey instead of reinventing the wheel.
Attachment #8634685 - Attachment is obsolete: true
Attachment #8634685 - Flags: review?(jmathies)
Flags: needinfo?(VYV03354)
Attachment #8635647 - Flags: review?(jmathies)
Comment on attachment 8635647 [details] [diff] [review]
patch v2

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

ZGenerally looks great but I'm concerned about jank on the main thread.

::: browser/components/shell/nsWindowsShellService.cpp
@@ +276,5 @@
>                        0, nullptr, nullptr, &si, &pi)) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (aWait) {

This is on the main thread and will jank the entire browser UI for the duration. Are we sure we need this? I there any way we can do this async?
Attachment #8635647 - Flags: review?(jmathies) → review-
Posted patch patch v3 (obsolete) — Splinter Review
(In reply to Jim Mathies [:jimm] from comment #8)
> This is on the main thread and will jank the entire browser UI for the
> duration. Are we sure we need this? I there any way we can do this async?

This is needed only if the default browser is changed from a different Firefox installation. So I think we can live without this. Most users will install only one Firefox instance. Also, Firefox will fallback to the old method even if it fails to set the default browser.
Attachment #8635647 - Attachment is obsolete: true
Attachment #8636289 - Flags: review?(jmathies)
To be clear, v3 removed the aWait parameter.
Comment on attachment 8636289 [details] [diff] [review]
patch v3

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

::: browser/components/shell/nsWindowsShellService.cpp
@@ +370,5 @@
> +
> +  LPCWSTR progID = isProtocol ? L"FirefoxURL" : L"FirefoxHTML";
> +  bool isDefault = !wcsicmp(registeredApp, progID);
> +  CoTaskMemFree(registeredApp);
> +  if (!isDefault) {

looks like you can return isDefault here instead.
Attachment #8636289 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/7c48a3782efe
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
This looks like a patch that we'll want on Firefox 40 so we can get it to users hopefully before they upgrade to Windows 10 and lose their default web browser association.

:emk, I read through the code and it looks like we'll want it for Firefox 40. Can you confirm that if we get this on Firefox 40 and users run it on Windows 8 before updating to Windows 10, their "default browser" experience will be a lot better? (note that we only have a few more days to get the uplift requests in and landed on beta before the final betas will be built)
Flags: needinfo?(VYV03354)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> This looks like a patch that we'll want on Firefox 40 so we can get it to
> users hopefully before they upgrade to Windows 10 and lose their default web
> browser association.
> 
> :emk, I read through the code and it looks like we'll want it for Firefox
> 40. Can you confirm that if we get this on Firefox 40 and users run it on
> Windows 8 before updating to Windows 10, their "default browser" experience
> will be a lot better? (note that we only have a few more days to get the
> uplift requests in and landed on beta before the final betas will be built)

Yes, I confirmed using Windows 8.1 VM and Build 10320 ISO.
Flags: needinfo?(VYV03354)
[Tracking Requested - why for this release]:
We have done a lot of work towards default browser UI with Firefox 40, as the default browser UI is much more complicated in Windows 10.

Can you please rebase the patch for mozilla-aurora and mozilla-beta? It looks like mozilla-aurora just needs the header file moved up to the top of the file but mozilla-beta needs more manual rebasing.
Flags: needinfo?(VYV03354)
Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Default browser UX would be much worse on Windows 10.
[Describe test coverage new/current, TreeHerder]: Manually tested.
[Risks and why]: Low. Even if Firefox fails to set the default, it will fallback to the current method.
[String/UUID change made/needed]: none
Flags: needinfo?(VYV03354)
Attachment #8637502 - Flags: approval-mozilla-aurora?
Approval Request Comment
See the above request comment for aurora.
Attachment #8637503 - Flags: approval-mozilla-beta?
Bug 791501 will conflict with this bug. If bug 791501 get a beta-approval, please use this patch instead.
Comment on attachment 8637503 [details] [diff] [review]
patch for beta

This change has been manually tested. Worst case this should fall back to the existing behaviour if there is a failure. We'll take this change in support of browser default in Windows 10 in beta7. Please verify the fix in beta as well. Beta+ Aurora+
Attachment #8637503 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8637502 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking 40 as this is important for the Windows 10 launch.
Setting for verification in Beta from either Masatoshi or Softvision.
Flags: qe-verify+
I verified this using the latest Firefox 40 beta and Windows 10 RTM.
Removing qe-verify+ flag since Masatoshi verified this in Firefox 40 Beta, and that should suffice.
Flags: qe-verify+
I need to understand this a little better, can someone answer these questions?

1. Starting with Fx40 if the user has Firefox as a default on Windows 8 or 8.1 we store the hash so that we can reapply the change when the user upgrades to 10 and they decide to set Firefox as default again, we can do it without an interstitial dialog?

2. If the user upgrades from 39 to 40 on Win8 and has Firefox as default, do we store this hash, or does the user have to reapply the default change for us to catch it?

3. If the user upgrades from 7 there is no hash to store so we fall back to the associations settings pane, right?
(In reply to Nick Nguyen [:osunick] from comment #28)
> 1. Starting with Fx40 if the user has Firefox as a default on Windows 8 or
> 8.1 we store the hash so that we can reapply the change when the user
> upgrades to 10 and they decide to set Firefox as default again, we can do it
> without an interstitial dialog?

Yes.

> 2. If the user upgrades from 39 to 40 on Win8 and has Firefox as default, do
> we store this hash, or does the user have to reapply the default change for
> us to catch it?

If the user launches Firefox 40 on Win8, the hash will be stored. Firefox will check if it is the default and store the hash if it is.

> 3. If the user upgrades from 7 there is no hash to store so we fall back to
> the associations settings pane, right?

Right.
Depends on: 1240892
You need to log in before you can comment on or make changes to this bug.