Closed Bug 502933 Opened 11 years ago Closed 10 years ago

Installation takes way too long

Categories

(Firefox for Android Graveyard :: General, enhancement)

ARM
Windows Mobile 6 Professional
enhancement
Not set

Tracking

(status1.9.2 final-fixed, fennec1.0-wm+)

RESOLVED FIXED
Tracking Status
status1.9.2 --- final-fixed
fennec 1.0-wm+ ---

People

(Reporter: alexp, Assigned: alexp)

References

Details

Attachments

(6 files, 22 obsolete files)

35.00 KB, application/msword
Details
267.24 KB, patch
alexp
: review+
Details | Diff | Splinter Review
2.00 KB, patch
alexp
: review+
Details | Diff | Splinter Review
1.84 MB, patch
alexp
: review+
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
76.88 KB, patch
crowderbt
: review+
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
1.86 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11 (.NET CLR 3.5.30729)
Build Identifier: fennec-1.0a2.en-US.wince-arm

It takes up to several minutes to install Fennec from a CAB on a WM device.


Reproducible: Always

Steps to Reproduce:
1. Copy fennec*.cab to a device.
2. Run it.
3. Notice the time it takes to install.
The CAB installer does not do anything special - it just copies all the files to the specified folders, so we are limited just by the speed of this operation, which is pretty long due to a large number of files, and their total size.

But as you can see from the time measurements, this standard system functionality is not implemented the most optimal way. Total Commander performs copying and extracting from an archive much faster (on the Asus it was 3 times faster).

So if we want to improve this installation speed we could use some kind of a custom/hand-made installer, which itself would extract the files from an archive.
OS: Other → Windows Mobile 6 Professional
Hardware: Other → ARM
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-fennec1.0+
tracking-fennec: --- → 1.0-wm+
Flags: wanted-fennec1.0+
This version extracts a zip file, which has the same name as the exe, just with
extension .zip, and is located in the same folder.
For example if you want to install fennec-1.0a3pre.en-US.wince-arm.zip, build
the program, rename fennec_installer.exe to fennec-1.0a3pre.en-US.wince-arm.exe,
copy both files to a Windows Mobile device and run the exe.

Note: Requires zlib.patch (see another attachment) to avoid conflict between mozz.lib and mozce_shunt.

Known issues:
- The Makefile is copied to the $(OBJDIR), but does not get invoked when the whole tree is built with "make -f client.mk build";
- nsZipArchive.* is a copy from modules/libjar - needs to be built from its original location, but with different compiler option;
- rc.exe cannot find "resdefce.h" when called from the make, though it works when the same command line is executed from cmd.exe.
Assignee: nobody → alex.mozilla
Status: NEW → ASSIGNED
[Work in Progress] Zip Extractor implementation, which is supposed to replace CAB installer.

This version extracts a ZIP file appended to the EXE, i.e. works like a self-extracted ZIP.

For example if you want to install fennec-1.0a3pre.en-US.wince-arm.zip, build the program,
run the following command:

  cat fennec_installer.exe fennec-1.0a3pre.en-US.wince-arm.zip >fennec-1.0a3pre.en-US.wince-arm.exe

Then copy fennec-1.0a3pre.en-US.wince-arm.exe to a device and run it.

Note: Requires zlib.patch to avoid conflict between mozz.lib and mozce_shunt.
Attachment #389633 - Attachment is obsolete: true
Updated version, which includes the uninstaller.

Known issue to be fixed:
- rc.exe cannot find "resdefce.h" when called from the make, though it works
when the same command line is executed from cmd.exe.
Attachment #391230 - Attachment is obsolete: true
Depends on: 508721
Some updates to the code.
Also includes the fix for the bug 503109 (do our first run work during cab installation).
Attachment #392662 - Attachment is obsolete: true
Updated version - includes a couple zip headers, which still have standalone mode support removed with a fix to the bug 505784.
Attachment #393041 - Attachment is obsolete: true
Comment on attachment 393963 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

A couple quick things

