Closed Bug 241145 Opened 20 years ago Closed 20 years ago

nsIPrintSettings should not include nsFont.h

Categories

(Core Graveyard :: GFX, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mpgritti, Assigned: mpgritti)

Details

(Keywords: fixed1.7)

Attachments

(2 files, 3 obsolete files)

It's mentioned in http://www.mozilla.org/projects/embedding/apiReviewNotes.html
The more urgent problem is that nsFont.h drag in nsString.h. Currently it's not
possible to use embed strings with the print api.
I'll try provide a patch that removes it. The problem is that not building the
whole mozilla code there is a chance to break the build because of missing includes.
Caillon was suggesting to move the NS_GFX macros in gfx dir instead of adding
yet another nsComObsolete.h inclusion.
Similarly to what is done in msgCore.h, I was thinking to add a gfxCore.h with
#include "nscore.h" and the macros, then modify gfx headers to use it. I couldnt
find any other existing file suitable to host these macros...
Would this work ?
Comment on attachment 146642 [details] [diff] [review]
remove the unused include

Marco, I think that would be swell, though if you want this for 1.7 (which
seems both reasonable and likely), I say go with this patch on branch only, and
let's do the better fix for trunk.

r=caillon
Attachment #146642 - Flags: review+
Comment on attachment 146642 [details] [diff] [review]
remove the unused include

Yeah it seem a good idea to get a fix in for 1.7 to me. The only risk is
possible include fallouts (it works ok on my linux gtk2 build but ...) though
tinderboxes should easily address that.
Attachment #146642 - Flags: superreview?(dbaron)
Attachment #146642 - Flags: approval1.7?
Attachment #146642 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 146642 [details] [diff] [review]
remove the unused include

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #146642 - Flags: approval1.7? → approval1.7+
I dont have cvs access, so if someone could check it in I'd appreciate ;)
Landed on the 1.7 branch.
Keywords: fixed1.7
Note, I landed a bustage fix to
embedding/tests/mfcembed/components/PrintProgressDialog.cpp on the branch as well.
Attached patch better fix for trunk (obsolete) — Splinter Review
Attachment #147446 - Flags: review?(caillon)
Comment on attachment 147446 [details] [diff] [review]
better fix for trunk

This looks good.  Do you also want to do widget while you're at it and kill off
nsComObsolete.h altogether?
Yeah. There are a few more places where nsComObsolete.h is included according to
lxr, but I can deal with them.
Prolly we should try to get someone to test it on win32 too though, or we risk
some bad build bustage ...
Comment on attachment 147446 [details] [diff] [review]
better fix for trunk

Removing request for now.  Please come back when you figure out what you are
doing with widget, and testing on other platforms (find someone on #mozillazine
perhaps for platforms you do not have access to) would be nice.
Attachment #147446 - Flags: review?(caillon)
Attached patch Kill off nsComObsolete.h (obsolete) — Splinter Review
I'll see if I can setup a windows build here and get someone to test on Mac.
Attachment #147446 - Attachment is obsolete: true
Comment on attachment 147714 [details] [diff] [review]
Kill off nsComObsolete.h

I tested this on linux gtk2 and windows mingw. I have not been able to find
someone to test on mac so far (will retry).
Attachment #147714 - Flags: review?(caillon)
Comment on attachment 147714 [details] [diff] [review]
Kill off nsComObsolete.h

>-NS_BEGIN_EXTERN_C
>+#ifdef __cplusplus
>+extern "C" {
>+#endif

>-NS_END_EXTERN_C
>+#ifdef __cplusplus
>+}
>+#endif


All of these should become PR_{BEGIN|END}_EXTERN_C

The rest appears fine, but please clear it with dbaron and maybe even
blizzard/bryner.
Ok, ccing blizzard and bryner. Maybe they have comment on the nsComObsolete.h
killoff. Will ask sr to dbaron.
Attached patch use PR_BEGIN/END_EXTERN_C (obsolete) — Splinter Review
Attachment #147714 - Attachment is obsolete: true
Attachment #148119 - Flags: superreview?(dbaron)
Attachment #148119 - Flags: review?(caillon)
Attachment #147714 - Flags: review?(caillon)
Attachment #148119 - Attachment is obsolete: true
Attachment #148119 - Flags: superreview?(dbaron)
Attachment #148119 - Flags: review?(caillon)
Attachment #148120 - Flags: superreview?(dbaron)
Attachment #148120 - Flags: review?(caillon)
Attachment #148120 - Flags: superreview?(dbaron) → superreview+
Attachment #148120 - Flags: review?(caillon) → review+
attachment 148120 [details] [diff] [review] checked into trunk 2004-05-18 11:09 PDT.

Marco, is anything left to be done here?
Assignee: general → marco
No, I think this is fixed now. Thanks!
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Do any of the compilers we still want to support not support anonymous namespaces?

Otherwise we should look (in a new bug, though) into replacing |static| with
|namespace { ... }| to hide symbols in a translation unit.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.