Closed
Bug 968916
Opened 10 years ago
Closed 10 years ago
Pave over upgrades w/metrofx set as the last browser launched, installer launches desktop
Categories
(Firefox for Metro Graveyard :: Install/Update, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: jimm, Assigned: jimm)
References
Details
(Whiteboard: p=3 s=it-30c-29a-28b.3 r=ff30 [qa-])
Attachments
(2 files, 5 obsolete files)
5.00 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
STR: 1) install an older nightly 2) flip from desktop to metro 3) download latest stub installer in metrofx and run result: stub switches to desktop 4) flip back to metrofx and close it 5) flip to Windows desktop and complete upgrade result: after complete, the stub launches the desktop browser expected: stub should launch into metrofx
Updated•10 years ago
|
Blocks: metrobacklog
Whiteboard: p=0
Assignee | ||
Updated•10 years ago
|
Whiteboard: p=0 → p=3
Updated•10 years ago
|
Assignee: nobody → jmathies
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: kamiljoz
Whiteboard: p=3 → p=3 s=it-30c-29a-28b.2 r=ff30
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8378495 -
Attachment is patch: true
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8378495 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
This switches between launch firefox.exe directly and using a new helper I've added to the ceh for launching the metro browser. Metro is only launched if the install is the default browser, and if the metro front end was loaded last. One question I had herer, there's comment here about $INSTDIR - http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#747 That statement doesn't appear to be true. The macro IsMetroBrowserDefaultOnWin8 is using $INSTDIR and it's valid in the elevated process. I dumped it via a MessageBox to be sure. I'm wondering if I'm missing something here or if that comment can be removed. Note this patch isn't complete, I need to add this to the stub as well.. but I'm not sure how I can go about testing my changes there, maybe a try built stub installer would work?
Attachment #8378524 -
Attachment is obsolete: true
Attachment #8378537 -
Flags: feedback?(robert.strong.bugs)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8378539 -
Attachment description: part 2 - ceh entry point → part 1 - ceh entry point
Assignee | ||
Comment 5•10 years ago
|
||
Cleaned up version without the message boxes.
Attachment #8378537 -
Attachment is obsolete: true
Attachment #8378537 -
Flags: feedback?(robert.strong.bugs)
Attachment #8378544 -
Flags: feedback?(robert.strong.bugs)
Comment 6•10 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #3) > Created attachment 8378537 [details] [diff] [review] > part 1 - installer patch v.1 > > This switches between launch firefox.exe directly and using a new helper > I've added to the ceh for launching the metro browser. Metro is only > launched if the install is the default browser, and if the metro front end > was loaded last. > > One question I had herer, there's comment here about $INSTDIR - > > http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/ > installer.nsi#747 > > That statement doesn't appear to be true. The macro > IsMetroBrowserDefaultOnWin8 is using $INSTDIR and it's valid in the elevated > process. I dumped it via a MessageBox to be sure. I'm wondering if I'm > missing something here or if that comment can be removed. ; Find the installation directory when launching using GetFunctionAddress ; from an elevated installer since $INSTDIR will not be set in this installer The elevated process is making the unelevated process launch the application and when this was implemented the plugin used for elevation didn't sync the main variables so $INSTDIR in the unelevated process could definitely be set to a different value than $INSTDIR in the elevated process. I believe the plugin now syncs the main variables but I haven't had a need to verify this since what we have works. > Note this patch isn't complete, I need to add this to the stub as well.. but > I'm not sure how I can go about testing my changes there, maybe a try built > stub installer would work? The stub just launches the full installer so as long as your new code in the full installer handles the silent run case properly as it does currently when installing from the stub it should be fine. As for testing, you should be able to use a locally built stub and the try server doesn't build stub installers.
Comment 7•10 years ago
|
||
Comment on attachment 8378544 [details] [diff] [review] part 2 - installer patch Looks fine overall though this will need a more thorough review. Use the AppAssocReg plugin for finding out whether Firefox is set as default http://nsis.sourceforge.net/Application_Association_Registration_plug-in
Attachment #8378544 -
Flags: feedback?(robert.strong.bugs) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
> Use the AppAssocReg plugin for finding out whether Firefox is set as default
> http://nsis.sourceforge.net/Application_Association_Registration_plug-in
That is a huge help!
Assignee | ||
Updated•10 years ago
|
Attachment #8378539 -
Flags: review?(netzen)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8378544 -
Attachment is obsolete: true
Attachment #8383252 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8383252 -
Attachment is obsolete: true
Attachment #8383252 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8383256 [details] [diff] [review] part 2 - installer patch this time with the right patch.
Attachment #8383256 -
Flags: review?(robert.strong.bugs)
Comment 12•10 years ago
|
||
Comment on attachment 8383256 [details] [diff] [review] part 2 - installer patch >diff -r 53cfed52f7f2 browser/installer/windows/nsis/installer.nsi >--- a/browser/installer/windows/nsis/installer.nsi Thu Feb 27 14:19:07 2014 -0600 >+++ b/browser/installer/windows/nsis/installer.nsi Thu Feb 27 15:05:39 2014 -0600 >... >@@ -734,34 +731,66 @@ > > Function LaunchApp > ${ManualCloseAppPrompt} "${WindowClass}" "$(WARN_MANUALLY_CLOSE_APP_LAUNCH)" > > ClearErrors > ${GetParameters} $0 > ${GetOptions} "$0" "/UAC:" $1 > ${If} ${Errors} >- Exec "$\"$INSTDIR\${FileMainEXE}$\"" >+ StrCpy $1 "0" Add StrCpy $2 "0" >+!ifdef MOZ_METRO >+ ; Check to see if this install location is currently set as the >+ ; default browser. >+ AppAssocReg::QueryAppIsDefaultAll "${AppRegName}" "effective" >+ Pop $1 >+ ; Check for a last run type to see if metro was the last browser >+ ; front end in use. >+ ReadRegDWORD $2 HKCU "Software\Mozilla\Firefox" "MetroLastAHE" >+!endif >+ ${If} $1 == "1" >+ ${AndIf} $2 == "1" ; 1 equals AHE_IMMERSIVE >+ ; Relaunch into metro >+ Exec '$\"$INSTDIR\CommandExecuteHandler.exe$\" --launchmetro' Change to Exec "$\"$INSTDIR\CommandExecuteHandler.exe$\" --launchmetro" since that is the prevailing style >+ ${Else} >+ ; Relaunch into desktop >+ Exec "$\"$INSTDIR\${FileMainEXE}$\"" >+ ${EndIf} > ${Else} > GetFunctionAddress $0 LaunchAppFromElevatedProcess > UAC::ExecCodeSegment $0 > ${EndIf} > FunctionEnd > > Function LaunchAppFromElevatedProcess > ; Find the installation directory when launching using GetFunctionAddress > ; from an elevated installer since $INSTDIR will not be set in this installer > ${StrFilter} "${FileMainEXE}" "+" "" "" $R9 > ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" "" > ${GetPathFromString} "$0" $0 > ${GetParent} "$0" $1 > ; Set our current working directory to the application's install directory > ; otherwise the 7-Zip temp directory will be in use and won't be deleted. > SetOutPath "$1" >- Exec "$\"$0$\"" >+ StrCpy $2 "0" Add StrCpy $3 "0" >+!ifdef MOZ_METRO >+ ; Check to see if this install location is currently set as the >+ ; default browser. >+ AppAssocReg::QueryAppIsDefaultAll "${AppRegName}" "effective" >+ Pop $2 >+ ; Check for a last run type to see if metro was the last browser >+ ; front end in use. >+ ReadRegDWORD $3 HKCU "Software\Mozilla\Firefox" "MetroLastAHE" >+!endif >+ ${If} $2 == "1" >+ ${AndIf} $3 == "1" ; 1 equals AHE_IMMERSIVE >+ Exec '$\"$1\CommandExecuteHandler.exe$\" --launchmetro' Change to Exec "$\"$1\CommandExecuteHandler.exe$\" --launchmetro" since that is the prevailing style >+ ${Else} >+ Exec "$\"$0$\"" >+ ${EndIf} > FunctionEnd
Attachment #8383256 -
Flags: review?(robert.strong.bugs) → review+
Updated•10 years ago
|
Whiteboard: p=3 s=it-30c-29a-28b.2 r=ff30 → p=3 s=it-30c-29a-28b.3 r=ff30
Updated•10 years ago
|
Attachment #8378539 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 13•10 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/343f8b01d180 remote: https://hg.mozilla.org/integration/fx-team/rev/d8033ff0e71a
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/343f8b01d180 https://hg.mozilla.org/mozilla-central/rev/d8033ff0e71a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment 15•10 years ago
|
||
For testing and verification. Reopen if any defects found.
Flags: needinfo?(kamiljoz)
Updated•10 years ago
|
Whiteboard: p=3 s=it-30c-29a-28b.3 r=ff30 → p=3 s=it-30c-29a-28b.3 r=ff30 [qa-]
Updated•10 years ago
|
Flags: needinfo?(kamiljoz)
You need to log in
before you can comment on or make changes to this bug.
Description
•