645288 - Associate the webm file extension with Firefox

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Jim Mathies [:jimm]]

Clicking a webm file on the desktop results in the default "Use the web to find the correct program" dialog. To load Firefox as a player, we should register fx as a .webm handler.

STR:

1) Save the following file to the desktop:

http://videos.mozilla.org/serv/brand/Mozilla_Firefox_Manifesto_v0.1.webm

2) double click the resulting file.

I'll post a patch that addresses this next.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

- added uninstall code.

Rob, thoughts? This works well for webm files.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 522075 [details] [diff] [review]
patch

We should only take over .webm files if there isn't already a handler for it so if the system has a media player application that handles .webm Firefox won't end up being the handler for .webm when there is an application installed on the system that handles .webm better.

Comment 4

[Jim Mathies [:jimm]]

(In reply to comment #3)
> Comment on attachment 522075 [details] [diff] [review]
> patch
> 
> We should only take over .webm files if there isn't already a handler for it so
> if the system has a media player application that handles .webm Firefox won't
> end up being the handler for .webm when there is an application installed on
> the system that handles .webm better.

Ok, so, that would require a check here - 

  ReadRegStr $6 SHCTX "$0\.webm" ""
  ${If} "$6" != "FirefoxHTML"
    WriteRegStr SHCTX "$0\.webm"  "" "FirefoxHTML"
  ${EndIf}

for an existing handler first, and only set it if it's not assigned?

Comment 5

[Robert Strong (they/them - no direct email)]

Yes. It would also require only removing it when we own it which iirc RegCleanFileHandler does correctly and it will likely need to take it over when an existing Firefox is the current handler.

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch

(In reply to comment #5)
> Yes. It would also require only removing it when we own it which iirc
> RegCleanFileHandler does correctly and it will likely need to take it over when
> an existing Firefox is the current handler.

Looks like all we need is an empty string check here. There's no path information in these entries so there's no need to update it. If it's "" then set it to FirefoxHTML, if there's a string there leave it alone. (If that string happens to be FirefoxHTML that's fine.)

One thing I noticed, $0\Capabilities\FileAssociations entries don't get removed on uninstall so I don't have to worry about that. Curious though if that was meant to be that way or we just forgot to add those to the uninstaller?

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 522353 [details] [diff] [review]
patch

Though the default value will likely be set it is not required. This should only add if .webm doesn't exist.

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #7)
> Comment on attachment 522353 [details] [diff] [review]
> patch
> 
> Though the default value will likely be set it is not required. This should
> only add if .webm doesn't exist.

Something sort of like this?

  ClearErrors
  ReadRegStr $6 SHCTX "$0\.webm" ""
  ; Only add this if it's not taken
  IfErrors 0 +3
  ${If} "$6" == ""
    WriteRegStr SHCTX "$0\.webm"  "" "FirefoxHTML"
  ${EndIf}

Comment 9

[Robert Strong (they/them - no direct email)]

Looks right though I haven't verified that it does the right thing

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 522353 [details] [diff] [review]
> > patch
> > 
> > Though the default value will likely be set it is not required. This should
> > only add if .webm doesn't exist.
> 
> Something sort of like this?
> 
>   ClearErrors
>   ReadRegStr $6 SHCTX "$0\.webm" ""
>   ; Only add this if it's not taken
>   IfErrors 0 +3
>   ${If} "$6" == ""
>     WriteRegStr SHCTX "$0\.webm"  "" "FirefoxHTML"
>   ${EndIf}

That won't work. Looks like we'll have to add a macro:

http://nsis.sourceforge.net/Check_for_a_Registry_Key

Comment 12

[Robert Strong (they/them - no direct email)]

How about enumerating the key to see if it has any values?

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #12)
> How about enumerating the key to see if it has any values?

Looks like that's what the macro on sf does:

!include LogicLib.nsh
!macro IfKeyExists ROOT MAIN_KEY KEY
  Push $R0
  Push $R1
  Push $R2
 
  # XXX bug if ${ROOT}, ${MAIN_KEY} or ${KEY} use $R0 or $R1
 
  StrCpy $R1 "0" # loop index
  StrCpy $R2 "0" # not found
 
  ${Do}
    EnumRegKey $R0 ${ROOT} "${MAIN_KEY}" "$R1"
    ${If} $R0 == "${KEY}"
      StrCpy $R2 "1" # found
      ${Break}
    ${EndIf}
    IntOp $R1 $R1 + 1
  ${LoopWhile} $R0 != ""
 
  ClearErrors
 
  Exch 2
  Pop $R0
  Pop $R1
  Exch $R2
!macroend

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #12)
> How about enumerating the key to see if it has any values?

or are you talking about the webm key?

Comment 15

[Robert Strong (they/them - no direct email)]

Both... enum the .webm key as the macro does.

Comment 16

[Jim Mathies [:jimm]]

Attachment: patch

Ok, added the macro, which returns the result on the stack. Tested with an empty .webm key to confirm it doesn't take it in that case. If the .webm key is not there, we take and subsequently uninstall it. If the webm key's default value is modified, we do not uninstall it.

Comment 17

[Robert Strong (they/them - no direct email)]

Comment on attachment 523099 [details] [diff] [review]
patch

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -240,16 +240,38 @@
>     ${If} ${FileExists} "$QUICKLAUNCH\${BrandFullName}.lnk"
>       ShellLink::SetShortCutWorkingDirectory "$QUICKLAUNCH\${BrandFullName}.lnk" \
>                                              "$INSTDIR"
>     ${EndIf}
>   ${EndUnless}
> !macroend
> !define ShowShortcuts "!insertmacro ShowShortcuts"
> 
>+; Helper for checking for the existence of a registry key.
>+; Pop for result
>+!macro IfKeyExists ROOT MAIN_KEY KEY
>+  Push "0"
>+  Push $R0
>+  Push $R1
>+  StrCpy $R1 "0" # loop index
>+  ${Do}
>+    EnumRegKey $R0 ${ROOT} "${MAIN_KEY}" "$R1"
>+    ${If} $R0 == "${KEY}"
>+        Push "1"
>+        Exch 2
>+      ${Break}
>+    ${EndIf}
>+    IntOp $R1 $R1 + 1
>+  ${LoopWhile} $R0 != ""
>+  ClearErrors
>+  Pop $R0
>+  Pop $R1
>+!macroend
>+!define IfKeyExists "!insertmacro IfKeyExists"
Would be nice to add a macro for this to common.nsh but it shouldn't replace registers if it is added there so I'm fine with this being in shared.nsh if you don't want to deal with that.

> ; Adds the protocol and file handler registry entries for making Firefox the
> ; default handler (uses SHCTX).
> !macro SetHandlers
>   ${GetLongPath} "$INSTDIR\${FileMainEXE}" $8
> 
>   StrCpy $0 "SOFTWARE\Classes"
>   StrCpy $2 "$\"$8$\" -requestPending -osint -url $\"%1$\""
> 
>@@ -274,16 +296,23 @@
>     WriteRegStr SHCTX "$0\.xht"   "" "FirefoxHTML"
>   ${EndIf}
> 
>   ReadRegStr $6 SHCTX "$0\.xhtml" ""
>   ${If} "$6" != "FirefoxHTML"
>     WriteRegStr SHCTX "$0\.xhtml" "" "FirefoxHTML"
>   ${EndIf}
> 
>+  ; Only add webm if it's not present
>+  ${IfKeyExists} SHCTX "$0" ".webm"
>+  Pop $7
>+  ${If} $7 == "0"
>+    WriteRegStr SHCTX "$0\.webm"  "" "FirefoxHTML"
>+  ${EndIf}
>+
>   StrCpy $3 "$\"%1$\",,0,0,,,,"
> 
>   ; An empty string is used for the 5th param because FirefoxHTML is not a
>   ; protocol handler
>   ${AddDDEHandlerValues} "FirefoxHTML" "$2" "$8,1" "${AppRegName} Document" "" \
>                          "${DDEApplication}" "$3" "WWW_OpenURL"
> 
>   ${AddDDEHandlerValues} "FirefoxURL" "$2" "$8,1" "${AppRegName} URL" "true" \
>@@ -355,16 +384,17 @@
>   WriteRegStr HKLM "$0\Capabilities" "ApplicationIcon" "$8,0"
>   WriteRegStr HKLM "$0\Capabilities" "ApplicationName" "${BrandShortName}"
> 
>   WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".htm"   "FirefoxHTML"
>   WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".html"  "FirefoxHTML"
>   WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".shtml" "FirefoxHTML"
>   WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".xht"   "FirefoxHTML"
>   WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".xhtml" "FirefoxHTML"
>+  WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".webm"  "FirefoxHTML"
This would make Firefox the default handler when there is already a default handler... I don't think we should get into the media player business like browser used to be in the image viewer business. I think we should just add ourselves when setting as default with the application provided method.

Minusing mainly to get your thoughts on this.

It would also be a good thing to do the same for ogg which is bug 452254 and has some additional info

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Comment on attachment 523099 [details] [diff] [review]
> patch
> 
> Would be nice to add a macro for this to common.nsh but it shouldn't replace
> registers if it is added there so I'm fine with this being in shared.nsh if you
> don't want to deal with that.

Alright, I'll move it over.

> >+  WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".webm"  "FirefoxHTML"
> This would make Firefox the default handler when there is already a default
> handler... I don't think we should get into the media player business like
> browser used to be in the image viewer business. I think we should just add
> ourselves when setting as default with the application provided method.

We can live without this. Looks like we still pickup the association if there are no other webm compat programs insstalled via the .webm hander entry. (Unless this is left over from previous testing, but it doesn't look like it.)

> It would also be a good thing to do the same for ogg which is bug 452254 and
> has some additional info

I'll take a look. But I'll post any patches for ogg there not here. Related - did you know IE9 takes ownership of and adds capabilities for .svg? We might want to file a bug on that as well.

Comment 19

[Jim Mathies [:jimm]]

Attachment: patch

Comment 20

[Jim Mathies [:jimm]]

Attachment: same thing for ogv (Ogg Video)

Comment 21

[Jim Mathies [:jimm]]

Attachment: standard registration for .svg

If we choose to do this, I think it should be a standard registration. We might want to add mime support as well? Not sure. This is the easy part here.

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to comment #21)
> Created attachment 523349 [details] [diff] [review]
> standard registration for .svg
> 
> If we choose to do this, I think it should be a standard registration. We might
> want to add mime support as well? Not sure. This is the easy part here.
I filed bug 353124 about evaluating the current registrations a while ago and svg, etc. should probably be done there along with a couple of others.

Comment 23

[Robert Strong (they/them - no direct email)]

Comment on attachment 523336 [details] [diff] [review]
patch

>diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh
>--- a/toolkit/mozapps/installer/windows/nsis/common.nsh
>+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh
>@@ -1115,16 +1115,77 @@
>     !insertmacro CreateRegKey
> 
>     !undef _MOZFUNC_UN
>     !define _MOZFUNC_UN
>     !verbose pop
>   !endif
> !macroend
> 
>+/**
>+ * Helper for checking for the existence of a registry key.
>+ * SHCTX is the root key to search.
>+ *
>+ * @param   _MAIN_KEY
>+ *          Sub key to iterate for the key in question
>+ * @param   _KEY
>+ *          Key name to search for
>+ * @return  _RESULT
>+ *          'true' / 'false' result
>+ */
>+!macro TestIfRegistryKeyExists
Currect macro names use Check instead of Test and RegKey instead of RegistryKey. How about changing this to CheckIfRegKeyExists?

>+  !ifndef TestIfRegistryKeyExists
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define TestIfRegistryKeyExists "!insertmacro TestIfRegistryKeyExistsCall"
>+
>+    Function TestIfRegistryKeyExists
>+      ; stack: main key, key
>+      Exch $R9 ; main key
>+      Exch 1
>+      Exch $R8 ; key
>+      Push $R7
>+      Push $R6
>+      Push $R5
>+
>+      StrCpy $R5 "false"
>+      StrCpy $R7 "0" # loop index
>+      ${Do}
>+        EnumRegKey $R6 SHCTX "$R9" "$R7"
>+        ${If} "$R6" == "$R8"
>+          StrCpy $R5 "true"
>+          ${Break}
>+        ${EndIf}
>+        IntOp $R7 $R7 + 1
>+      ${LoopWhile} $R6 != ""
I had some issues with some of the newer logiclib macros with NSIS 2.33u which is what we use on the Tinderbox. Can you verify this works with 2.33u?

r=me with that

Comment 24

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/09b605eb3e0d

Comment 25

[Simona Badau, Desktop QA]

Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 523336 [details] [diff] [review]
patch

not doing another 4.0.x