The default bug view has changed. See this FAQ.

[10.5] Print/Page Setup Menu items grayed out after printing a page using File | Print

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Cocoa
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: marcia, Assigned: jtd)

Tracking

({regression, verified1.9.0.7, verified1.9.1})

Trunk
mozilla1.9.2a1
All
Mac OS X
regression, verified1.9.0.7, verified1.9.1
Points:
---
Bug Flags:
wanted-next +
blocking1.9.1 +
blocking1.9 -
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [See comments #26 and #37])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032804 Minefield/3.0pre.

STR:
1. Go to a web page and select File | Print | Save as PDF
2. Go back to the File menu and inspect the file choices.

Expected: All would be active
Actual: New Tab, Close, Save Page As, Send Link, Page Setup and Print are greyed out. I can get the menu items back easily by switching focus off the page.

This does not happen using the equivalent nightly on Tiger.
(Reporter)

Comment 1

9 years ago
I actually just noticed when testing on PPC mac that many other menus are greyed out besides the File Menu after following the STR in Comment 0. Switching focus out of the window and back restores all the menu items to active.
That's a regression which has been caused by a fix with the last days. Running a test with beta5 rc2 doesn't show this problem. I'll try to get the regression window.

Asking for blocking-firefox3 because of limited functionality.
Flags: blocking1.9?
Keywords: regression
Hardware: PC → All
Regression window: 20080327-04 and 20080328-04

Checkins in this timeframe:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-27+03%3A00%3A00&maxdate=2008-03-28+04%3A00%3A00&cvsroot=%2Fcvsroot

Possibly related: bug 425593
I'm not sure I've understood your description, Marcia.  But as far as
I can tell this isn't a bug.

The Save As dialog you get when you do File | Print | Save as PDF is a
native modal dialog (one provided by the OS, and not by Gecko).  While
it's running, it correctly disables a number of menu items, including
the ones you mention.  The idea is that this is a modal dialog, and
you need to finish whatever tasks it requires before you go off to do
anything else.  It works the same way in Safari.

When the dialog closes (whether you press Save or Cancel) the
greyed-out menu items always get re-enabled (at least in my tests).
(In reply to comment #4)
> When the dialog closes (whether you press Save or Cancel) the
> greyed-out menu items always get re-enabled (at least in my tests).

Exactly this doesn't happen under OS X 10.5. After you close the print dialog these menu items are not re-enabled. You have to switch the focus (like Marcia said) to get the menu items re-enabled. 

> After you close the print dialog these menu items are not re-enabled.

I can't reproduce this.  I tested with today's nightly on OS X 10.5.2 (Intel).
> I can't reproduce this.

Sorry, I take that back.  I _can_ reproduce it..
My patch for bug 422827 landed in Henrik's regression range from comment #3, and I was a _little_ afraid ... but I've now exonerated it.

Disabling my patch for bug 422827 doesn't fix this bug -- which shows that it's unrelated.
> Possibly related: bug 425593

Nope.  Backing out that bug's patch doesn't fix this problem.
Steven, are you able to verify the regression range? If not, I was wrong. Otherwise another patch listed by the above query should be responsible for.
> Steven, are you able to verify the regression range?

I haven't had a chance to (I haven't tried yet).  I hope to do so tonight or tomorrow.
Within the regression range, bug 425593 is probably covering up (hiding) the issue.

Starting with this hourly build: 20080326_1830, the print dialogue throws up a sheet (bug 425593). Clicking in the sheet to dismiss it shifts the focus around (somehow the steps in comment 1), and restores the functionality of the menus

I would be more suspicious of bug 372571 (that also causes 'vanishing menu issues' in Camino, see bug 426011). Bug 372571 landed slightly earlier than the regression range.
And with the 20080326_1732 hourly build, I can't reproduce the issue.
Philippe, where do you get these hourly builds?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
(In reply to comment #13)
> Philippe, where do you get these hourly builds?
> 

http://hourly-archive.localgho.st/mac.html

Updated

9 years ago
Duplicate of this bug: 426990

Comment 16

9 years ago
(In reply to comment #12)
> Within the regression range, bug 425593 is probably covering up (hiding) the
> issue.

Not really, see bug 426990.  I experienced the exact same thing, and it's because an OS window is taking focus.  Oddly, the upload dialog doesn't do it, but whenever I upgrade my nightly build and have to reauthorize access to the 1passwd keychain for the changed Minefield.app, coming back to Minefield breaks New Tab (etc).  I've duped my bug here because I'm certain it's the same, it's accessing an OS X feature and doesn't really recognize that focus has returned.  That's why (I think) switching apps back and forth fixes the issue.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040404 Minefield/3.0pre

Comment 17

9 years ago
This bug, whatever might have triggered it, is the result of our Carbon printing dialog doing whatever it wants to our Cocoa menus. We can't override that in the obvious way like we can for Cocoa app-modal dialogs. We need to get rid of our Carbon printing dialog and replace it with a Cocoa one. That said, there may be a way to work around this, which we can do for Gecko 1.9.0.x.

This bug is much less severe now that the patch for bug 398514 has landed. Now the keyboard commands will work just fine even in the menu items are disabled.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
Priority: P1 → P2

Comment 18

9 years ago
@Josh Aas

That makes sense for printing, but does that mean that bug 426990 should be unmarked as a dupe since it's a separate bug with the same effects?

Comment 19

9 years ago
I'd leave them both open for now, until we dig into more gritty details.
(Reporter)

Comment 20

9 years ago
Just as a point of info it happens when not only with Print to PDF, but print too so I am updating the summary of this bug.
Keywords: relnote
Summary: [10.5] Menu items grayed out when Print | Save as PDF is selected → [10.5] Menu items grayed out after printing a page using File | Print
Bug 433778 is probably related ... though it isn't a dup.
(In reply to comment #21)
> Bug 433778 is probably related ... though it isn't a dup.

Perhaps Masayuki can help here...

Updated

9 years ago
Duplicate of this bug: 435864

Comment 24

9 years ago
OS X 10.5.3 here, with FF3RC2, and when I click Print, then Cancel, I have to either create a new Firefox window or switch to another application, or open a Firefox sub-window (preferences, open a file, etc). Switching tabs doesn't do anything.

Comment 25

9 years ago
(In reply to comment #24)
> OS X 10.5.3 here, with FF3RC2, and when I click Print, then Cancel, I have to
> either create a new Firefox window or switch to another application, or open a
> Firefox sub-window (preferences, open a file, etc). Switching tabs doesn't do
> anything.
> 

Switching tabs doesn't because all existing tabs have the bug.  Try it again, but this time after hitting cancel, do Command-T to open a new tab.  I think that, for some reason, re-enabled the menu items (at least for me).
(Reporter)

Comment 26

9 years ago
Another way to get the menu item re-enabled is to switch focus outside the Firefox window. When you focus back on the app you will again have the "print" menu available. Just a workaround until we get this bug fixed...

Comment 27

9 years ago
Just for the record, this bug is still here in FF3RC3 on OSX 10.5.3 (Intel).

It "simply" looks like menus are not reset after the Print dialog is closed. If you open the Print dialog and look at the menus while the focus is on it, you will see that many menu items are grayed out, which is probably correct, as you cannot do any browsing-related operation on the Print dialog. But the problem is that these grayed out menus stay the same even after closing the Print dialog somehow (hitting Cancel, printing, etc.).
Duplicate of this bug: 438790
(Reporter)

Updated

9 years ago
Duplicate of this bug: 431952
Duplicate of this bug: 440507
Duplicate of this bug: 441513
Duplicate of this bug: 442556

Updated

9 years ago
Duplicate of this bug: 443598

Updated

9 years ago
Flags: blocking1.9.1?

Updated

9 years ago
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-

Comment 34

9 years ago
We also see this bug. Mac OS X 10.5.3 on PPC using 3.0.

Updated

9 years ago
Duplicate of this bug: 444643

Comment 36

9 years ago
I am seeing this same problem on 10.5.4. Going to Page Setup will then gray out the Page Setup and Print options in the File menu.

Comment 37

9 years ago
Please do not comment that you still see this bug.  It is known to exist and with the large amount of duplicates, it is confirmed that many people are seeing it.  It is confirmed.  Commenting that you still see it e-mails all 'subscribed' to this bug with 'bugspam'.  It will not cause the bug to become fixed any sooner.  Patches, however, are welcome.
Whiteboard: [See comments #26 and #37]

Updated

9 years ago
Duplicate of this bug: 448731
Duplicate of this bug: 449559
Duplicate of this bug: 450109

Updated

9 years ago
Duplicate of this bug: 451319

Updated

9 years ago
Flags: wanted1.9.1+ → wanted-next+
Duplicate of this bug: 453731
(Reporter)

Updated

9 years ago
Duplicate of this bug: 456610

Comment 44

9 years ago
As one of the people who reported this, I am more interested in understanding when we will see a version that has the fix for it. When is a new release due? RJ

Comment 45

9 years ago
You won't see a version that has a fix for it until at least one day after someone has submitted a patch for it.
Depends on: 456646

Comment 46

9 years ago
Is anyone working on a patch for it?

How do I get access to a patch when it is available?

Comment 47

9 years ago
(In reply to comment #46)
> Is anyone working on a patch for it?
> 
> How do I get access to a patch when it is available?

Yes, and just follow bug 456646 and wait until it has been marked fixed.  That means that the patch has been pushed into the repository and nightlies are being built from it.  Please refrain from needlessly commenting on the bug, as this creates extra bugmail for everyone on the CC list.  Thanks.

Comment 48

9 years ago
> Please refrain from needlessly commenting on the bug, as this creates extra bugmail for everyone on the CC list.

This wasn't "needless commenting", it was informative followup. Thanks RJS for your questions and Thomas for your answers. As someone on the CC list I'm glad there has been some progress.

Ryan

Comment 49

9 years ago
(In reply to comment #48)
> This wasn't "needless commenting", it was informative followup.

Ryan, I was referring to bug 456646, because he said:
>I split this work into a new bug to get a better tracking.

He obviously created the new bug to make it easier to resolve, as the only comments should be ones related to the patch and not related to a new dupe or questions about when the bug will be resolved.

Sorry for the confusion.
Duplicate of this bug: 460671

Comment 51

9 years ago
Have tracked through the 50 comments. (My comments were no.s 44 and 46.)

I am not familiar with the protocols re bug fixes, as I am just a user of Firefox, not a developer.

Is anyone working on the fix? Have looked at 456646 and there seems no progress there.

When this item is fixed where do I go to get an updated version?

Thanks

RJ
(In reply to comment #51)
> Have tracked through the 50 comments. (My comments were no.s 44 and 46.)
> 
> I am not familiar with the protocols re bug fixes, as I am just a user of
> Firefox, not a developer.
> 
> Is anyone working on the fix? Have looked at 456646 and there seems no progress
> there.

your answer is partly in comment 45 and 47 - they get fixed when someone has the time and the insight to code the solution. It could be hours, it could be weeks - asking when generally doesn't improve the odds of it happening sooner.


> When this item is fixed where do I go to get an updated version?

when the patch lands, the next day it will be in ftp://ftp.mozilla.org/pub/firefox/nightly/latest-trunk/
Flags: wanted1.9.1?
Duplicate of this bug: 463713
(Reporter)

Updated

9 years ago
Duplicate of this bug: 465745
(Reporter)

Updated

9 years ago
Duplicate of this bug: 458054

Updated

8 years ago
Duplicate of this bug: 467327

Comment 57

8 years ago
Lots of items marked as duplicates - the duplicates showing many people are seeing the problem. But no one has done anything to fix it. (I would, but have no technical skills.)

Is there a work around?
> Is there a work around?

Sure, don't use the menu to print: cmd+p

Comment 59

8 years ago
(In reply to comment #58)
> > Is there a work around?
> 
> Sure, don't use the menu to print: cmd+p

Or see the workaround in comment 26, mentioned in the whiteboard, and the request not to say "I still see this and nobody's posted a workaround" in comment 37, also mentioned in the whiteboard.
Duplicate of this bug: 469088
Duplicate of this bug: 474764
(Reporter)

Updated

8 years ago
Summary: [10.5] Menu items grayed out after printing a page using File | Print → [10.5] Print/Page Setup Menu items grayed out after printing a page using File | Print
(Assignee)

Comment 62

8 years ago
This bug is actually a lot more serious than would appear at first glance.  There are actually two parts to this bug: (1) while the Page Setup or Print dialogs are up, many menu items are enabled which shouldn't be and (2) once the dialog is dismissed, the state of menus remains in the the same state it was in while those dialogs were up.  The first part is cosmetic, nothing bad happens, but the second is why so many folks have logged dupes on this, because it hinders functionality.

So it's not just some items in the File menu, but *most* menu items are out of sync:

    File >
      Close               enable ==> disable
      Save Page As        enable ==> disable
      Send Link           enable ==> disable
      Page Setup          enable ==> disable
      Print               enable ==> disable
    
    Edit >
      Undo                disable ==> enable (WTF?!?)
      Redo                disable ==> enable
      Cut                 disable ==> enable
      Paste               disable ==> enable
      Delete              disable ==> enable
      Find                disable ==> enable
      Find Again          disable ==> enable
      
    View >
      *all*               enable ==> disable
      
    Bookmarks >
      Bookmark This Page  enable ==> disable
      Bookmark All Tabs   enable ==> disable
      
    Tools >
      Page Info           enable ==> disable
      
    Window >
      Minimize            enable ==> disable
      Zoom                enable ==> disable
  
We currently call the Carbon printing API's PMSessionPageSetupDialog and PMSessionPrintDialog, which is a bit odd in a Cocoa app.  So I'm sure the right approach is to move to Cocoa printing API's as Josh suggests.  However that's a big, out-of-scope task for either 3.1 or a 3.0 hotfix.  So I would suggest this, that we call a simple utility method that iterates over menu bar items and assures that they are in sync with the content, similar to what's going on here:

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsMenuItemX.mm#357

Call this method directly or from an event here:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/printingui/src/mac/nsPrintingPromptServiceX.mm#349

and here:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/printingui/src/mac/nsPrintingPromptServiceX.mm#487

I really, really think this needs to block for 1.9.1.

Screenshot of menus before and after bringing up the Page Setup dialog (before = top, after = bottom):

http://people.mozilla.org/~jdaggett/menusbeforeafter.png

Steps to reproduce:

1. Go to any page with text
2. Select a word
3. File > Page Setup
4. Click Cancel

Result: Menus change state from before to after as in the screenshot

Expected result: Menus should return to their original state
John, please also see bug 456646, which handles the switch from Carbon to Cocoa.
Re-nominating based on the workaround proposal in comment 62.
Flags: blocking1.9.1- → blocking1.9.1?
(Reporter)

Comment 65

8 years ago
I agree with Comment 62. In my daily work, this is probably the most annoying Mac bug I see consistently and the one most often mentioned by Hendrix mac users.
(Assignee)

Updated

8 years ago
Assignee: joshmoz → jdaggett
(Assignee)

Comment 66

8 years ago
I tracked down the change that caused this problem, it was the patch checked in to fix bug 372571, 2008-03-26 20:42 (josh).

To reproduce this, pull CVS trunk with MOZ_CO_DATE="26 Mar 2008 20:35 PDT".  Build and the problem does not occur.  Apply the patch for 372571 and it occurs.

This jives with the appearance of this bug in nightly builds, I came up with a slightly different regression window than the one suggested in comment 3 (and Philippe suggested it might be this bug in comment 12):

Bug does not occur: 2008-03-26-04 (4:47 AM)
Bug occurs:         2008-03-27-04 (4:49 AM)

Code changes during this time period:

http://tinyurl.com/macmenubugchanges
(Assignee)

Comment 67

8 years ago
Created attachment 358791 [details] [diff] [review]
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup

I don't know why this works but taking the two new routines from bug 372571, PrepareForNativeAppModalDialog and CleanUpAfterNativeAppModalDialog in nsCocoaUtils, and bracketing the calls to Carbon print dialog routines seems to eliminate the problem.  I stuck access methods for these in nsPrintSettingsX because I'm not quite sure how to call widget code from within the printgui component.  Lameness supreme...

Josh and/or Steven, can you take a look and let me know what you think the correct way of fixing this is?
Attachment #358791 - Flags: review?(joshmoz)
Comment on attachment 358791 [details] [diff] [review]
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup

>+++ b/widget/public/nsIPrintSettingsX.idl
You should update the uuid
..and remove printfs ;)
Blocks: 372571

Comment 69

8 years ago
Comment on attachment 358791 [details] [diff] [review]
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup

I tried something like this a while ago and it had no effect - the menu preparation did nothing for Carbon dialogs. I may have missed a spot or made some other mistake though.

Please remove the debugging printfs and rev the uuid like smaug said.

+  /* terribly rude hack */ 

Please remove this comment. This is how things are supposed to work given the way XUL and Mac OS X interact.

+  [noscript] void PrepareForNativeAppModalDialog();
+  [noscript] void CleanUpAfterNativeAppModalDialog();

The first character of method names should not be capitalized in idl files. See methods above the ones you added.

Once that is taken care of, if the patch works in your testing I'm fine with the code. This needs to be tested on Mac OS X 10.4 and 10.5 before landing - if you don't have one of those versions to test on let us know, smichaud can probably help you out.

Comment 70

8 years ago
blocking1.9.1+ so long as the approach in John Daggett's patch works. With a workable strategy we should not ship with this bug. However, if John's patch doesn't work out and we get back to rewriting our print dialogs in Cocoa we're going to have to minus this bug since that changes is unacceptable for 1.9.1 at this point.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+

Updated

8 years ago
Attachment #358791 - Flags: review?(joshmoz) → review-
(Assignee)

Comment 71

8 years ago
This is not strictly dependent on Cocoa printing dialogs, bug 456646, removing the dependency.
No longer depends on: 456646
(Assignee)

Comment 72

8 years ago
Created attachment 358993 [details] [diff] [review]
patch, v.0.2, cleanup and remove debugging code

Cleaned up version of the original patch.  It would be nice to have a better place to put this but leaving it as is for now.

Tested on 10.5, the graying out behavior is gone.  Although the code uses code simiilar to the code used to display an Edit menu when opening a file, this menu doesn't appear.  So Edit menu behavior remains broken, same as before.
Attachment #358791 - Attachment is obsolete: true
Attachment #358993 - Flags: review?(joshmoz)
(Assignee)

Comment 73

8 years ago
Tested on 10.4.  Before the patch when selecting Page Setup/Print, the entire menu bar would appear but edit controls (cut/copy/paste) would not work.  With this patch a single Edit menu appears and edit controls are usable to some degree but not all.  So the behavior is better, but there are still problems with edit controls that need to get fixed.  But that's a much smaller problem than menu items being out of whack on 10.5.

Try server build available here for those who want to test:

http://tinyurl.com/fixmenudisable
(Assignee)

Updated

8 years ago
Blocks: 475483
John, I've tested the tryserver build and it looks great on 10.5. Everything is accessible after the the print dialog was open. All menus look fine.

Comment 75

8 years ago
I have tested the tryserver build with 10.5. (10.5.6) as well and had some strange hangs with Print and Save Page, which were not reproducible. But this one is reproducible now. Before was running FF 3.0.5. Start build, open a webpage and try Save Page as ... 
Build hangs after some sec. and uses 99% Cpu.
John, after this patch lands on m-c and 1.9.1, can you please work up a 1.9.0 patch?
(Assignee)

Comment 77

8 years ago
(In reply to comment #75)
> I have tested the tryserver build with 10.5. (10.5.6) as well and had some
> strange hangs with Print and Save Page, which were not reproducible. But this
> one is reproducible now. Before was running FF 3.0.5. Start build, open a
> webpage and try Save Page as ... 
> Build hangs after some sec. and uses 99% Cpu.

This sounds like a separate bug.  Do you open the Save Page As dialog without choosing either Page Setup or Print first?  If so, this patch isn't the problem, this affects 3.1 beta builds also most likely.

Are these the steps you're using?

1. Start tryserver build
2. Open webpage
3. File > Save Page As

Result: build hangs and CPU goes to 99%

Is this what you're seeing?  (I'm on 10.4 right now, will test on 10.5 a little later).
(Assignee)

Comment 78

8 years ago
(In reply to comment #76)
> John, after this patch lands on m-c and 1.9.1, can you please work up a 1.9.0
> patch?

will do.
(Assignee)

Comment 79

8 years ago
Testing on 10.5, can't reproduce the behavior noted in comment 72.
No longer blocks: 475483
(Assignee)

Comment 80

8 years ago
Created attachment 359175 [details] [diff] [review]
patch, v.0.3, simpler version, just paint main window menu bar

I think this is a much simpler version of the fix, namely to simply repaint the menu bar of the main window after calling either the page setup or print dialogs:

  NSWindow* mainWindow = [NSApp mainWindow];
  if (mainWindow)
    [WindowDelegate paintMenubarForWindow:mainWindow];

This fixes the menu state problem on 10.5 and leaves 10.4 behavior completely unaffected.  It's effectively the equivalent of the workarounds mentioned, to switch to another window and then back again to restore the menu state.
Attachment #358993 - Attachment is obsolete: true
Attachment #359175 - Flags: review?(joshmoz)
Attachment #358993 - Flags: review?(joshmoz)

Comment 81

8 years ago
(In reply to comment #77)
It's true, John, it is a seperate bug. I checked this bug and it appears only if there are more than two files to store in sub folder. It doesn't matter if these are include files (js, css...) or images.
There is no bug reported at this time, I guess. Could be this a bug only in this build? And/or under which version it should be reported?
(Assignee)

Comment 82

8 years ago
Yeah, my guess is you've found a bug in 3.1/3.2 code that's not in 3.0.  The try server build is equivalent to using the latest nightly build, so try your test with a latest trunk nightly:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

If you run into a problem with one of these builds, log a bug with version "Trunk" and include the build number (<app> menu > About Minefield).  Be sure to include steps to reproduce, sounds like you're seeing a problem when saving a specific type of page, right?
(Assignee)

Comment 83

8 years ago
Try server build with latest patch for those who want to test:

http://tinyurl.com/fixmenudisablev03
(In reply to comment #82)
> Yeah, my guess is you've found a bug in 3.1/3.2 code that's not in 3.0.  The
> try server build is equivalent to using the latest nightly build, so try your
> test with a latest trunk nightly:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
> 
> If you run into a problem with one of these builds, log a bug with version
> "Trunk" and include the build number (<app> menu > About Minefield).  Be sure
> to include steps to reproduce, sounds like you're seeing a problem when saving
> a specific type of page, right?

it is a known issue - and already fixed: bug 475078.

Updated

8 years ago
Attachment #359175 - Flags: review?(joshmoz) → review-

Comment 85

8 years ago
Comment on attachment 359175 [details] [diff] [review]
patch, v.0.3, simpler version, just paint main window menu bar

>+   * After bringing up Carbon print dialogs, clean up menus.
>+   */
>+  [noscript] void cleanUpAfterDialog();
>+  

Lets call this "cleanUpAfterCarbonDialog" to make it clear when this is to be used. Please note a bug number in the comment for this new method. Get rid of the extra newline added after the method.

>+NS_IMETHODIMP nsPrintSettingsX::CleanUpAfterDialog()
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>+
>+  NSWindow* mainWindow = [NSApp mainWindow];
>+  if (mainWindow)
>+    [WindowDelegate paintMenubarForWindow:mainWindow];
>+  
>+  NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(noErr);
>+}

The exception code here is returning "noErr", a Carbon return value for no error, but the method signature says it returns an nsresult value. Change "NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(noErr)" to "NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT".

Also, we probably want to paint the menu bar for the hidden window if there is no main window after the dialog closes. See the logic for that in "nsCocoaUtils::CleanUpAfterNativeAppModalDialog()". The hidden window is the window that we use when there appears to be no windows open in a Gecko application.
(Assignee)

Comment 86

8 years ago
Created attachment 359423 [details] [diff] [review]
patch, v.0.4, changes based on review comments
Attachment #359175 - Attachment is obsolete: true
Attachment #359423 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #359423 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 87

8 years ago
pushed
http://hg.mozilla.org/mozilla-central/rev/d2542ec865bb
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0b8bb6f5ae23
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
(Assignee)

Comment 88

8 years ago
Created attachment 359487 [details] [diff] [review]
patch, v.0.4a, patch for 3.0 cvs trunk

1.9.0 patch, as per comment 76.  die bug die
Attachment #359487 - Flags: approval1.9.0.7?
(Assignee)

Comment 89

8 years ago
Created attachment 359495 [details] [diff] [review]
patch, v.0.4b, fixup

adjusted to changed location for hidden menu routine
Attachment #359487 - Attachment is obsolete: true
Attachment #359495 - Flags: approval1.9.0.7?
Attachment #359487 - Flags: approval1.9.0.7?
John-san, you must not change any interfaces on stable branch. You need to create a new interface for the new APIs.
See following patch:
https://bugzilla.mozilla.org/attachment.cgi?id=217349&action=diff
(In reply to comment #90)
> John-san, you must not change any interfaces on stable branch. You need to
> create a new interface for the new APIs.
> See following patch:
> https://bugzilla.mozilla.org/attachment.cgi?id=217349&action=diff

It should be fine to change the stable interface provided the UUID is revved and/or that the new function is added to the end.  I think both things are being done here, so it should be fine.
Thanks John! I think we can remove relnote now.

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre ID:20090129020336

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090129 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090129021345
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1, relnote → verified1.9.1
Target Milestone: --- → mozilla1.9.2a1
(In reply to comment #91)
> (In reply to comment #90)
> > John-san, you must not change any interfaces on stable branch. You need to
> > create a new interface for the new APIs.
> > See following patch:
> > https://bugzilla.mozilla.org/attachment.cgi?id=217349&action=diff
> 
> It should be fine to change the stable interface provided the UUID is revved
> and/or that the new function is added to the end.  I think both things are
> being done here, so it should be fine.

Really? I was not allowed in the bug (see bug 187772 comment 31).
In that particular case, it's likely because nsIRollupListener was added to the concrete implementation class' inheritance tree -- which may or may not have changed the concrete class layout.  I think it still should have been fine though, as people should have been QI'ing anyway to get the other implementations; mconnor was just being overzealous, I guess.

But, I don't know for sure -- would need a call from the 1.9.0 drivers, though.  To me, it would be fine to just add it at the end.
We have never allowed interface changes on stable branches under any circumstances.
(Assignee)

Comment 96

8 years ago
Created attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

Same patch but moved into a separate interface for 1.9.0.x branch code.
Attachment #359495 - Attachment is obsolete: true
Attachment #360034 - Flags: review?(joshmoz)
Attachment #360034 - Flags: approval1.9.0.7?
Attachment #359495 - Flags: approval1.9.0.7?
Whiteboard: [See comments #26 and #37] → [needs r=josh][See comments #26 and #37]
nsIPrintSettingsX_MOZILLA_1_9_BRANCH should be inherited nsIPrintSettingsX. Then:

-NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
+NS_IMPL_ISUPPORTS_INHERITED2(nsPrintSettingsX, 
                              nsPrintSettings, 
-                             nsIPrintSettingsX)
+                             nsIPrintSettingsX,
+                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)

can be:

 NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
                              nsPrintSettings, 
-                             nsIPrintSettingsX)
+                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)

And when a client gets nsIPrintSettingsX_MOZILLA_1_9_BRANCH interface, it can also use nsIPrintSettingsX interface. So, you can reduce to query interface in nsPrintingPromptServiceX.mm.

Comment 98

8 years ago
Comment on attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

Masayuki is probably right, waiting for a response from John.

Comment 99

8 years ago
Comment on attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

I'm actually fine either way, though please try what Masayuki suggested. The rest of this looks fine to me.
Attachment #360034 - Flags: review?(joshmoz) → review+
(In reply to comment #97)
> nsIPrintSettingsX_MOZILLA_1_9_BRANCH should be inherited nsIPrintSettingsX.
Could or should, right.

> Then:
> 
> -NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
> +NS_IMPL_ISUPPORTS_INHERITED2(nsPrintSettingsX, 
>                               nsPrintSettings, 
> -                             nsIPrintSettingsX)
> +                             nsIPrintSettingsX,
> +                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)
> 
> can be:
> 
>  NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
>                               nsPrintSettings, 
> -                             nsIPrintSettingsX)
> +                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)
This is not true. This would mean the the object doesn't support nsIPrintSettingsX, but it supports only nsIPrintSettingsX_MOZILLA_1_9_BRANCH.
QueryInterfacing to nsIPrintSettingsX would fail.
Comment on attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

Approved for 1.9.0.7, a=dveditz for release-drivers.

>+interface nsIPrintSettingsX_MOZILLA_1_9_BRANCH : nsISupports

subclassing nsIPrintSettingsX instead of nsISupports would have accomplished what comment 97 was getting at. Olli's right about the macros, though.
Attachment #360034 - Flags: approval1.9.0.7? → approval1.9.0.7+
(Assignee)

Comment 102

8 years ago
I'm going to leave this as-is.  I can definitely see the reason for subclassing vs. using a mix-in class but this is a very simple piece of code on a branch, I don't see any compelling reason to add more complexity in the form of subclasses to the problem.
Keywords: fixed1.9.0.7
(Assignee)

Comment 103

8 years ago
Checked in to CVS trunk
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009020404 GranParadiso/3.0.7pre ID:2009020404
Keywords: fixed1.9.0.7 → verified1.9.0.7
(In reply to comment #101)
> subclassing nsIPrintSettingsX instead of nsISupports would have accomplished
> what comment 97 was getting at. Olli's right about the macros, though.

Oh, right. I didn't do so in bug 187772, sorry.

Updated

8 years ago
Duplicate of this bug: 478620
Whiteboard: [needs r=josh][See comments #26 and #37] → [See comments #26 and #37]
Marcia, as you have said this is not fixed for 10.6. Is this still true?

Comment 108

8 years ago
Firefox is currently the only application on MacOSX Intel 10.5.6 that have this bug.
Not be able to print in 2009 is very poor and we will switch back to Safari and forget Firefox.
Free application must not be buggy
(In reply to comment #108)
> Firefox is currently the only application on MacOSX Intel 10.5.6 that have this
> bug.

This will be fixed in the next beta version of Firefox 3.1 and for Firefox 3.0.7. Please stay tuned.
(Reporter)

Comment 110

8 years ago
I will check 10.6 the next time I am in the lab and report back.
You need to log in before you can comment on or make changes to this bug.