Closed
Bug 965182
Opened 10 years ago
Closed 10 years ago
Rename CommandExecuteHandler to something more user friendly
Categories
(Firefox for Metro Graveyard :: Shell, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: jimm, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
4.14 KB,
patch
|
Details | Diff | Splinter Review |
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.)
Reporter | ||
Comment 1•10 years ago
|
||
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/
Reporter | ||
Updated•10 years ago
|
Whiteboard: [beta28]
Updated•10 years ago
|
Blocks: metrov1backlog
Whiteboard: [beta28] → [beta28] [feature] p=0
Reporter | ||
Comment 2•10 years ago
|
||
I went with firefox-ceh.exe.
Assignee: nobody → jmathies
Attachment #8367303 -
Flags: review?(netzen)
Reporter | ||
Comment 3•10 years ago
|
||
We should also add a manifest for this.
Attachment #8367316 -
Flags: review?(netzen)
Comment 4•10 years ago
|
||
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?
Attachment #8367303 -
Flags: review?(netzen)
Updated•10 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Comment 5•10 years ago
|
||
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>
Attachment #8367316 -
Flags: review?(netzen) → feedback+
Reporter | ||
Comment 6•10 years ago
|
||
(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.
Reporter | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
> 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•10 years ago
|
||
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.
Reporter | ||
Comment 10•10 years ago
|
||
(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.
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8367316 [details] [diff] [review] manifest patch I'll post this in another bug.
Attachment #8367316 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Updated•10 years ago
|
Assignee: jmathies → nobody
No longer blocks: metrov1it23
Priority: P2 → --
QA Contact: jbecerra
Whiteboard: [beta28] [feature] p=0
You need to log in
before you can comment on or make changes to this bug.
Description
•