Replace Carbon printing dialog with Cocoa one

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: whimboo, Assigned: mstange)

Tracking

Trunk
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
blocking1.9.1 -
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 6 obsolete attachments)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080922020341 Minefield/3.1b1pre ID:20080922020341

Currently we fail while re-enabling the menu items inside some menus after the printing dialog is closed (see bug 425844). This is caused by using the old Carbon dialog. While we have already switched to mostly using Cocoa, the printing dialog has to be replaced by the Cocoa one - that is what Josh mentioned in bug 425844 comment 17.

I split this work into a new bug to get a better tracking.
Flags: blocking1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
No longer blocks: 425844
Flags: wanted1.9.2+
Flags: wanted1.9.1+ → wanted1.9.1-
Priority: -- → P1
Assignee: joshmoz → mstange
Blocks: 495567
Blocks: 468509
Posted patch v1 (obsolete) — Splinter Review
This will neither build nor work on 10.4. Josh, should I #ifdef all of the changes?

I'm using the following APIs that don't exist on 10.4:
 1. -[NSPrintPanel showModalWithPrintInfo:]
 2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]
 3. -[NSString stringByReplacingOccurrencesOfString:withString:]
 4. -[NSPrintPanel addAccessoryController:]
 5. PMPageFormatCreate(With)DataRepresentation
and probably some more.

The first two are essential to the implementation and I haven't found 10.4 replacements for them. 3 is just for convenience and 4 and 5 get rid of deprecated warnings.
Attachment #398583 - Flags: review?(joshmoz)
Status: NEW → ASSIGNED
(In reply to comment #1)
> Created an attachment (id=398583) [details]
> v1
> 
> This will neither build nor work on 10.4. Josh, should I #ifdef all of the
> changes?
> 
> I'm using the following APIs that don't exist on 10.4:
>  1. -[NSPrintPanel showModalWithPrintInfo:]

On 10.4, will it work to just create a dummy NSPrintOperation, load it with the NSPrintInfo via its setPrintInfo, and then call runModal?

>  2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]

On 10.4, will it work to use initWithDictionary NSPrintInfo initializer after filling in a custom dictionary from the Gecko print settings?
(In reply to comment #2)
> On 10.4, will it work to just create a dummy NSPrintOperation, load it with the
> NSPrintInfo via its setPrintInfo, and then call runModal?

Could be! I haven't tested yet (and can't at the moment).
 
> >  2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]
> 
> On 10.4, will it work to use initWithDictionary NSPrintInfo initializer after
> filling in a custom dictionary from the Gecko print settings?

I don't think that's enough. We also need these Carbon objects in nsDeviceContextSpecX.mm for things like ::PMSessionBeginPageNoDialog, for which there is no Cocoa equivalent.
(In reply to comment #3)
> (In reply to comment #2)
> > On 10.4, will it work to just create a dummy NSPrintOperation, load it with the
> > NSPrintInfo via its setPrintInfo, and then call runModal?
> 
> Could be! I haven't tested yet (and can't at the moment).

The docs talk about the "current print operation" when using [NSPrintPanel runModal]. Probably referring to [NSPrintOperation setCurrentOperation].

> > >  2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]
> > 
> > On 10.4, will it work to use initWithDictionary NSPrintInfo initializer after
> > filling in a custom dictionary from the Gecko print settings?
> 
> I don't think that's enough. We also need these Carbon objects in
> nsDeviceContextSpecX.mm for things like ::PMSessionBeginPageNoDialog, for which
> there is no Cocoa equivalent.

Any benefit in rewriting that code in Cocoa?  Not sure if this would work:  Initiate the print operation using NSPrintOperation.  Suppress the print dialog using the setShowsPrintPanel: method.  And you'll need a Quartz surface which you can get by calling the context method to get a NSGraphicsContext (which I believe can be toll-free bridged to CGContextRef).  Not sure how extensive such changes would be though.
Probably would need to suppress the status UI with setShowsProgressPanel: as well.
I don't think it's worth it if 10.4 compatibility is the only benefit of such a rewrite.
I've tried this patch and with this patch Thunderbird 3.1a1pre (haven't tested for Firefox) builds fine with the 10.6 SDK. But at the last step I get the error:

