Switch out the setup api for using the cabinet API directly....

Switch out the setup api for using the cabinet API directly.
The setup API and tools such as expand.exe and makecab.exe all use the cabinet API.
However, the setup API has additional dependencies that we don't need or want
in the mini installer.  So, this is a part of a pet pieve effort of mine to
remove these dependencies and hopefully reduce the number of installation failures.

TEST=Run accompanied unit test and also make sure that the mini installer works as before.
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78434

[grt (UTC plus 2), 2011-03-15 14:34:10]

Hi Tommi.  Please add chrome/installer/mini_installer/decompress.h and
re-upload.

[grt (UTC plus 2), 2011-03-15 14:34:35]

Same for decompress_test.cc.

[tommi (sloooow) - chröme, 2011-03-15 14:45:24]

thanks greg - done.

(I wish svn wouldn't refuse to add files with \r\n but just fix it!)

On Tue, Mar 15, 2011 at 10:34 AM, <grt@chromium.org> wrote:

> Same for decompress_test.cc.
>
>
> http://codereview.chromium.org/6693015/
>

[amit, 2011-03-15 20:54:35]

lg with a few nits

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.cc (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:21: char* WideToUtf8(const
wchar_t* str) {
nit: we probably need mini_installer_utils.h/cc for these?

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:181: g_fdi =
::LoadLibraryW(L"cabinet.dll");
shouldn't this be loaded from a known path, to prevent someone from sneaking in
malicious cabinet.dll? Maybe delay load it?

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.h (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.h:17: bool Expand(wchar_t* source,
const wchar_t* destination);
if source is writable, why not take in the source size as well?

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress_test.cc (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress_test.cc:21:
TEST_F(MiniDecompressTest, ExpandTest) {
cool test

[tommi (sloooow) - chröme, 2011-03-16 02:43:30]

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.cc (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:21: char* WideToUtf8(const
wchar_t* str) {
On 2011/03/15 20:54:35, amit wrote:
> nit: we probably need mini_installer_utils.h/cc for these?

Yes, eventually.  However right now, we don't need it and these implementation
do depend on the alloc and free functions that I'm using for the cabinet api.
Adding more files also requires more p4 changes, so I'd like to punt :)

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:181: g_fdi =
::LoadLibraryW(L"cabinet.dll");
On 2011/03/15 20:54:35, amit wrote:
> shouldn't this be loaded from a known path, to prevent someone from sneaking
in
> malicious cabinet.dll? Maybe delay load it?

We don't have a lib for cabinet.dll but this is effectively the same as delay
loading the dll.
What do you think about using %WINDIR%\\system32\\cabinet.dll? (and
ExpandEnvironmentStrings)

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.h (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.h:17: bool Expand(wchar_t* source,
const wchar_t* destination);
On 2011/03/15 20:54:35, amit wrote:
> if source is writable, why not take in the source size as well?

The idea is similar to how CreateProcess works.  The string must be a valid path
and as such it must be zero terminated.  For sake of simplicity, I chose to have
it writable buffer and avoid creating extra buffers.

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress_test.cc (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress_test.cc:21:
TEST_F(MiniDecompressTest, ExpandTest) {
On 2011/03/15 20:54:35, amit wrote:
> cool test

thanks :)

[amit, 2011-03-16 15:50:26]

lg++

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.cc (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:21: char* WideToUtf8(const
wchar_t* str) {
On 2011/03/16 02:43:30, tommi wrote:
> On 2011/03/15 20:54:35, amit wrote:
> > nit: we probably need mini_installer_utils.h/cc for these?
> 
> Yes, eventually.  However right now, we don't need it and these implementation
> do depend on the alloc and free functions that I'm using for the cabinet api.
> Adding more files also requires more p4 changes, so I'd like to punt :)

That's reasonable.

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:181: g_fdi =
::LoadLibraryW(L"cabinet.dll");
On 2011/03/16 02:43:30, tommi wrote:
> On 2011/03/15 20:54:35, amit wrote:
> > shouldn't this be loaded from a known path, to prevent someone from sneaking
> in
> > malicious cabinet.dll? Maybe delay load it?
> 
> We don't have a lib for cabinet.dll but this is effectively the same as delay
> loading the dll.
> What do you think about using %WINDIR%\\system32\\cabinet.dll? (and
> ExpandEnvironmentStrings)

sounds good

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.h (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.h:17: bool Expand(wchar_t* source,
const wchar_t* destination);
On 2011/03/16 02:43:30, tommi wrote:
> On 2011/03/15 20:54:35, amit wrote:
> > if source is writable, why not take in the source size as well?
> 
> The idea is similar to how CreateProcess works.  The string must be a valid
path
> and as such it must be zero terminated.  For sake of simplicity, I chose to
have
> it writable buffer and avoid creating extra buffers.

Probably OK but CreateProcess has caused enough pain due to this.
OTOH, if you create a version of WideToUTF8 that takes source length, you won't
need to modify the source arg.

[tommi (sloooow) - chröme, 2011-03-16 20:22:19]

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.cc (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.cc:181: g_fdi =
::LoadLibraryW(L"cabinet.dll");
On 2011/03/16 15:50:26, amit wrote:
> On 2011/03/16 02:43:30, tommi wrote:
> > On 2011/03/15 20:54:35, amit wrote:
> > > shouldn't this be loaded from a known path, to prevent someone from
sneaking
> > in
> > > malicious cabinet.dll? Maybe delay load it?
> > 
> > We don't have a lib for cabinet.dll but this is effectively the same as
delay
> > loading the dll.
> > What do you think about using %WINDIR%\\system32\\cabinet.dll? (and
> > ExpandEnvironmentStrings)
> 
> sounds good

Done.

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress.h (right):

http://codereview.chromium.org/6693015/diff/6001/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress.h:17: bool Expand(wchar_t* source,
const wchar_t* destination);
On 2011/03/16 15:50:26, amit wrote:
> On 2011/03/16 02:43:30, tommi wrote:
> > On 2011/03/15 20:54:35, amit wrote:
> > > if source is writable, why not take in the source size as well?
> > 
> > The idea is similar to how CreateProcess works.  The string must be a valid
> path
> > and as such it must be zero terminated.  For sake of simplicity, I chose to
> have
> > it writable buffer and avoid creating extra buffers.
> 
> Probably OK but CreateProcess has caused enough pain due to this.
> OTOH, if you create a version of WideToUTF8 that takes source length, you
won't
> need to modify the source arg.

of course (doh)! done!

[M-A Ruel, 2011-06-24 12:40:22]

http://codereview.chromium.org/6693015/diff/9024/chrome/installer/mini_instal...
File chrome/installer/mini_installer/decompress_test.cc (right):

http://codereview.chromium.org/6693015/diff/9024/chrome/installer/mini_instal...
chrome/installer/mini_installer/decompress_test.cc:13: class MiniDecompressTest
: public testing::Test {
Please do not use empty fixture class! Someone on irc just took this code as an
example but this is clearly a bad example to follow. Please clean up.

Remove lines 13-19.
On line 21: s/TEST_F/TEST/.

You may want to read a bit more about google-test at
http://code.google.com/p/googletest/wiki/Primer.

[tommi (sloooow) - chröme, 2011-06-27 09:36:37]

Thanks MA - I'm settling in for a couple of days probably but will get this
done this week.  Let me know if it's more urgent and I can make time.

On Fri, Jun 24, 2011 at 8:40 AM, <maruel@chromium.org> wrote:

>
> http://codereview.chromium.**org/6693015/diff/9024/chrome/**
>
installer/mini_installer/**decompress_test.cc<http://codereview.chromium.org/6693015/diff/9024/chrome/installer/mini_installer/decompress_test.cc>
> File chrome/installer/mini_**installer/decompress_test.cc (right):
>
> http://codereview.chromium.**org/6693015/diff/9024/chrome/**
>
installer/mini_installer/**decompress_test.cc#newcode13<http://codereview.chromium.org/6693015/diff/9024/chrome/installer/mini_installer/decompress_test.cc#newcode13>
> chrome/installer/mini_**installer/decompress_test.cc:**13: class
> MiniDecompressTest : public testing::Test {
> Please do not use empty fixture class! Someone on irc just took this
> code as an example but this is clearly a bad example to follow. Please
> clean up.
>
> Remove lines 13-19.
> On line 21: s/TEST_F/TEST/.
>
> You may want to read a bit more about google-test at
>
http://code.google.com/p/**googletest/wiki/Primer<http://code.google.com/p/go...
> .
>
>
http://codereview.chromium.**org/6693015/<http://codereview.chromium.org/6693...
>

[Marc-Antoine Ruel (Google), 2011-06-27 13:00:32]

On 2011/06/27 09:36:37, tommi wrote:
> Thanks MA - I'm settling in for a couple of days probably but will get this
> done this week.  Let me know if it's more urgent and I can make time.

I don't mind as long it's done within the next month. Have fun!