1) This should be in m-c's toolkit/mozapps/installer/wince instead of m-b's installer/wince
2) delete things instead of commenting them out
3) Attach the .ico file for ui review
4) Build the shunt statically (as well as dynamically) and link to that
5) mozz.lib is in dist/lib, link to it there

Comments on the actual code to follow
Updated according to review comments.
Attachment #389635 - Attachment is obsolete: true
Attachment #393963 - Attachment is obsolete: true
Attachment #394427 - Flags: review?(bugmail)
Create EXE instead of CAB with "make package".
Attachment #394429 - Flags: review?(bugmail)
Attachment #394429 - Attachment mime type: application/octet-stream → text/plain
Attachment #394429 - Attachment is patch: true
Attachment #394429 - Flags: review?(bugmail) → review+
Comment on attachment 394427 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

Rob, since this is in toolkit now do you mind taking the review?
Attachment #394427 - Flags: review?(bugmail) → review?(robert.bugzilla)
Comment on attachment 394427 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

Drive by comments:
* This patch is too fennec centric. I mean, we should be more like xulrunner_stub here. This includes:
** Do not include the fennec.ico icon in mozapps. Use _no_ icon and mobile can insert the right icon from the mobile build side (we do this for fennec.exe now)
** Name the installer "xulrunner_stub_installer" in mozapps. Yes I know it's long and ugly, but the point is mobile will rename it from the mobile build (we do this now for xulrunner_stub -> fennec)
** I'd prefer all occurrences of "Fennec" to be changed to "XULRunner" since this is really a method of installing xulrunner applications via a special installer.
Comment on attachment 394427 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

Glancing at the patch, I see several hard-coded strings that should be exposed to l10n. That should be done by either a .properties or .ini file, I guess. There shouldn't be any compilations to be run to repack en-US for l10n.

Reading the comments, we have:

- two files to download, the zip and the exe, which need to be next to each other at launching the exe?

- first-run code in the exe. How fennec-specific is that code?
Localization has been on my TODO list for quite a while now. Have to think of the best way to implement it so that everything will be still in just one file.

As for the questions:

- The latest version of the installer works like SFX-ZIP - there will be only one file to download - this is a must, I believe.

- First-run code is just executing "fennec.exe -silent". We are thinking about making it easily customizable - have some kind of a script file with a command (or several commands) to run.
- Fixed a couple of issues including a build error;
- Renamed fennec_installer to xulrunner-stub-installer.
Attachment #394427 - Attachment is obsolete: true
Attachment #394784 - Flags: review?
Attachment #394427 - Flags: review?(robert.bugzilla)
Fixed the file paths.
Attachment #394429 - Attachment is obsolete: true
Attachment #394785 - Flags: review?
I noticed that the latest 7-Zip which has better compression now has support for Windows CE.
Comment on attachment 394784 [details] [diff] [review]
[Work in Progress] Zip Extractor implementation to replace CAB installer

Please switch to use /xulrunner/app/xulrunner.ico or /toolkit/mozapps/installer/windows/nsis/setup.ico

We do not want a fennec.ico in mozilla-central
Attachment #394784 - Flags: review-
And we will need to fix the hard coded strings quickly too.
Implemented changes as per review comments:
- Separated text strings from the code into a separate file (setup.str), so it could be localized and customized for other versions;
- Replaced Fennec.ico with xulrunner.ico;
- Made Uninstall use the strings file too;
- Uninstall now copies itself to a temp directory and runs from there to delete the whole installation.
Attachment #394784 - Attachment is obsolete: true
Attachment #395510 - Flags: review?
Attachment #394784 - Flags: review?
Added strings file handling to the mobile installer Makefile.
Attachment #394785 - Attachment is obsolete: true
Attachment #394785 - Flags: review?
Attachment #395511 - Flags: review?
Attachment #395510 - Flags: review? → review?(robert.bugzilla)
Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

drive by review part 1

Meta comment, I think we can just go with wide strings rather than the ambiguous TCHARs