rm -f ../../mozilla/dist/Thunderbird.app/Contents/MacOS/thunderbird-bin
rsync -aL thunderbird-bin ../../mozilla/dist/Thunderbird.app/Contents/MacOS
mkdir -p ../../mozilla/dist/Thunderbird.app/Contents/Plug-Ins
rsync -a --copy-unsafe-links ../../mozilla/dist/package/PrintPDE.plugin ../../mozilla/dist/Thunderbird.app/Contents/Plug-Ins
rsync: link_stat "/temp/src/obj-i386-apple-darwin10.0.0/mail/app/../../mozilla/dist/package/PrintPDE.plugin" failed: No such file or directory (2)
rsync error: some files could not be transferred (code 23) at /SourceCache/rsync/rsync-37.3/rsync/main.c(992) [sender=2.6.9]
make[5]: *** [libs] Error 23
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_app] Error 2
make[2]: *** [tier_app] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2
(In reply to comment #6)
> I don't think it's worth it if 10.4 compatibility is the only benefit of such a
> rewrite.

Probably true. Plus Cocoa inverts how the print operation works by driving the process itself and telling the NSView to draw each page, as opposed to Carbon, where the application tells Carbon about each page.

Would there be any benefit to having an all Cocoa solution going forward apart from 10.4 compatibility?
(In reply to comment #7)
> But at the last step I get the error:

Try removing the printpde.plugin line from mail/app/Makefile.in. It looks like I need to remove that line from browser/app/Makefile.in, too.

(In reply to comment #8)
> Plus Cocoa inverts how the print operation works by driving the
> process itself and telling the NSView to draw each page, as opposed to Carbon,
> where the application tells Carbon about each page.

That's what scared me away from such a rewrite ;-)

> Would there be any benefit to having an all Cocoa solution going forward apart
> from 10.4 compatibility?

I don't know. It looks like the parts of (Carbon) Core Printing that we use with this patch are fully supported by Apple and 64 bit clean. Josh?
Posted image Broken print dialog
After applying this patch, the print dialog looks like this. I've tried it no three times with fresh source code and checked all modified files.
Markus - your patch needs to account for this line:

http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#348

That packages the PrintPDE plugin in the browser and shouldn't be executed when building with cocoa printing.
Also, we unfortunately do need a patch that doesn't drop 10.4 support right now. Can you fix this to leave 10.4 support by making Cocoa printing a build-time option, much like the Core Text backend is a build-time option?
(In reply to comment #11)
> Markus - your patch needs to account for this line:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#348
> 
> That packages the PrintPDE plugin in the browser and shouldn't be executed when
> building with cocoa printing.

Same thing for Thunderbird 3.1a1pre (Thunderbird 3.next) in /mail/app/Makefile.in
Otherwise you will get a build error.
Attachment #398583 - Flags: review?(joshmoz)
I haven't been able to write a proper configure test. Is there a way to make AC_TRY_RUN run Objective C?
Attachment #398583 - Attachment is obsolete: true
Attachment #403177 - Flags: review?(joshmoz)
Attachment #403177 - Attachment is obsolete: true
Attachment #403183 - Flags: review?(joshmoz)
Attachment #403177 - Flags: review?(joshmoz)
Comment on attachment 403183 [details] [diff] [review]
v3, re-using the core text check

+#include <Carbon/Carbon.h>

Use "ApplicationServices/ApplicationServices.h" - I doubt we need to use the actual Carbon headers anywhere.

+- (NSTextField*)label:(const char*)aLabel
+            withFrame:(NSRect)aRect
+            alignment:(NSTextAlignment)aAlignment

+- (NSPopUpButton*)headerFooterItemListWithFrame:(NSRect)aRect
+                                   selectedItem:(const PRUnichar*)aCurrentString

These method names suggest that the returned objects are autoreleased, per obj-c convention. In fact, as implemented, callers are required to release. Please return autoreleased objects and check all new obj-c methods for this problem.

+- (NSButton*)radioWithLabel:(const char*)aLabel andFrame:(NSRect)aRect;

This method doesn't appear to be used.

In general this looks great, thanks!
Attachment #403183 - Flags: review?(joshmoz) → review-
Also, when I built with this patch and tried to pull up a print dialog I saw what nomis saw in comment #10, blank labels and options.

I see this console output when that happens:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
###!!! ASSERTION: nsPrintSettingsX::ReadPageFormatFromPrefs() failed: 'NS_SUCCEEDED(rv)', file /Users/josh/src/mozilla/ff_193_debug/widget/src/cocoa/nsPrintOptionsX.mm, line 92
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1055
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040154: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 910
getPrintSettings: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIPrintSettingsService.defaultPrinterName]"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: chrome://global/content/printUtils.js :: anonymous :: line 125"  data: no]
2009-09-28 01:51:57.657 firefox-bin[65739:813] 	ERROR
(In reply to comment #16)
Adressed.

(In reply to comment #17)
> Also, when I built with this patch and tried to pull up a print dialog I saw
> what nomis saw in comment #10, blank labels and options.

I can't reproduce this. I've added a plus to to the line in jar.mn, maybe that helps? What do you see when you put chrome://global/locale/printdialog.properties into your URL bar?

> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file
> /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp,
> line 1107

This seems to be unrelated. It's hidden deep in cross-platform code I didn't touch.

> ###!!! ASSERTION: nsPrintSettingsX::ReadPageFormatFromPrefs() failed:
> 'NS_SUCCEEDED(rv)', file
> /Users/josh/src/mozilla/ff_193_debug/widget/src/cocoa/nsPrintOptionsX.mm, line
> 92

The problem here is that I've changed the format the page format is stored in. This assertion will go away the next time you click OK in the Page Setup dialog.
I've changed the code in nsPrintSettingsX::ReadPageFormatFromPrefs to not return an error result code when the settings import fails.

The rests seems unrelated.
Posted patch v4 (obsolete) — Splinter Review
Attachment #403183 - Attachment is obsolete: true
Attachment #403344 - Flags: review?(joshmoz)
Posted patch v4.1 (obsolete) — Splinter Review
This has the + in jar.mn.
Attachment #403344 - Attachment is obsolete: true
Attachment #403345 - Flags: review?(joshmoz)
Attachment #403344 - Flags: review?(joshmoz)
Surely what you want is another name, rather than a plus? You use a plus when you want to overwrite an existing file that someone else put in the jar, even if their version is newer, because your file is a total replacement for theirs (same keys, different values, for a .properties).

In this case, you're either replacing dom/locales/en-US/chrome/printdialog.properties with your printdialog.properties on all platforms, which seems like it's going to come as a bit of a surprise to embedding/components/printingui/src/win/nsPrintDialogUtil.cpp since you don't have the same keys, or if dom/locales builds after toolkit/locales (which I don't think it does, but I'm often surprised), then you would have to touch yours every time that one changes, to keep it from overwriting you.
dom/locales! Thanks!
Posted patch v5 (obsolete) — Splinter Review
This combines the two printdialog.properties files.
Attachment #403345 - Attachment is obsolete: true
Attachment #403414 - Flags: review?(joshmoz)
Attachment #403345 - Flags: review?(joshmoz)
Attachment #403414 - Flags: review?(joshmoz) → review+
Comment on attachment 403414 [details] [diff] [review]
v5

+- (NSTextField*)label:(const char*)aLabel
+            withFrame:(NSRect)aRect
+            alignment:(NSTextAlignment)aAlignment
+{
+  NSTextField* label = [[NSTextField alloc] initWithFrame:aRect];
+  [label setStringValue:[self localizedString:aLabel]];
+  [label setEditable:NO];
+  [label setSelectable:NO];
+  [label setBezeled:NO];
+  [label setBordered:NO];
+  [label setDrawsBackground:NO];
+  [label setFont:[NSFont systemFontOfSize:[NSFont systemFontSize]]];
+  [label setAlignment:aAlignment];
+  return [label autorelease];
+}

Typically autorelease is done when the object is created. Doing so avoids problems if someone adds an early return, much like stack-based memory mgmt. Please fix that in the multiple places the problem occurs.

Other than that, this looks good. I don't see the blank labels problem any more.
(In reply to comment #24)
> I don't see the blank labels problem any
> more.
I can confirm this. v5 also works for me (no blank labels anymore).
Posted patch v6Splinter Review
Changed the location of the autorelease calls. I also changed some sizes in order to allow for longer labels for verbose localizations.
Attachment #403414 - Attachment is obsolete: true
Comment on attachment 403606 [details] [diff] [review]
v6

sr=jst for the dom related changes.
Attachment #403606 - Flags: superreview+
Attachment #403606 - Flags: review?(benjamin)
Comment on attachment 403606 [details] [diff] [review]
v6

r=me on the build bits and such... I didn't do a code review
Attachment #403606 - Flags: review?(benjamin) → review+
I think that is all the necessary reviews, lets get this in. Thanks Markus!
http://hg.mozilla.org/mozilla-central/rev/8c4658f8f0dc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 520494
No longer blocks: 433778
No longer blocks: 495567
You need to log in before you can comment on or make changes to this bug.