Change the system-level EULA dialog to not use GET parameters with res:// urls. Instead use the d...

Change the system-level EULA dialog to not use GET parameters with res:// urls. Instead use the dialogArgument property on the window object. res:// urls with GET parameters don't appear to work with the IE6 version of MSHTML.

BUG=88192
TEST=Install system-level Chrome on a new machine with a master_preferences file that includes "require_eula":true. Observe that a non-blank EULA dialog shows up and that the user can run Chrome.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91507

[grt (UTC plus 2), 2011-07-04 18:42:04]

lookin' good.  just some small comments.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/setup/eula/...
File chrome/installer/setup/eula/oem.js (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/setup/eula/...
chrome/installer/setup/eula/oem.js:4: document.getElementById('ifr').src =
inner_frame;
why not something like:
if (inner_frame && inner_frame.length != 0)
  document.getElementById('ifr').src = inner_frame;

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
File chrome/installer/util/html_dialog.h (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog.h:73: explicit EulaHTMLDialog(const
std::wstring& file,
please remove "explicit" since this is no longer a one-arg ctor and add a
description of |param| to the comment.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
File chrome/installer/util/html_dialog_impl.cc (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:6: 
remove blank line?

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:44: explicit HTMLDialogWin(const
std::wstring& url,
remove "explicit"

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:130: base::win::ScopedVariant
dialog_args(param_.c_str());
nit: dialog_args(param_.data(), param_.size()) will avoid an extra call to
strlen.

[robertshield, 2011-07-04 19:12:31]

Thanks, PTAL

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/setup/eula/...
File chrome/installer/setup/eula/oem.js (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/setup/eula/...
chrome/installer/setup/eula/oem.js:4: document.getElementById('ifr').src =
inner_frame;
On 2011/07/04 18:42:04, grt wrote:
> why not something like:
> if (inner_frame && inner_frame.length != 0)
>   document.getElementById('ifr').src = inner_frame;

Good idea, the empty string evaluates to false, so I can do just if
(inner_frame) methinks.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
File chrome/installer/util/html_dialog.h (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog.h:73: explicit EulaHTMLDialog(const
std::wstring& file,
On 2011/07/04 18:42:04, grt wrote:
> please remove "explicit" since this is no longer a one-arg ctor and add a
> description of |param| to the comment.

Done.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
File chrome/installer/util/html_dialog_impl.cc (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:6: 
On 2011/07/04 18:42:04, grt wrote:
> remove blank line?

Done.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:44: explicit HTMLDialogWin(const
std::wstring& url,
On 2011/07/04 18:42:04, grt wrote:
> remove "explicit"

Done.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:130: base::win::ScopedVariant
dialog_args(param_.c_str());
On 2011/07/04 18:42:04, grt wrote:
> nit: dialog_args(param_.data(), param_.size()) will avoid an extra call to
> strlen.

It would avoid an extra call, but it feels like premature optimization and (you
can correct me if I'm wrong) I'm not sure it is guaranteed to work for empty
strings: basic_string::data() is documented as returning for an empty string "a
non-null pointer that cannot be dereferenced" and I'm unsure if there are any
guarantees as to how SysAllocStringLen() called with length 0 would deal with
that. 

I'd prefer to leave this as is if you don't strongly object since I feel the
above is clearer, with less stuff to worry about and if MSHTML dialogs are ever
created in bulk, then the extra strlen() call will not be the source of our
performance concerns.

[grt (UTC plus 2), 2011-07-04 19:24:19]

LGTM with one style nit.

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
File chrome/installer/util/html_dialog_impl.cc (right):

http://codereview.chromium.org/7309008/diff/5001/chrome/installer/util/html_d...
chrome/installer/util/html_dialog_impl.cc:130: base::win::ScopedVariant
dialog_args(param_.c_str());
On 2011/07/04 19:12:31, robertshield wrote:
> On 2011/07/04 18:42:04, grt wrote:
> > nit: dialog_args(param_.data(), param_.size()) will avoid an extra call to
> > strlen.
> 
> It would avoid an extra call, but it feels like premature optimization and
(you
> can correct me if I'm wrong) I'm not sure it is guaranteed to work for empty
> strings: basic_string::data() is documented as returning for an empty string
"a
> non-null pointer that cannot be dereferenced" and I'm unsure if there are any
> guarantees as to how SysAllocStringLen() called with length 0 would deal with
> that. 

SysAllocStringLen is broken if it reads memory beyond what it's told.  You could
use c_str() rather than data() if that's expected.  A better move would be to
add a const std::wstring& overload to ScopedVariant that did the right thing,
but I'm 99.9999% positive you don't want to do that in this CL.  :-)

> I'd prefer to leave this as is if you don't strongly object since I feel the
> above is clearer, with less stuff to worry about and if MSHTML dialogs are
ever
> created in bulk, then the extra strlen() call will not be the source of our
> performance concerns.

Works for me.

http://codereview.chromium.org/7309008/diff/6/chrome/installer/util/html_dial...
chrome/installer/util/html_dialog_impl.cc:44: explicit HTMLDialogWin(const
std::wstring& url,
Google C++ style guide says to put the colon on this line indented four spaces.

[robertshield, 2011-07-04 19:45:10]

Thanks!

http://codereview.chromium.org/7309008/diff/6/chrome/installer/util/html_dial...
File chrome/installer/util/html_dialog_impl.cc (right):

http://codereview.chromium.org/7309008/diff/6/chrome/installer/util/html_dial...
chrome/installer/util/html_dialog_impl.cc:44: url_(url), param_(param) {
On 2011/07/04 19:24:19, grt wrote:
> Google C++ style guide says to put the colon on this line indented four
spaces.

Done.

[commit-bot: I haz the power, 2011-07-04 19:45:28]

Can't apply patch for file chrome/installer/setup/eula/oem.js.
While running patch -p0 --forward --force;
patching file chrome/installer/setup/eula/oem.js
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/installer/setup/eula/oem.js.rej