>-DIRS = downloads extensions update xpinstall plugins handling
>+DIRS = downloads extensions update xpinstall plugins handling installer
and
>diff --git a/toolkit/mozapps/installer/Makefile.in b/toolkit/mozapps/installer/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/installer/Makefile.in
we try to avoid no-op make files.  Just add installer/wince above


>+CStrings::CStrings()
Let's rename this to avoid confusion with MS's CString and to describe its function more clearly.  nsLocalizedStrings would seem to work.

>+CStrings::~CStrings()
>+{
>+  delete[] m_sBuf;
>+}
if (m_sBuf)
   delete[] m_sBuf;

>+    if (ReadFile(f, m_sBuf, m_nBufLen * sizeof(TCHAR), &dwRead, NULL))
>+      bResult = ParseStrings();

you should probably fill msBuf in a loop 

Also, m_cMaxStringID isn't very obvious.  mNumbStringIDs maybe?



>+  // SHInitExtraControls should be called once during your application's initialization to initialize any
>+  // of the device specific controls such as CAPEDIT and SIPPREF.
Sounds like these comments were taken from some example code or boiler plate.
Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

part 2

rename CZipExtractorDlg to nsZipExtractorDlg

>diff --git a/toolkit/mozapps/installer/wince/nsZipArchive.cpp b/toolkit/mozapps/installer/wince/nsZipArchive.cpp
>diff --git a/toolkit/mozapps/installer/wince/nsZipArchive.h b/toolkit/mozapps/installer/wince/nsZipArchive.h
>diff --git a/toolkit/mozapps/installer/wince/nsZipExtractor.cpp b/toolkit/mozapps/installer/wince/nsZipExtractor.cpp
>diff --git a/toolkit/mozapps/installer/wince/nsZipExtractor.h b/toolkit/mozapps/installer/wince/nsZipExtractor.h
>diff --git a/toolkit/mozapps/installer/wince/zipfile.h b/toolkit/mozapps/installer/wince/zipfile.h
>diff --git a/toolkit/mozapps/installer/wince/zipstub.h b/toolkit/mozapps/installer/wince/zipstub.h

why are you copying these files?


>diff --git a/toolkit/mozapps/installer/wince/stdafx.h b/toolkit/mozapps/installer/wince/stdafx.h
we don't usually use these sort of headers
Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

r- on the strings parts. Not sure what '.str' is supposed to be (can't tell from the diff), but you should use one of the existing formats, either .ini or .properties, or .inc. Look at what the crashreporter and the updater code do, there's already standalone code to copy for reading .ini files.

The localizable content will need to be in locales/en-US/, like in a subdir installer, that's where other apps have that.
Attachment #395510 - Flags: review-
Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

I had a discussion with bsmedberg and taras today and they don't want to un-nuke standalone libjar (I didn't realize it had been nuked before the conversation). They both agreed that a solution using 7-Zip (we already have the 7-Zip source under other-licenses though it would need to be updated to the latest to support Windows CE) would be accepted.
Attachment #395510 - Flags: review?(robert.bugzilla) → review-
This archive contains compiled zip-extractor binary and a script, which can be used to create an EXE installer package from existing fennec directory.
Can be used to test the current version of the EXE installer.
- Fixed code review issues
- Localizable text is now stored in locales/en-US/installer/setup.ini
- Modified updater/readstrings.* parser to support other applications
- Refactored archive extractor to make a switch to another archiver easier
Attachment #395510 - Attachment is obsolete: true
- Added locales/en-US/installer/setup.ini
- Modified mobile/installer/Makefile to handle ZIP archive and setup.ini.
Attachment #395511 - Attachment is obsolete: true
Attachment #395511 - Flags: review?
[corrected patch]

