Masatoshi Kimura [:emk] Assignee
	Description • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 1 • 9 years ago

What is the hash based on?

	Masatoshi Kimura [:emk] Assignee
	Comment 2 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 3 • 9 years ago

(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?

	Brian R. Bondy [:bbondy]
	Comment 4 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 5 • 9 years ago

None of this patch applies to tip cleanly, mind updating?

	Masatoshi Kimura [:emk] Assignee
	Comment 6 • 9 years ago

* Rebased to tip.
* Used nsIWindowsRegKey instead of reinventing the wheel.

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 9 years ago

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf84b7da846

	Jim Mathies [:jimm]
	Comment 8 • 9 years ago

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?

	Masatoshi Kimura [:emk] Assignee
	Comment 9 • 9 years ago

(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.

	Masatoshi Kimura [:emk] Assignee
	Comment 10 • 9 years ago

To be clear, v3 removed the aWait parameter.

	Jim Mathies [:jimm]
	Comment 11 • 9 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 12 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2645ea066437

	Pulsebot
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/7c48a3782efe

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7c48a3782efe

	Jared Wein [:jaws] (please needinfo? me)
	Comment 15 • 9 years ago

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)

	Masatoshi Kimura [:emk] Assignee
	Comment 16 • 9 years ago

(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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 17 • 9 years ago

[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.

	Masatoshi Kimura [:emk] Assignee
	Comment 18 • 9 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 19 • 9 years ago

Approval Request Comment
See the above request comment for aurora.

	Masatoshi Kimura [:emk] Assignee
	Comment 20 • 9 years ago

Bug 791501 will conflict with this bug. If bug 791501 get a beta-approval, please use this patch instead.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 21 • 9 years ago

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+

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 22 • 9 years ago

Tracking 40 as this is important for the Windows 10 launch.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/92caaca12e6d

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/e52fe64a174f

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 25 • 9 years ago

Setting for verification in Beta from either Masatoshi or Softvision.

	Masatoshi Kimura [:emk] Assignee
	Comment 26 • 9 years ago

I verified this using the latest Firefox 40 beta and Windows 10 RTM.

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 27 • 9 years ago

Removing qe-verify+ flag since Masatoshi verified this in Firefox 40 Beta, and that should suffice.

	Nick Nguyen [:osunick]
	Comment 28 • 9 years ago

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?

	Masatoshi Kimura [:emk] Assignee
	Comment 29 • 9 years ago

(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.