965182 - Rename CommandExecuteHandler to something more user friendly

Status: RESOLVED WONTFIX

(Firefox for Metro Graveyard :: Shell, defect)

Description

[Jim Mathies [:jimm]]

This isn't the most user friendly or informative name for an explorer shell extension. We should rename this to something more firefoxy, maybe 'firefox-shell.exe' would suffice.

Down the road we can investigate some other more long term solutions - switching to a dll or potentially embedding this directly in firefox.exe.

(If we decide to do this, it needs to get done this week.)

Comment 1

[Jim Mathies [:jimm]]

Not much to do here and I think the transition to a new exe should be fine - 

http://mxr.mozilla.org/mozilla-central/search?string=CommandExecuteHandler

installer exe names:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#383
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/shared.nsh#15

misc. build stuff:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

I went with firefox-ceh.exe.

Comment 3

[Jim Mathies [:jimm]]

Attachment: manifest patch

We should also add a manifest for this.

Comment 4

[Brian R. Bondy [:bbondy]]

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

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

This looks good but people with zip builds will have startup problems. PostUpdate doesn't run for those people unfortunately.  I wanted to change this in the past but there was concern about supporting people who use "Portable Firefox".  And also people who just don't want extra registry entries added (even know they'd have some). 

Another problem with this is that if a user updates their mozilla-central build, but not their aurora build yet, they'll have startup problems for aurora.

The patch itself is fine as far as I can tell, but I'm a bit concerned about those 2 cases.

The benefit gained in terms of users knowing which program uses the process name, might not be wroth it.
We have other names that aren't descriptive to the product as well, like plugin-container. 

Also if we do want to go with it, it's probably worth sending this through an oak update to make sure the rename is handled properly in terms of registry settings.

What do you think?

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 8367316 [details] [diff] [review]
manifest patch

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

It's not super visible, but probably visible via process explorer or possibly task manager. Should someone else approve the description used?

::: browser/metro/shell/commandexecutehandler/firefox-ceh.exe.manifest
@@ +10,5 @@
> +        processorArchitecture="*"
> +        name="Mozilla.FirefoxCEH"
> +        type="win32"
> +/>
> +<description>Mozilla Explorer Execution Handler</description>

Mozilla Firefox launcher maybe?

@@ +33,5 @@
> +</ms_asmv3:trustInfo>
> +  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
> +    <application>
> +      <supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
> +      <supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>

I don't think it matters much but pls also list Vista, same as updater.exe.manifest:
<application>
      <supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
      <supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
      <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
      <supportedOS Id="{e2011457-1546-43c5-a5fe-008deee3d3f0}"/>
    </application>

Comment 6

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Comment on attachment 8367303 [details] [diff] [review]
> patch
> 
> Review of attachment 8367303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good but people with zip builds will have startup problems.
> PostUpdate doesn't run for those people unfortunately.  I wanted to change

I'm not concerned about breakage for dev build. Broken mc is expected. If we roll out to beta/release with this though we're kinda stuck with it.

> this in the past but there was concern about supporting people who use
> "Portable Firefox".  And also people who just don't want extra registry
> entries added (even know they'd have some). 

I don't understand the concern here, looks like we're just changing a registry entry, not adding a new one. AFAICT this is the only key that gets an update - 

WriteRegStr SHCTX "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}\LocalServer32" "" "${DELEGATE_EXECUTE_HANDLER_PATH}"

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#7757

> Another problem with this is that if a user updates their mozilla-central
> build, but not their aurora build yet, they'll have startup problems for
> aurora.

We can push this to aurora a couple days layer. 

> Also if we do want to go with it, it's probably worth sending this through
> an oak update to make sure the rename is handled properly in terms of
> registry settings.

I want to do this however oak is a little busted (bug 964996). I'm going to try to get that fixed up today. I still want to do some additional update testing as well.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Comment on attachment 8367316 [details] [diff] [review]
> manifest patch
> 
> Review of attachment 8367316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's not super visible, but probably visible via process explorer or
> possibly task manager. Should someone else approve the description used?
> 
> ::: browser/metro/shell/commandexecutehandler/firefox-ceh.exe.manifest
> @@ +10,5 @@
> > +        processorArchitecture="*"
> > +        name="Mozilla.FirefoxCEH"
> > +        type="win32"
> > +/>
> > +<description>Mozilla Explorer Execution Handler</description>
> 
> Mozilla Firefox launcher maybe?

WFM, maybe rstrong can sr.

> @@ +33,5 @@
> > +</ms_asmv3:trustInfo>
> > +  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
> > +    <application>
> > +      <supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
> > +      <supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
> 
> I don't think it matters much but pls also list Vista, same as
> updater.exe.manifest:
> <application>
>       <supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
>       <supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
>       <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
>       <supportedOS Id="{e2011457-1546-43c5-a5fe-008deee3d3f0}"/>
>     </application>

We don't register this on vista right?

Comment 8

[Brian R. Bondy [:bbondy]]

> We can push this to aurora a couple days layer. 

Right but there will be some period of days of bustage. We should notify people if we're doing  it somehow. It might be worth doing a special type of uplift where both branches happen at around the same time in a single day. You'd have to ask for approval with an explanation of why though to drivers.  There would still be some breakage though in terms of both browsers won't be updated by clients at exactly the same time.  I'm not even sure if a user could start their browser from taskbar to do an update during that period.

> We don't register this on vista right?

Yep good point, ignore my comment on that one.

> WFM, maybe rstrong can sr.

Ya please get an sr just because I'm not sure if I have the power to approve something like that. You can r=me though for all other bits in the patch.

> I don't understand the concern here, looks like we're just changing a registry entry, 
> not adding a new one. AFAICT this is the only key that gets an update - 

So we'd be breaking startup long term for anyone that is using a zip build, but also anyone that is using something like Portalbe Firefox.  While you're sr'ing rstrong for the other patch please discuss if this is something we should be concerned about or not.

Personally I'd rather just live with the old name and avoid problems, but I don't feel strongly enough about it to not be convinced.

Comment 9

[Brian R. Bondy [:bbondy]]

Talked to Jim on IRC:
So the aurora/mozilla-central problem is moot. Things are registered with the CEH ID and not the CEH path. The other problem still seems legit though with Portable Firefox and zip builds.

Jim thought of another potential problem though with manually installed CEH settings on the test slaves.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> Talked to Jim on IRC:
> So the aurora/mozilla-central problem is moot. Things are registered with
> the CEH ID and not the CEH path. The other problem still seems legit though
> with Portable Firefox and zip builds.
> 
> Jim thought of another potential problem though with manually installed CEH
> settings on the test slaves.

Yeah, I think that last one is a deal killer at this point. if the name becomes an issue for users, we can revisit.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8367316 [details] [diff] [review]
manifest patch

I'll post this in another bug.