- Added locales/en-US/installer/setup.ini
- Modified mobile/installer/Makefile to handle ZIP archive and setup.ini.
Attachment #398908 - Attachment is obsolete: true
Test version of EXE installer using 7-zip compression is now available at
http://people.mozilla.com/~alexp/
Attached patch lib7z (obsolete) — Splinter Review
Library to extract 7z archives.
Includes 7-zip LZMA SDK version 9.07 beta.
Attachment #402977 - Flags: review?(robert.bugzilla)
New version, which uses 7-zip instead of Zip.
Requires lib7z.
Attachment #396952 - Attachment is obsolete: true
Attachment #398907 - Attachment is obsolete: true
Attachment #402981 - Flags: review?(robert.bugzilla)
Includes localizable strings and a change to Makefile to create a package.
Attachment #398909 - Attachment is obsolete: true
Attachment #402983 - Flags: review?
Alex, I have the flu and am rather out of it so please have someone else review these patches especially since I'm not a peer of this part of the tree
Attachment #402977 - Flags: review?(robert.bugzilla) → review?(benjamin)
Attachment #402981 - Flags: review?(robert.bugzilla) → review?(benjamin)
Attachment #402983 - Flags: review? → review?(bugmail)
Comment on attachment 402983 [details] [diff] [review]
Additional patch for the mobile tree


>-	cd $(DIST) && $(PYTHON) $(topsrcdir)/build/package/wince/make_wince_cab.py $(CABARGS) "$(VSINSTALLDIR)/SmartDevices/SDK/SDKTools/cabwiz.exe" "$(MOZ_PKG_DIR)" "$(MOZ_APP_DISPLAYNAME)" "$(PKG_PATH)$(PKG_BASENAME).cab" && $(ZIP) -r9D $(PACKAGE) $(MOZ_PKG_DIR) && echo "Installer $(PKG_PATH)$(PKG_BASENAME).cab created!"
>+	cp $(srcdir)/../locales/$(AB_CD)/installer/setup.ini $(DIST)/setup.ini
>+	cd $(DIST) && mv $(MOZ_PKG_DIR)/xulrunner/uninstall.exe $(MOZ_PKG_DIR)/ && 7z a $(PKG_PATH)$(PKG_BASENAME).7z $(MOZ_PKG_DIR) setup.ini -x!$(MOZ_PKG_DIR)/xulrunner/xulrunner-stub-installer.sfx && cat $(MOZ_PKG_DIR)/xulrunner/xulrunner-stub-installer.sfx $(PKG_PATH)$(PKG_BASENAME).7z>$(PKG_PATH)$(PKG_BASENAME).exe && rm $(PKG_PATH)$(PKG_BASENAME).7z && rm setup.ini
>+	@echo Installer $(PKG_PATH)$(PKG_BASENAME).exe created!

two nits, use nsinstall instead of cp and we don't need this echo
Attachment #402983 - Flags: review?(bugmail) → review+
Blocks: 519230
Blocks: 503109
Fixed the "nits".
Attachment #402983 - Attachment is obsolete: true
This patch contains windows-style line endings in many places. Please make sure that only unix-style endings are checked in.

I presume that the files in LZMASDK are copied without modification from the 7zip SDK? Was that taken from version control or a release number? I think you should at least have a README.mozilla file which explains where the sources came from.

> diff --git a/modules/lib7z/7zLib.cpp b/modules/lib7z/7zLib.cpp

> +/* -*- Mode: C++; c-basic-offset: 2; tab-width: 4; indent-tabs-mode: nil; -*- */^M

Here and in 7zLib.h: tab-width should be 8 (so it's easy to notice
tabs that get inserted by accident). There are windows-style newlines
in this file. Please remove them.

> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

> + *

> + * The contents of this file are subject to the Mozilla Public License Version

> + * 1.1 (the "License"); you may not use this file except in compliance with

> + * the License. You may obtain a copy of the License at

> + * http://www.mozilla.org/MPL/

> + *

> + * Software distributed under the License is distributed on an "AS IS" basis,

> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License

> + * for the specific language governing rights and limitations under the

> + * License.

> + *

> + * The Original Code is 7z Client and 7z Standalone Extracting Plugin.

> + *

> + * The Initial Developer of the Original Code is

> + * Igor Pavlov

