367829 - not possible to build w/o MOZ_ENABLE_POSTSCRIPT

Status: RESOLVED WONTFIX

(Core :: Widget: Gtk, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

(don't know if this is widget or Printing)
I used to build w/o MOZ_ENABLE_POSTSCRIPT on trunk which is not possible anymore since bug 323928 landed.
This attachment from bug 323928 doesn't fix the build
https://bugzilla.mozilla.org/attachment.cgi?id=247531 :

nsDeviceContextSpecG.o: In function `nsDeviceContextSpecGTK::GetSurfaceForPrinter(gfxASurface**)':nsDeviceContextSpecG.cpp:(.text+0x607): undefined reference to `nsPrintJobFactoryGTK::CreatePrintJob(nsDeviceContextSpecGTK*, nsIPrintJobGTK*&)'

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Build fix.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Undefined reference build fix

also ifdef-ed
nsPrintJobFactoryGTK::CreatePrintJob

Comment 4

[Oleg Romashin (:romaxa)]

It should help for
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest
Xulrunner Min

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This patch doesn't apply cleanly against current trunk, can you update it?

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Update to trunk for checkin

whole function ifdefed, return NS_ENSURE_SUCCESS to see problem in debug builds..

Comment 8

[Phil Ringnalda (:philor)]

That's not "updated to trunk" - it's a different patch. If you want to do that instead, please get it re-reviewed and re-approved, or if you think it's the same thing, please persuade your reviewer to check it in, because to the random peons pulled in by checkin-needed, that's too much change.

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Update to trunk2 for checkin

Comment 10

[Oleg Romashin (:romaxa)]

I hope this is more looks like updated to trunk?

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 276404 [details] [diff] [review]
Update to trunk for checkin

> NS_IMETHODIMP nsDeviceContextSpecGTK::GetSurfaceForPrinter(gfxASurface **aSurface)
> {
>+#ifndef MOZ_ENABLE_POSTSCRIPT
>+  NS_ENSURE_SUCCESS(NS_ERROR_FAILURE, NS_ERROR_FAILURE);
>+#else

It's not my module, but to me success(failure) is needlessly opaque. Please spell it out

    NS_NOTREACHED("Shouldn't call this if no Postscript support");
    return NS_ERROR_FAILURE;

Or NS_WARNING() if you just want a debug message without the assert. NS_NOTYETIMPLEMENTED() if you think you'll flesh this out some day.

Comment 12

[Nickolay_Ponomarev]

Checked in attachment 278004 [details] [diff] [review]. Please close the bug if needed.

mozilla/widget/src/gtk2/Makefile.in               1.67
mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp  1.91

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 278004 [details] [diff] [review]
Update to trunk2 for checkin

+  nsresult rv = NS_ERROR_FAILURE;
+#ifndef MOZ_ENABLE_POSTSCRIPT
+  nsPrintJobFactoryGTK::CreatePrintJob(this, mPrintJob);
+#endif
   if (NS_FAILED(rv))
     return rv;

Isn't this missing an "rv =" inside the ifndef?

Comment 14

[Nickolay_Ponomarev]

Fixed that: mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp  1.92

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 278004 [details] [diff] [review]
Update to trunk2 for checkin

>Index: src/gtk2/nsDeviceContextSpecG.cpp
>===================================================================
> 
>-  nsresult rv = nsPrintJobFactoryGTK::CreatePrintJob(this, mPrintJob);
>+  nsresult rv = NS_ERROR_FAILURE;
>+#ifndef MOZ_ENABLE_POSTSCRIPT
>+  nsPrintJobFactoryGTK::CreatePrintJob(this, mPrintJob);
>+#endif
>   if (NS_FAILED(rv))
>     return rv;

Hmm? Why did that turn into an #ifndef? I would expect that we want to create a print job if 
POSTSCRIPT is actually enabled?

Comment 16

[Nickolay_Ponomarev]

Sigh, fixed in rev 1.93.

Comment 17

[Stuart Parmenter]

sorry, i didn't look closely enough at this patch.

We should be getting rid of MOZ_ENABLE_POSTSCRIPT #ifdefs not adding new ones.  we should remove the --enable/disable-postscript option from configure entirely.

Comment 18

[Stuart Parmenter]

i've filed 394413 on removing MOZ_ENABLE_POSTSCRIPT entirely.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This patch caused bug 394407: MOZ_ENABLE_POSTSCRIPT seems to not be defined when building widget/src/gtk2/nsDeviceContextSpecG.cpp.  Or something.  The .i file has:

  nsresult rv = ((nsresult) 0x80004005L);

  if (((__builtin_expect(!!((rv) & 0x80000000), 0))))
    return rv;

which is clearly bad.

Comment 20

[Nickolay_Ponomarev]

I'm going to back this patch out this weekend, per stuart's comment and because this caused regressions. If anyone beats me to it, great.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Note that the patch in bug 394160 switches to the right define.

Pending that landing, I have backed this out to fix smoketest blocker bug 394407.

Comment 22

[Daniel Brooks [:db48x]]

Ok, this fix seems to have been lost in the shuffle. It caused a regression, and bug 394160 had a patch that fixed the regression. This bug was then backed out, and marked as fixed because the new patch would leave this bug in a fixed state. However, the new bug was then closed because it was just a regression from a patch that got backed out, and thus was no longer needed.

I'll attach a patch that corrects romaxa's typo for review.

Comment 23

[Daniel Brooks [:db48x]]

Attachment: 367829-5.diff

Comment 24

[Boris Zbarsky [:bzbarsky]]

I believe Stuart said that this bug as filed should be wontfixed.

Comment 25

[Oleg Romashin (:romaxa)]

>Created an attachment (id=282927) [details]
>367829-5.dif

This is wrong,

MOZ_ENABLE_POSTSCRIPT cannot be used there, USE_POSTSCRIPT is ok.
It makes printing broken by default... Bug 394160.

Also there still one using not initialized mPrintJob without check in end document...
https://bugzilla.mozilla.org/show_bug.cgi?id=394160
https://bugzilla.mozilla.org/attachment.cgi?id=278825

Comment 26

[Oleg Romashin (:romaxa)]

Merge 
https://bugzilla.mozilla.org/attachment.cgi?id=282927
+
https://bugzilla.mozilla.org/attachment.cgi?id=278825
=
Probably good result.

Comment 27

[Daniel Brooks [:db48x]]

Yes, it would be nice to get bug 394413 done, but it's not even assigned to anybody. I don't know anything about how cairo or gfx work, so in the mean time this bustage fix seems like a good way to go. It won't add much to the work required to fix 394413.

Comment 28

[Daniel Brooks [:db48x]]

Attachment: 367829-6.diff

thanks romaxa, I had forgotten about the other change

Comment 29

[Oleg Romashin (:romaxa)]

>widget/src/gtk2/Makefile.in
.....
>+ifdef USE_POSTSCRIPT

I guess USE_POSTSCRIPT is not defined in Make... there are MOZ_....
USE_POSTSCRIPT defined here: as -D...
http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/Makefile.in#157


If we want to use USE_POSTSCRIPT in Makefile, than we have to define it somewhere...

Comment 30

[Stuart Parmenter]

as i said before, we should just remove all of these ifdefs.