Closed Bug 645288 Opened 9 years ago Closed 9 years ago

Associate the webm file extension with Firefox

Categories

(Firefox :: Installer, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
- added uninstall code.

Rob, thoughts? This works well for webm files.
Attachment #522074 - Attachment is obsolete: true
Attachment #522075 - Flags: feedback?(robert.bugzilla)
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.
Attachment #522075 - Flags: feedback?(robert.bugzilla) → feedback-
(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?
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.
Attached patch patch (obsolete) — Splinter Review
(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?
Assignee: nobody → jmathies
Attachment #522075 - Attachment is obsolete: true
Attachment #522353 - Flags: review?(robert.bugzilla)
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.
Attachment #522353 - Flags: review?(robert.bugzilla) → review-
(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}
Looks right though I haven't verified that it does the right thing
(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
How about enumerating the key to see if it has any values?
(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
(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?
Both... enum the .webm key as the macro does.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #522353 - Attachment is obsolete: true
Attachment #523099 - Flags: review?(robert.bugzilla)
Attachment #523099 - Attachment is patch: true
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
Attachment #523099 - Flags: review?(robert.bugzilla) → review-
(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.
Attached patch patchSplinter Review
Attachment #523099 - Attachment is obsolete: true
Attachment #523336 - Flags: review?(robert.bugzilla)
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.
(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 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
Attachment #523336 - Flags: review?(robert.bugzilla) → review+
http://hg.mozilla.org/mozilla-central/rev/09b605eb3e0d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #523336 - Flags: approval2.0?
Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Status: RESOLVED → VERIFIED
Comment on attachment 523336 [details] [diff] [review]
patch

not doing another 4.0.x
Attachment #523336 - Flags: approval2.0? → approval2.0-
You need to log in before you can comment on or make changes to this bug.