> + *

> + * Portions created by the Initial Developer are Copyright (C) 2009

> + * the Initial Developer. All Rights Reserved.


This is incorrect. Please have Gerv create a correct license header for this
file. Although Igor Pavlov is the author of the original code, he explicitly
disclaimed all copyright by placing it into the public domain. Your
modifications of his code can be tri-licensed, but the header needs to say
something different than this.

> diff --git a/modules/lib7z/7zobjs.mk b/modules/lib7z/7zobjs.mk

> +7ZIPSRCS = \

> +	$(7ZIPSRCDIR)/CPP/7zip/Archive/7z/7zDecode.cpp \


In order for the build system to calculate dependencies correctly, use
need to use bare filenames in CPPSRCS/CSRCS instead of directories. So
what you want here is:

7ZIPCPPSRCS = \
  7ZDecode.cpp \
  7zExtract.cpp \
  etc...

7ZIPCSRCS = \
  7zCrc.c \
  etc..

vpath %.cpp $(7ZIPSRCDIR)/CPP/7zip/Archive/7z
vpath %.cpp $(7ZIPSRCDIR)/CPP/Archive
vpath %.cpp $(7ZIPSRCDIR)/CPP/7zip/Archive/Common
etc...
vpath %.c $(7ZIPSRCDIR)/C

Also please use 7ZIPCPPSRCS instead of 7ZIPSRCS. It took me a minute to figure out what the difference between 7ZIPSRCS and 7ZIPCSRCS was.

More windows-style line endings

> diff --git a/modules/lib7z/Makefile.in b/modules/lib7z/Makefile.in

> +# The Initial Developer of the Original Code is
> +# Alex Pakhotin <alexp@mozilla.com>

The correct initial developer for employees/contractors is "The Mozilla Foundation <http://www.mozilla.org>"

Can we move the WINCE ifdef out of this makefile. There's no point even recursing into this directory if we're not on WINCE; it just takes extra build time. Put the ifdef in toolkit-tiers.mk

> +OS_LIBS += oleaut32.lib ole32.lib

Why do you have this line? You're creating a static library, so
OS_LIBS doesn't apply, I don't think.
Attachment #402977 - Flags: review?(benjamin) → review-
Attachment #402981 - Flags: review?(benjamin) → review?(robert.bugzilla)
Because the 7zip code is in the public domain, it is entirely *legal* for us to slap tri-license headers on our copies, and say that these particular copies (even without modifications) are tri-licensed.

It is legal, but it's probably not the right thing to do. The nice thing to do is to put our changes in the public domain also, so that 7zip can pick them up if they want. So I suggest that you do not add any license headers, to Igor's files, but instead have a README which says something like:

This code is from the LZMA SDK. It is in the public domain (see <url>).
Any copyright in these files held by contributors to the Mozilla Project is also dedicated to the Public Domain.
http://creativecommons.org/licenses/publicdomain/

Gerv
The library consists of two parts.

LZMASDK sub-directory contains unchanged code from LZMA SDK. It is planned to be intact, I'm only adding a README.mozilla file as recommended by Benjamin, to state the version and origin.
Theoretically the files in that directory could be replaced with the newer version of the SDK later.

4 files outside of LZMASDK including a C++ source, Makefile, and a couple of include files are not from the SDK. Makefile is Mozilla-build specific and created from scratch. 7zLib.cpp does contain a lot of code from the SDK, because it is basically a heavily modified for our purposes version of LZMASDK/CPP/7zip/UI/Client7z/Client7z.cpp with some specific additions to make it work in a static library. It could be placed in the public domain, though I doubt our changes could be picked up "as is".

Taking this into account, do you still want me to just add that README and not have any header in 7zLib.cpp?

And if we place this code in the public domain, should we move it to other-licenses again? Please note this library is integrated into the build system, I'm not sure if it will the right thing to use it this way from other-licenses.
Please add a header like the one I suggest to the heavily-modified file 7zLib.cpp. There is no need to move anything to other-licenses; "other" means "things not under the tri-license or a trvially compatible one (such as BSD or public domain)". Add a tri-license header to the Makefile. That will be all :-)

