Closed Bug 791238 Opened 8 years ago Closed 7 years ago

Clean up Output() function in nsSuiteApp.cpp on Windows (Use MultiByteToWideChar instead of NS_ConvertUTF8toUTF16)

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: ewong)

References

Details

(Whiteboard: [good first bug][lang=c++][mentor=Neil][difficulty=apprentice])

Attachments

(1 file, 3 obsolete files)

From Bug 762310 Comment 0:

> The `Output()` function in nsBrowserApp.cpp [1] attempts to call `NS_ConvertUTF8toUTF16` 
> which fails if XPCOM has not been initialized.  `Output()` is called in 2 locations 
> without XPCOM loaded: [2] and [3].  Since these are both error conditions, it is 
> unlikely that this will affect users, but it is definitely a bug as the intended error 
> dialogs will never be displayed.

http://mxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp?rev=0a10c274a1fe#45
http://mxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp?rev=0a10c274a1fe#186

We need to port this Firefox patch from Bug 762310:
https://hg.mozilla.org/mozilla-central/rev/f72108b85036

To our version of Output() is here:
http://mxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp?rev=0a10c274a1fe&mark=45-59#45

Also see TB Bug 778683 - Clean up `Output()` function in nsMailApp.cpp on Windows.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #661440 - Flags: review?(neil)
Comment on attachment 661440 [details] [diff] [review]
Clean up Output() function in nsSuiteapp.cpp on Windows. (v1)

>+#ifndef XP_WIN
>+  vfprintf(stderr, fmt, ap);
> #else
>-  vfprintf(stderr, fmt, ap);
[Bah, this is what happens when you swap around the if and else parts.]

>-  MessageBoxW(NULL, msg, L"XULRunner", MB_OK | MB_ICONERROR);
>+  MessageBoxW(NULL, wide_msg, L"SeaMonkey", MB_OK
[Not sure changing the title string is correct. Will comment on original bug.]
Attachment #661440 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #2)
> Comment on attachment 661440 [details] [diff] [review]
> Clean up Output() function in nsSuiteapp.cpp on Windows. (v1)
> 
> >+#ifndef XP_WIN
> >+  vfprintf(stderr, fmt, ap);
> > #else
> >-  vfprintf(stderr, fmt, ap);
> [Bah, this is what happens when you swap around the if and else parts.]

Did I miss something?  What do I need to change here?

> 
> >-  MessageBoxW(NULL, msg, L"XULRunner", MB_OK | MB_ICONERROR);
> >+  MessageBoxW(NULL, wide_msg, L"SeaMonkey", MB_OK
> [Not sure changing the title string is correct. Will comment on original
> bug.]

So this should stay |L"XULRunner"|?
> So this should stay |L"XULRunner"|
IMO For the time being let's leave this as |L"XULRunner"| and get this landed.

Once Bug 777974 lands we can remove our implementation of Output() and use the one in Core.
Summary: Clean up Output() function in nsMailApp.cpp on Windows (Use MultiByteToWideChar instead of NS_ConvertUTF8toUTF16) → Clean up Output() function in nsSuiteApp.cpp on Windows (Use MultiByteToWideChar instead of NS_ConvertUTF8toUTF16)
Attachment #661440 - Attachment is obsolete: true
Attachment #669399 - Flags: review?(neil)
Sorry for the delay.

(In reply to Edmund Wong from comment #3)
> (In reply to comment #2)
> > (From update of attachment 661440 [details] [diff] [review])
> > >+#ifndef XP_WIN
> > >+  vfprintf(stderr, fmt, ap);
> > > #else
> > >-  vfprintf(stderr, fmt, ap);
> > [Bah, this is what happens when you swap around the if and else parts.]
> Did I miss something?  What do I need to change here?
Well, ideally you would switch over the the two halves and switch it to an #ifdef.

> > >-  MessageBoxW(NULL, msg, L"XULRunner", MB_OK | MB_ICONERROR);
> > >+  MessageBoxW(NULL, wide_msg, L"SeaMonkey", MB_OK
> > [Not sure changing the title string is correct. Will comment on original bug.]
> So this should stay |L"XULRunner"|?
For now, at least, thanks.
Attachment #669399 - Attachment is obsolete: true
Attachment #669399 - Flags: review?(neil)
Attachment #681817 - Flags: review?(neil)
Comment on attachment 681817 [details] [diff] [review]
Clean up Output() function in nsSuiteapp.cpp on Windows. (v3)

>+#ifdef XP_WIN
>+  vfprintf(stderr, fmt, ap);
> #else
>+  char msg[2048];
>+
>+  vsnprintf_s(msg, _countof(msg), _TRUNCATE, fmt, ap);
>+
>+  wchar_t wide_msg[2048];
>+  MultiByteToWideChar(CP_UTF8, 0, msg, -1, wide_msg, _countof(wide_msg));
>+
>+#if MOZ_WINCONSOLE
>+  fwprintf_s(stderr, wide_msg);
>+#else
>+  MessageBoxW(NULL, wide_msg, L"XULRunner", MB_OK
>+                                          | MB_ICONERROR
>+                                          | MB_SETFOREGROUND);
>+#endif
> #endif
You misunderstood the bit about the #else - it belongs between the two #endif above and the vfprintf belongs after it.
Attachment #681817 - Flags: review?(neil) → review-
Attachment #681817 - Attachment is obsolete: true
Attachment #683842 - Flags: review?(neil)
Comment on attachment 683842 [details] [diff] [review]
Clean up Output() function in nsSuiteApp.cpp on Windows. (v4)

Excellent, this has helped me find two unrelated bugs!
Attachment #683842 - Flags: review?(neil) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/99fd4744910d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.