Closed Bug 778683 Opened 10 years ago Closed 3 years ago

Clean up `Output()` function in nsMailApp.cpp on Windows

Categories

(Thunderbird :: Build Config, defect)

All
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 982077

People

(Reporter: standard8, Unassigned)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #762310 +++

(We should port this to Thunderbird)

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.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp?rev=a15d75939cd5#40
[2] https://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp?rev=a15d75939cd5#200
[3] https://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp?rev=a15d75939cd5#200
Sorry, can't build on Windows.
Summary: Clean up `Output()` function in nsBrowserApp.cpp on Windows → Clean up `Output()` function in nsMailApp.cpp on Windows
Whiteboard: [mentor=standard8][lang=cpp]
Hi,
I would like to work on this bug,but I am new to work on debug job, and would be grateful if this bug could be assigned to me.How can I start with it?
Sorry, I found I can't build on Windows too.
Whiteboard: [mentor=standard8][lang=cpp] → [mentor=standard8][lang=c++]
I can't build on Windows either (right now), but I don't think that's a good enough reason to wait any longer. If anybody is uncomfortable with this patch (which is a straight port), I'm happy to push it to try.
Assignee: nobody → florian
Attachment #8378169 - Flags: review?(Pidgeot18)
Comment on attachment 8378169 [details] [diff] [review]
Patch for mail/ and im/

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

Unless there's a few things hiding in macros I don't see, all of the strings involved in passing to this function are const char literals. This means:

A. we don't need to use varargs and printf-like methods.
B. if we reformat into a macro, we can make the Windows variant use an L"" version of the string instead of the "" version.
Attachment #8378169 - Flags: review?(Pidgeot18) → review-
(In reply to Joshua Cranmer [:jcranmer] from comment #5)

> Unless there's a few things hiding in macros I don't see, all of the strings
> involved in passing to this function are const char literals.

This is true for nsMailApp.cpp, but not for browser/app/nsBrowserApp.cpp where there are at least 3 Output() calls with a string parameter (%s).

While I agree that the code touched by this patch is more complicated than it strictly needs to be for c-c (both before and after the patch), I would favor not forking code that we don't have to, for the ease of future ports.
 Joshua, does comment 6 change your opinion about attachment 8378169 [details] [diff] [review]?
Flags: needinfo?(Pidgeot18)
*/app is already forked up the wazoo in the first place, and the diff between nsMailApp.cpp and nsBrowserApp.cpp is extremely large. Given that Firefox's main wrapper supports features we are not likely to support anytime soon (Metro, -app), I don't think there's a particularly strong benefit to making sure the code is identical. Given that the %s parameter is used solely for the -app feature, it's not clear to me that keeping the code paths strictly identical is necessary.
Flags: needinfo?(Pidgeot18)
Mentor: standard8
Whiteboard: [mentor=standard8][lang=c++] → [lang=c++]
Mentor: standard8
Magnus, can you decide the dilemma here? Optimized code for c-c or just keep parity with m-c version of the file?
Flags: needinfo?(mkmelin+mozilla)
3 years later, the patch here is likely obsolete.
Assignee: florian → nobody
In other bugs we have been favoring m-c parity, I think that is what we should do here. I think we did some nsMailApp porting recently, so this could be WFM.
Agreed keeping m-c parity is preferable.
Flags: needinfo?(mkmelin+mozilla)
Fixed here: https://hg.mozilla.org/comm-central/rev/7ef9725513af#l1.23 in bug 982077.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 982077
You need to log in before you can comment on or make changes to this bug.