Gerv
Attached patch lib7z (obsolete) — Splinter Review
Library to extract 7z archives.
Includes 7-zip LZMA SDK version 9.07 beta.

Implemented changes as per the review.
Attachment #402977 - Attachment is obsolete: true
Attachment #404365 - Flags: review?
Attachment #404365 - Flags: review? → review?(benjamin)
+# Having this rule here to compile 7zIn.cpp, but not 7zIn.c
+%.$(OBJ_SUFFIX): %.cpp $(GLOBAL_DEPS)

I wanted this to be a static pattern rule:

7zIn.$(OBJ_SUFFIX): %.$(OBJ_SUFFIX): %.cpp $(GLOBAL_DEPS)

And any custom rules should be *after* rules.mk, not before, to avoid messing up other ordering. r=me with that change
Comment on attachment 402981 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

>diff --git a/toolkit/mozapps/update/src/updater/readstrings.cpp b/toolkit/mozapps/update/src/updater/readstrings.cpp
>--- a/toolkit/mozapps/update/src/updater/readstrings.cpp
>+++ b/toolkit/mozapps/update/src/updater/readstrings.cpp
>...
>@@ -106,19 +111,52 @@ NS_strtok(const char *delims, char **str
>...
>+/**
>+ * Very basic parser for updater.ini taken mostly from nsINIParser.cpp
>+ * Modified to be used for the installer or similar standalone app as well.
nit: Change the comment to s/Very/A very/
Change the second line to:
that can be used by standalone apps.

r=me for the updater changes... do you need me to review the remainder as well?
Attachment #402981 - Flags: review?(robert.bugzilla) → review+
Attachment #404365 - Flags: review?(benjamin) → review+
Attached patch lib7zSplinter Review
Library to extract 7z archives.
Includes 7-zip LZMA SDK version 9.07 beta.
---
Fixed Makefile.in as suggested.
Attachment #404365 - Attachment is obsolete: true
Attachment #404907 - Flags: review+
Attachment #403350 - Flags: review+
Attachment #402980 - Flags: review+
The latest version of the patch with some minor fixes.
Attachment #402981 - Attachment is obsolete: true
Attachment #406260 - Flags: review?(robert.bugzilla)
Alex, have you had a chance to send the patches to the try server as I asked yesterday and if so, what are the results especially in regards to Firefox on WinCE?
The patch has to be fixed a bit (one makefile), after that it's OK on all the platforms, ...except WinMo!
Now I'm trying to figure out why it fails. It builds fine on my machine with exactly the same mozconfig.
Fixed the Makefile for Firefox WinCE build.

Everything builds on the Try server now.
Attachment #406260 - Attachment is obsolete: true
Attachment #406736 - Flags: review?(robert.bugzilla)
Attachment #406260 - Flags: review?(robert.bugzilla)
Comment on attachment 406736 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

Robert asked me to take a glance at this, so here are my thoughts:

Non-NIT:  Prefer MessageBoxW(...) to MessageBox(...) wherever possible, and use explicit calls instead of overloads for all code like this.

+class ns7zipExtractor : public nsArchiveExtractor
+{
+public:
NIT: If it's all public, how about a struct {} ?  I don't know what our standards say about this, though.

+nsArchiveExtractor::nsArchiveExtractor(const WCHAR *sArchiveName, DWORD dwSfxStubSize, nsExtractorProgress *pProgress)
+{
+  m_sArchiveName = NULL;
+  if (sArchiveName)
+    m_sArchiveName = wcsdup(sArchiveName);
+  m_pProgress = pProgress;
+  m_dwSfxStubSize = dwSfxStubSize;
+}
NIT: How about using an initializer list for all the copyable stuff here (and in similar classes)?  Again, not sure what our standards say about this.

+  if (m_sArchiveName)
+    free(m_sArchiveName);
NIT: free() null-checks.

+  if (!InitInstance(hInstance, nCmdShow)) 
+  {
+    return nResult;
+  }
NIT: Don't need the extra  { } scope here (other similar spots throughout).

+  hImage = CreateFile(g_sExeFullFileName,
+    GENERIC_READ,
+    FILE_SHARE_READ,
+    NULL,
+    OPEN_EXISTING,
+    FILE_ATTRIBUTE_NORMAL,
+    NULL);
NIT: Robert can speak more authoritatively here, but I prefer indentation that aligns with the the column after the opening ( of the function call.

+    ErrorMsg(L"Could not open the EXE"); //GetLastError()
How does this get localized?

+  ReadBytes(hImage,
+    &image_dos_header,
+    sizeof(IMAGE_DOS_HEADER));
I recommend that you prefer sizeof(image_dos_header) over sizeof(IMAGE_DOS_HEADER) here (that is, use the variable itself rather than its explicit type), so that if the type of the variable changes at a later date, this code does not become a potentially serious overrun.  Check everywhere you're using sizeof() or a constant like IMAGE_SIZEOF_FILE_HEADER.

+  if (!ReadFile(hFile,
+    buffer,
+    size,
+    &bytes,
+    NULL))
NIT: Are multiple lines here even necessary?  You have other places in your patch where you have all parameters on one line, so I think it would be okay here.

+  _snwprintf(sShortcutPath, MAX_PATH, L"%s\\%s.lnk",
+    sProgramsPath, Strings.GetString(StrID_AppShortName));
Possible for sProgramsPath to be used uninitialized here?

+  BOOL bResult = CreateProcess(sCmdLine, L"-silent -nosplash",
+    NULL, NULL, FALSE, 0, NULL, NULL, NULL, &pi);
Do we actually want this to run and enable the faststart "daemon"?
(In reply to comment #50)
> +    ErrorMsg(L"Could not open the EXE"); //GetLastError()
> How does this get localized?

This part is the "loader", which gets the localized strings. These English-only messages are not supposed to be shown in normal conditions. They may show up only in some very bad and very unrealistic case when we cannot do anything with that.

> +  BOOL bResult = CreateProcess(sCmdLine, L"-silent -nosplash",
> +    NULL, NULL, FALSE, 0, NULL, NULL, NULL, &pi);
> Do we actually want this to run and enable the faststart "daemon"?

This is different from the faststart "daemon". It's just the first run, which creates a user profile. It is supposed to speed up the first actual start by the user - the profile will already exist at that moment.
Code review changes.
Attachment #406736 - Attachment is obsolete: true
Attachment #410690 - Flags: review?(crowder)
Attachment #406736 - Flags: review?(robert.bugzilla)
(In reply to comment #51)
> (In reply to comment #50)
> > Do we actually want this to run and enable the faststart "daemon"?
> 
> This is different from the faststart "daemon". It's just the first run, which
> creates a user profile. It is supposed to speed up the first actual start by
> the user - the profile will already exist at that moment.

Understood.  I'm just saying, we could actually make that first start -really- fast by leaving the daemon running, too.  Probably good fodder for a follow-up.
(In reply to comment #53)
> I'm just saying, we could actually make that first start -really-
> fast by leaving the daemon running, too.  Probably good fodder for a follow-up.

Well, this makes sense, especially because the user is very likely going to run the freshly installed application. But the user must have the ability to disable it.
We have to think about the fast-start daemon anyway - the EXE installer does not support that at all at the moment. And the daemon definitely has to be optional, so we'll probably need a pref or something.
Comment on attachment 410690 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

Looks good to me.  Thanks!
Attachment #410690 - Flags: review?(crowder) → review+
Keywords: checkin-needed
Three patches have to be applied:
"lib7z" - it's 7-zip library, has to be applied first;
"7z SFX Archive implementation" - actual installer, it depends on lib7z;
"Additional patch for the mobile tree" - contains changes for mobile makefiles and localization.

"7-zip binaries" - is optional, but better be applied as well to have complete LZMA (7-zip) SDK in our tree.
I'd rather not put binaries into the tree; is there a reason beyond completeness that we need the 7-zip binaries patch?
I think this is all in, and this bug has already represented more than its share of patches; we can follow-up in another bug regarding the 7-zip binaries, I think?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Binaries are not required for the installer itself - they are just a part of LZMA SDK. I guess we may skip them for now.
Can't they be built from source?
Comment on attachment 404907 [details] [diff] [review]
lib7z

This patch is necessary to enable the .exe installer for Windows Mobile Fennec.
Attachment #404907 - Flags: approval1.9.2?
Comment on attachment 410690 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

This patch is necessary to enable the .exe installer for Windows Mobile Fennec.
Attachment #410690 - Flags: approval1.9.2?
(In reply to comment #62)
> Can't they be built from source?

Hey! That's right!

Just run nmake in these directories:
...\LZMASDK\CPP\7zip\Bundles\Alone7z\
...\LZMASDK\CPP\7zip\Compress\LZMA_Alone\

So we don't really need the compiled binaries there.
Is there an estimated amount of drop off time from before this patch was landed to afterwards? Installing the .cab file is faster, but I'd like to get a definite time to verify against.
Here are my timings using HTC TouchPro and the latest nightly build:
CAB Installation: 1 minute 23 seconds
EXE Installation: 55 seconds, including the first silent run to create a user profile (just installation itself without running Fennec took about 31 seconds).

File sizes:
CAB Installer: 10736398 bytes
EXE Installer:  7456321 bytes
Comment on attachment 404907 [details] [diff] [review]
lib7z

a192=beltzner, good gracious, this is a lot of code.
Attachment #404907 - Flags: approval1.9.2? → approval1.9.2+
Attachment #410690 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 410690 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

a192=beltzner, do we need tests here?
We definitely need this installation process to be tested via litmus, if it is not already.
Flags: in-litmus?
Attachment #412930 - Flags: review?(bhearsum) → review+
FWIW, I'm not sure if dropping the zip file creation was a good choice here. Didn't realize that that was about to happen from following the bug either.
afaik, zip-file creation should still be possible...  is there some reason releng should continue generating them?
Not sure if this is a biggie for mobile devices, but zip's are usually easy to just put somewhere for QA purposes.

The other piece is that we need to repack the en-US builds, and it's not clear on #mobile that 7z x foo.exe works for that. If we can't unpack the exe, using the zip and creating an exe out of that similar to the way it's done in en-US would be the alternative route.
.zip file creation is addressed in the bug 529420.
Depends on: 529420
No longer depends on: 508721
The setup.ini file has multiple instances of "Fennec" hardcoded in it.

I guess that's bad when we rebrand it to Firefox eventually?
(In reply to comment #79)

If needed we can handle it the same way as done for updater.ini - have a place-holder and a script, which puts the actual product name there.
Blocks: 533581
No longer blocks: 533581
Could you clarify what the correct make command is with this new installer ? Currently the tinderboxes are doing 'make installer' in objdir/mobile and hitting "*** No rule to make target `installer'.  Stop."
(In reply to comment #82)
> Could you clarify what the correct make command is with this new installer ?

Weird.
'make installer' in objdir/mobile works for me, as well as 'make package'.
Nothing has changed in this respect - the same command is used as it was for the CAB installer.
It'll be fallout from bug 515748, I'll reopen that.
how can I turn this to verified?  Are we doing .cab files anymore?
This original bug was in fact converted to a new one, about the EXE-installer.
It probably makes sense just to change the description to something like "Replace slow CAB installer with self-extracting EXE", because this is what actually it is.
I think you can verify it with this new, more appropriate summary. The solution to the cab installation being slow was to not use it and develop our own self extracting installer.
Summary: Installation from a CAB is too slow → Installation takes way too long
Depends on: 533890
Component: Windows Mobile → General
QA Contact: mobile-windows → general
You need to log in before you can comment on or make changes to this bug.