Closed Bug 878208 Opened 6 years ago Closed 6 years ago

first param passed metrotestharness gets ignored by the browser

Categories

(Firefox for Metro Graveyard :: Tests, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch patch v.1 (obsolete) — Splinter Review
This fixes the path bug and adds some fx path validation / logging improvements to make this a bit more fail safe for qa testers.
Attachment #756768 - Flags: review?(netzen)
Attached patch patch v.1Splinter Review
added a Trim call on asciiParams.
Attachment #756768 - Attachment is obsolete: true
Attachment #756768 - Flags: review?(netzen)
Attachment #756776 - Flags: review?(netzen)
Blocks: 845079
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()
Attachment #756776 - Flags: review?(netzen) → review+
(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. :)
Attached patch export patch (obsolete) — Splinter Review
Keywords: checkin-needed
Attached patch export patch (obsolete) — Splinter Review
Attachment #757296 - Attachment is obsolete: true
Attached patch export patchSplinter Review
Attachment #757297 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6c263ecf2beb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.