Wire the eula and the dialog display code...

Wire the eula and the dialog display code
- html, css and js to live in the installer as resources. The right combo of resources is selected based on system lang
- currently the approach is to have chrome show the eula by spawing setup.exe --show-eula

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

[kuchhal, 2009-01-26 18:32:59]

I have not looked at the complete change but it looks like this change need to
be broken into two changes. The icon will need to go in our internal repository
since it is specific to Google Chrome and not open source. Not sure about the
eula itself and the terms. But I am guessing they need to be in internal
repository too.

[cpu_(ooo_6.6-7.5), 2009-01-26 19:41:18]

It seems the only issue is the icon, I see the eulas being in the public repo:

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/resources/terms/


On 2009/01/26 18:32:59, kuchhal wrote:
> I have not looked at the complete change but it looks like this change need to
> be broken into two changes. The icon will need to go in our internal
repository
> since it is specific to Google Chrome and not open source. Not sure about the
> eula itself and the terms. But I am guessing they need to be in internal
> repository too.

[cpu_(ooo_6.6-7.5), 2009-01-26 21:35:31]

I have removed the terms html and now I am re-using the terms that are already
in app/resources/terms

I'll take the icon next.


On 2009/01/26 19:41:18, cpu wrote:
> It seems the only issue is the icon, I see the eulas being in the public repo:
> 
> http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/resources/terms/
> 
> 
> On 2009/01/26 18:32:59, kuchhal wrote:
> > I have not looked at the complete change but it looks like this change need
to
> > be broken into two changes. The icon will need to go in our internal
> repository
> > since it is specific to Google Chrome and not open source. Not sure about
the
> > eula itself and the terms. But I am guessing they need to be in internal
> > repository too.

[cpu_(ooo_6.6-7.5), 2009-01-26 23:07:12]

Now the icon is in app/theme and is part of the private repository.

Put all the resources under an #ifdef for chrome builds

please look again


On 2009/01/26 21:35:31, cpu wrote:
> I have removed the terms html and now I am re-using the terms that are already
> in app/resources/terms
> 
> I'll take the icon next.
> 
> 
> On 2009/01/26 19:41:18, cpu wrote:
> > It seems the only issue is the icon, I see the eulas being in the public
repo:
> > 
> > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/resources/terms/
> > 
> > 
> > On 2009/01/26 18:32:59, kuchhal wrote:
> > > I have not looked at the complete change but it looks like this change
need
> to
> > > be broken into two changes. The icon will need to go in our internal
> > repository
> > > since it is specific to Google Chrome and not open source. Not sure about
> the
> > > eula itself and the terms. But I am guessing they need to be in internal
> > > repository too.

[huanr, 2009-01-26 23:09:11]

OK with one change needed.

http://codereview.chromium.org/18732/diff/62/264
File chrome/installer/setup/main.cc (right):

http://codereview.chromium.org/18732/diff/62/264#newcode499
Line 499: } else if (installer_util::MASTER_PROFILE_REQUIRE_EULA & options) {
you need to change GetInstallOptions() in this file accordingly.

[kuchhal, 2009-01-26 23:23:31]

lgtm after you make the change in GetInstallOptions like Huan said. Some other
comments below.

http://codereview.chromium.org/18732/diff/62/264
File chrome/installer/setup/main.cc (right):

http://codereview.chromium.org/18732/diff/62/264#newcode446
Line 446: if (eula_path.empty())
This is probably serious error too. Add a log message?

http://codereview.chromium.org/18732/diff/62/263
File chrome/installer/setup/setup.rc (right):

http://codereview.chromium.org/18732/diff/62/263#newcode60
Line 60: #if defined(GOOGLE_CHROME_BUILD)
What would happen if --show-eula command line is passed to chromium builds? Is
there an easy way to handle this or exit  setup.exe gracefully?

http://codereview.chromium.org/18732/diff/62/263#newcode79
Line 79: IDR_TERMS_ES.HTML     HTML  
"..\\..\\app\\resources\\terms\\terms_es.html"
Doesn't looks like you are using any language other than en since oem_en.html
only refers to terms_en.html?

http://codereview.chromium.org/18732/diff/62/265
File chrome/installer/util/l10n_string_util.cc (right):

http://codereview.chromium.org/18732/diff/62/265#newcode168
Line 168: resource = L"IDR_OEMPG_FR.HTML";
Are you going to add these resources later?

[cpu_(ooo_6.6-7.5), 2009-01-27 00:42:43]

thanks for the review.

http://codereview.chromium.org/18732/diff/62/264
File chrome/installer/setup/main.cc (right):

http://codereview.chromium.org/18732/diff/62/264#newcode446
Line 446: if (eula_path.empty())
On 2009/01/26 23:23:32, kuchhal wrote:
> This is probably serious error too. Add a log message?

Done.

http://codereview.chromium.org/18732/diff/62/264#newcode499
Line 499: } else if (installer_util::MASTER_PROFILE_REQUIRE_EULA & options) {
On 2009/01/26 23:09:11, huanr wrote:
> you need to change GetInstallOptions() in this file accordingly.

Done.

http://codereview.chromium.org/18732/diff/62/263
File chrome/installer/setup/setup.rc (right):

http://codereview.chromium.org/18732/diff/62/263#newcode60
Line 60: #if defined(GOOGLE_CHROME_BUILD)
It shows an empty eula dialog. I sort of like it this way.
I assume there would be no reason for having this option turned on a chromium
build.


On 2009/01/26 23:23:32, kuchhal wrote:
> What would happen if --show-eula command line is passed to chromium builds? Is
> there an easy way to handle this or exit  setup.exe gracefully?

http://codereview.chromium.org/18732/diff/62/263#newcode79
Line 79: IDR_TERMS_ES.HTML     HTML  
"..\\..\\app\\resources\\terms\\terms_es.html"
Yes we are waiting for the oem_xx files from the translation folks.


On 2009/01/26 23:23:32, kuchhal wrote:
> Doesn't looks like you are using any language other than en since oem_en.html
> only refers to terms_en.html?

[huanr, 2009-01-27 06:19:16]

A comment regarding your latest change.

http://codereview.chromium.org/18732/diff/284/73
File chrome/installer/setup/main.cc (right):

http://codereview.chromium.org/18732/diff/284/73#newcode233
Line 233: options |= installer_util::MASTER_PROFILE_REQUIRE_EULA;
There are two sets of constants, MasterPrefResult defined in master_preference.h
used for preferences, and InstallOption defined in util_constants.h used for
options. It is better to add an installer_util::REQUIRE_EULA in util_cosntants.h
and change the code to:

if (preferences & installer_util::MASTER_PROFILE_REQUIRE_EULA)
  options |= installer_util::REQUIRE_EULA;

This way we don't have potential risk of collision if a new setting is added.

http://codereview.chromium.org/18732/diff/284/73#newcode505
Line 505: } else if (installer_util::MASTER_PROFILE_REQUIRE_EULA & options) {
Same change here.