Closed
Bug 778683
Opened 12 years ago
Closed 6 years ago
Clean up `Output()` function in nsMailApp.cpp on Windows
Categories
(Thunderbird :: Build Config, defect)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 982077
People
(Reporter: standard8, Unassigned)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file)
(deleted),
patch
|
jcranmer
:
review-
|
Details | Diff | Splinter Review |
+++ 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
Reporter | ||
Updated•12 years ago
|
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?
Updated•11 years ago
|
Whiteboard: [mentor=standard8][lang=cpp] → [mentor=standard8][lang=c++]
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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-
Comment 6•11 years ago
|
||
(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.
Comment 7•11 years ago
|
||
Flags: needinfo?(Pidgeot18)
Comment 8•11 years ago
|
||
*/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)
Assignee | ||
Updated•10 years ago
|
Mentor: standard8
Whiteboard: [mentor=standard8][lang=c++] → [lang=c++]
Reporter | ||
Updated•9 years ago
|
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)
Comment 10•8 years ago
|
||
3 years later, the patch here is likely obsolete.
Assignee: florian → nobody
Comment 11•8 years ago
|
||
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.
Comment 13•6 years ago
|
||
Fixed here: https://hg.mozilla.org/comm-central/rev/7ef9725513af#l1.23 in bug 982077.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•