878208 - first param passed metrotestharness gets ignored by the browser

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Tests, defect)

Description

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp#324

app runner always ignores the first parameter passed making the assumption that it's the exe path. We don't append this to the args we pass to XRE_main though, so the first param always gets ignored.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

This fixes the path bug and adds some fx path validation / logging improvements to make this a bit more fail safe for qa testers.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

added a Trim call on asciiParams.

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 756776 [details] [diff] [review]
patch v.1

Review of attachment 756776 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/shell/testing/metrotestharness.cpp
@@ +232,5 @@
> +  int binLen = wcslen(kFirefoxExe);
> +  if (sFirefoxPath.GetLength() && sFirefoxPath.Right(binLen) != kFirefoxExe) {
> +    Log(L"firefoxpath is missing a valid bin name! Assuming '%s'.", kFirefoxExe);
> +    if (sFirefoxPath.Right(1) != "\\") {
> +      sFirefoxPath += "\\";

nit: L"\\" here and prev line

@@ +294,5 @@
> +  CStringA asciiParams = sFirefoxPath;
> +  asciiParams += " ";
> +  asciiParams += sAppParams;
> +  asciiParams.Trim();
> +  Log(L"Browser command line args: '%s'", CString(asciiParams));

I'm not sure if this implicitly converts to wchar_t* for va_start?
I think you need CString(asciiParams).GetBuffer() or .GetString()

Comment 4

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Comment on attachment 756776 [details] [diff] [review]
> patch v.1
> 
> Review of attachment 756776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/shell/testing/metrotestharness.cpp
> @@ +232,5 @@
> > +  int binLen = wcslen(kFirefoxExe);
> > +  if (sFirefoxPath.GetLength() && sFirefoxPath.Right(binLen) != kFirefoxExe) {
> > +    Log(L"firefoxpath is missing a valid bin name! Assuming '%s'.", kFirefoxExe);
> > +    if (sFirefoxPath.Right(1) != "\\") {
> > +      sFirefoxPath += "\\";
> 
> nit: L"\\" here and prev line
> 
> @@ +294,5 @@
> > +  CStringA asciiParams = sFirefoxPath;
> > +  asciiParams += " ";
> > +  asciiParams += sAppParams;
> > +  asciiParams.Trim();
> > +  Log(L"Browser command line args: '%s'", CString(asciiParams));
> 
> I'm not sure if this implicitly converts to wchar_t* for va_start?
> I think you need CString(asciiParams).GetBuffer() or .GetString()

It does. Which is one of the reasons why I wish we had CString in platform code. :)

Comment 5

[Jim Mathies [:jimm]]

Attachment: export patch

Comment 6

[Jim Mathies [:jimm]]

Attachment: export patch

Comment 7

[Jim Mathies [:jimm]]

Attachment: export patch

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c263ecf2beb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c263ecf2beb