191046 - [OSX] Embedded plugins don't print - NPP_Print not called

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Peter Lubczynski]

On OSX platforms, NPP_Print is not being called for embedded plugins.

As a workaround, NPP_Print is being for full-page plugins so they should print.

Comment 1

[Peter Lubczynski]

oops--->plugins

Comment 2

[Peter Lubczynski]

Attachment: patch (wip)

Here's a patch that fixes up GrafPorts pointers to GrafPtrs and reuses the
Classic code path for printing. Unfortunatly it needs some more work as we
crash with Flash and the Default Plugin prints blank (MachO/Seamonkey).

Comment 3

[Jonathan Brecher]

This is a dup of 186353, but this has a patch, so I'm going to close the other one out even 
tho it was older than this one.

Note that sfraser has a copy of the ChemDraw plugin, which might be another plugin to 
use for testing -- assuming our printing code is solid, which isn't exactly a sure thing 
considering that I had no way to test it on Mac. I'll test things myself once a patched build 
is available.

Comment 4

[Jonathan Brecher]

*** Bug 186353 has been marked as a duplicate of this bug. ***

Comment 5

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 6

[Paul Wyskoczka]

ADT: Nominating topembed

Comment 7

[Michael Buckland]

peter, have you looked at the patch and do you think it's a good solution to the
problem?

Comment 8

[Peter Lubczynski]

Comment on attachment 112925 [details] [diff] [review]
patch (wip)

This patch crashes when Flash is tries to print and I'm not sure why. I think
we may need to check NPAPI version and ensure we pass the right pointer to
Flash.

Comment 9

[Jonathan Brecher]

Any progress on this? Sure would be nice to get this fixed...

Comment 10

[William Bardwell]

I am seeing similar problem on linux (FireFox 1.0.4)

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 359075 has been marked as a duplicate of this bug. ***

Comment 12

[Michelle Sintov]

Safari prints Flash plug-in content on the Mac, so Firefox should be able to do the same. Please contact me if you'd like some pointers on how to make this work.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

See bug 388082 for the current state of affairs.

Comment 15

[Pat Flaherty]

Can we say that this will probably never be fixed at this point ?

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Never is a long time :-)

But I don't think this can be fixed by the Firefox 3 release.

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: patch v0

I have been working on this recently, but haven't had time to finish it because I've been busy with Firefox 3 endgame bug fixing.

The attached patch works, but it needs some clean up before requesting review.  There also seems to be a bug with the offscreen buffer sizing or during clipping when we copy it back to the print surface, because you can see the chrome in YouTube videos is slightly clipped.  This also happens with QT plugins on Apple's site.  I'm actually not sure if the bug is ours or in Flash at this stage, because I'm seeing the same behaviour with Safari.  Need to investigate more.

The nasty hack in ns4xPluginInstance.cpp is scary.  Somehow plugins (Flash and Quicktime, at least) and Gecko disagree on the memory layout of the NPPrint structure.  In Gecko, there's two bytes of padding inside the struct after the uint16 mode member.  The plugins seem to expect no padding here.  There was a change to our copy of npapi.h last year that removed some structure padding pragmas, but I'm pretty sure these were only used on Mac Classic in the Gecko build system (they were hidden behind XP_MAC, not XP_MACOSX), but I do wonder if the plugins were built with XP_MAC defined.  Need to investigate this more as well.

Comment 19

[Andy]

Attachment: Fix for Bug#191046 macosx embedded plugin printing

There are four issues in the gecko code base preventing macosx embedded printing from plugins:
1/ NPP_Print not being called.
2/ NPPrint data structure has incorrect byte-alignment with what plugins are expecting.
3/ Gecko code incorrectly handling the order of events causing plugins to be in a bad state for printing.
4/ Plugins are expecting a CGrafPtr to be handed to them from Firefox to draw in, but gecko has no code to handle passing a graph port to the plugin

The provided patch addresses all four of these issues allowing plugins like Adobe Flash and Apple QuickTime to correctly do embedded printing.

Comment 20

[Michelle Sintov]

(In reply to comment #19)

Wow!!! Thank you!

Comment 21

[Andy]

What's the status of this bug, has there been a code review of the patch submitted 2009-05-04?
Thanks,

Comment 22

[Josh Aas]

It is high on our list, some other work has been slow-going though. We'll get to it soon. Thanks.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Andy, I've started to review your patch.

The first thing I need to do is to get to the bottom of the alignment
problem.  Specifically, I need to figure out why the fullPrint and
embedPrint members of the NPPrint structure are (at least some of the
time) placed at an offset of 4 bytes from the top of the structure,
when (by all rights) they should be at an offset of 2 bytes.  (Matthew
Gregan also noticed this, and hacked around it in his patch's
ns4xPluginInstance.cpp.  See comment #17.)

I hesitate to add any more #pragma pack(n) statements to npapi.h
(besides the one already in use for compilation on OS/2).  This file
implements a public API used by all vendors that implement the NPAPI.
So fiddling with its alignment is very dangerous, and we shouldn't do
it unless we have very good reasons.

So far I've managed to find out that compiling the following with
plain 'gcc' (and no extra parameters) shows that a 'uint16_t' gets
aligned on a 16-bit (2-byte) boundary -- which should mean that
fullPrint and embedPrint get placed at an offset of 2 bytes.

#include <stdio.h>
#include <stdint.h>

int main(int argc, char *argv[])
{
  printf("align of uint16_t is %i\n", __alignof__(uint16_t));
  return 0;
}

Right now I suspect we're somehow altering gcc's default alignment
behavior.  I'll keep digging.

If anyone has deeper insights into this problem, please let us know.

So far I've been testing with the gcc 4.0.1 that comes with XCode 3.0
on OS X 10.5.7.  Later I'll test with more recent XCode versions.

Comment 24

[Zack Weinberg (:zwol)]

(In reply to comment #23)
>So far I've managed to find out that compiling the following with
>plain 'gcc' (and no extra parameters) shows that a 'uint16_t' gets
>aligned on a 16-bit (2-byte) boundary -- which should mean that
>fullPrint and embedPrint get placed at an offset of 2 bytes.

Yes, but both NPEmbedPrint and NPFullPrint, in the absence of packing directives, require 4-byte alignment (or 8-byte on 64-bit systems) since they contain pointers.  Thus, so does the union.

(The pair of NPBools at the beginning of NPEmbedPrint do NOT fill the 2-byte gap, and would not fill the gap even if there were no union; the alignment of a structure is the maximum alignment of all its members.  Thus there's a 2-byte gap between 'mode' and the union, and there's *another* 2-byte gap between printOne and platformPrint.  It would have been better to put 'mode' at the end of NPPrint and sort all pointers to the beginning of NPWindow, NPFullPrint, and NPEmbedPrint.  I suppose there is nothing that can be done about this now.)

Comment 25

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #24)

Thanks for the info ... but the puzzle remains.

Why do Flash and Quicktime, which (presumably) use the same npapi.h,
pack it differently?  I assume they don't use explicit packing
directives -- which after all would be very dangerous.

Comment 26

[Zack Weinberg (:zwol)]

I have an extremely low opinion of plugin vendors.  I would assume that they have done at least one of these boneheaded moves:

 - copied npapi.h from some ancient version of Netscape and never
   updated it since
 - compiled all their code with explicit packing overrides, ABI be damned

Comment 27

[Steven Michaud [:smichaud] (Retired)]

:-)

So I guess I should study the history of npapi.h.

I'll do that.

Comment 28

[Andy]

The problem with offsets into the NPPrint structure appears to be that in the WebKit version of npapi.h structures are wrapped with:
#if !defined(__LP64__)
#if defined(XP_MAC) || defined(XP_MACOSX)
#pragma options align=mac68k
#endif
#endif /* __LP64__ */

Plugins built using the WebKit header will use a alignment of
#pragma options align=mac68k. Therefore leading to the incompatibility with the gecko version, for gecko to be compatible it would need to replicate this or define a #pragma pack(2) around its structures. 

Therefore the reason why Flash and Quicktime are packing differently  is because the plugins are built based on the webkit headers.

Comment 29

[Michelle Sintov]

(In reply to comment #28)
> The problem with offsets into the NPPrint structure appears to be that in the
> WebKit version of npapi.h structures are wrapped with:
> #if !defined(__LP64__)
> #if defined(XP_MAC) || defined(XP_MACOSX)
> #pragma options align=mac68k
> #endif
> #endif /* __LP64__ */

The #ifdef used above it used incorrectly. __LP64__ is a 1 or a 0. So, that code doesn't do what it intends, I think.

Comment 30

[Andy]

(In reply to comment #29)
> (In reply to comment #28)
> > The problem with offsets into the NPPrint structure appears to be that in the
> > WebKit version of npapi.h structures are wrapped with:
> > #if !defined(__LP64__)
> > #if defined(XP_MAC) || defined(XP_MACOSX)
> > #pragma options align=mac68k
> > #endif
> > #endif /* __LP64__ */
> 
> The #ifdef used above it used incorrectly. __LP64__ is a 1 or a 0. So, that
> code doesn't do what it intends, I think.

I believe it is working as intended look at Apple's documentation at:
http://developer.apple.com/MacOsX/64bit.html

it states the following:
To conditionally compile 64-bit code or define 64-bit APIs, you can use the __LP64__ and __ppc64__ macros. For example, the following shows a function prototype defined two different ways depending on whether the code is being compiled as a 64-bit executable or not:

#ifdef __LP64__   
  int  getattrlist(const char*,void*,void*,size_t, unsigned int); 
#else /*__LP64__*/  
  int  getattrlist(const char*,void*,void*,size_t, unsigned long); 
#endif

Note that actually checking for __LP64__ in this instance isn't necessary on the Mac because the XCode compiler automatically ignores all align=mac68k statements when doing 64-bit builds.

Comment 31

[Michelle Sintov]

Ah, you are right. I guess we have some confusion on our end.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

> The problem with offsets into the NPPrint structure appears to be
> that in the WebKit version of npapi.h structures are wrapped with:
> #if !defined(__LP64__)
> #if defined(XP_MAC) || defined(XP_MACOSX)
> #pragma options align=mac68k
> #endif
> #endif /* __LP64__ */

Turns out the alignment problem is our fault, not WebKit's.

Before the patch for bug 281889 landed (2006-07-13 12:47), Mozilla's
own npapi.h wrapped its "structures and definitions" with the
following:

#ifdef XP_MAC
#pragma options align=mac68k
#endif

The simplest solution would be to restore it.  But it's been missing
for a while, and restoring it might cause trouble with plugins that
use up-to-date copies of our own npapi.h.

I'll work through npapi.h to see where this change would make a
difference.  It's already clear, though, that most structures'
alignment wouldn't change ... otherwise we'd have stumbled on this
hornet's nest long ago.

In the meantime, I'd appreciate other opinions and suggestions.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Attachment: Utility to check the effects of different align= directives

Here's the source for a little utility that checks what effect
different align= directives (or no directive at all) have on the
alignment of structures in npapi.h that are currently wrapped by an
align=mac68k directive in WebKit source distros, and used to be in
Mozilla's npapi.h.  Instructions are in the source.

With it I've found that using or not using the align=mac68k directive
only makes a difference to the NPPrint and NPFullPrint structures.
Since printing currently doesn't work on OS X for any plugin, we can't
break anything by changing the alignment of these structures.

I think this clinches the matter -- we should restore the align=mac68k
directive that was dropped when the patch for bug 281889 landed.

Thoughts?

(It's possible that some plugins will contain printing code that
*should* work once this bug (bug 191046) is fixed, but which will fail
because of alignment problems.  In other words, this mess probably
doesn't have any seamless solution.  But I think restoring the
align=mac68k directive is as close to one as we can get.)

Comment 34

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #33)

My first tests of NPAPIAlignment.c were done on OS X 10.5.7/Intel.  I
just re-ran them on OS X 10.4.11/PPC and got the same results.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Andy, I'm reviewing your patch.  I intend to edit it together with
Matthew's, then do a bit of testing.  But first I have a question -- I
don't understand the following:

+ // NOTE!! Need to use a global for the gWorldPtr due to Print systems
+ // dependence on it, in order to avoid runaway links insure on
+ // subsequent prints the previous gWorldPtr is released.
+
+ if (gWorldPtr != 0)
+ {
+   DisposeGWorld(gWorldPtr);
+   gWorldPtr = 0;
+ }

Style notes:

1) Your patch mixes tabs and spaces.  Generally we don't want any tabs
   in our code.

2) The surrounding code uses indents of 2 spaces.  It's probably best
   for your patch to do the same.

Comment 36

[Andy]

(In reply to comment #35)
> Andy, I'm reviewing your patch.  I intend to edit it together with
> Matthew's, then do a bit of testing.  But first I have a question -- I
> don't understand the following:
> 
> + // NOTE!! Need to use a global for the gWorldPtr due to Print systems
> + // dependence on it, in order to avoid runaway links insure on
> + // subsequent prints the previous gWorldPtr is released.
> +
> + if (gWorldPtr != 0)
> + {
> +   DisposeGWorld(gWorldPtr);
> +   gWorldPtr = 0;
> + }
> 
> Style notes:
> 
> 1) Your patch mixes tabs and spaces.  Generally we don't want any tabs
>    in our code.
> 
> 2) The surrounding code uses indents of 2 spaces.  It's probably best
>    for your patch to do the same.

In the comments "links" should read "leaks". The reason for this logic is that even though the GWorldPtr is owned by the code I introduced and therefore responsible for releasing it, the MacOS system requires the GWorld to remain valid. Unfortunately that doesn't give me the opportunity to release the GWorld until the prinitng code is called for subsequent pages to print. This code then insure that only at most a single GWorld is ever leaked.

Comment 37

[Simon Fraser [no longer active]]

"ensure", not "insure".

Comment 38

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #36)

Thanks for the information.

So you appear to think that the OS will continue to access the
GWorldPtr even after nsObjectFrame::PrintPlugin() returns.  Do you
have any testscases that show this happening?

I notice that WebKit's printing code (in WebNetscapePluginView.mm's
_printedPluginBitmap), like Matthew's, doesn't use a global GWorldPtr.
Since this code is in fairly wide use, I have to assume that the OS
rarely, if ever, wants to use a GWorldPtr after the browser and plugin
are "done" with it.  So we really need a testcase before we can accept
this part of your patch.

This problem might also arise if a plugin caches the GWorldPtr and
tries to use it after the call to NPP_Print().  But I would consider
this a bug in the plugin.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Andy, I notice that you've added the following code to
nsPrintEngine::DoPrint() in nsPrintEngine.cpp:

+ // For MacOSX embedded printing need to make sure all other queued
+ // events are processed before printing to ensure plugins are setup
+ // correctly
+ NS_ProcessPendingEvents(nsnull);

Once again, I don't think we can accept this without a testcase that
shows it's necessary.

Comment 40

[Andy]

Hi Steven,

My understanding is your combining the patch I provided with a previous patch; I think the best approach is for you to pick and choose which elements you want to use, and then try embedded printing from QuickTime and Flash and see the results. If unsuccessful then try adding in other elements of the patches (e.g. like the the removal of all other queued events).

From my experiments I found that flushing the event queue before printing was necessary. It's been a while since I looked at the code but I believe the problem was that a NPP_New event was in the queue after printing had begun; causing the plugins to flush their current settings, resulting in state information being lost and therefore printing not able to proceed.




(In reply to comment #39)
> Andy, I notice that you've added the following code to
> nsPrintEngine::DoPrint() in nsPrintEngine.cpp:
> 
> + // For MacOSX embedded printing need to make sure all other queued
> + // events are processed before printing to ensure plugins are setup
> + // correctly
> + NS_ProcessPendingEvents(nsnull);
> 
> Once again, I don't think we can accept this without a testcase that
> shows it's necessary.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch v2.0

Here's what's probably a preliminary version of my patch to get Flash
plugin printing to work on OS X.

I say "Flash printing" because, as best I can tell, no other plugins
currently support printing on OS X.  I tested printing with current
versions of the QuickDraw, Real, Flip4Mac and Shockwave Director
plugins in Safari 4.0.1 (which has no problem printing Flash plugins),
and none of them printed.  (I also tested all of these in Opera 9.64,
with similar results.  But it appears Opera can't print Flash plugins
either.)  I also found a couple of references to a quote from a WebKit
developer saying more or less the same thing (at
http://trac.webkit.org/changeset/14001 and
http://trac.webkit.org/changeset/14154).

My patch is mostly based on Matthew's, but with two significant
alternations:  1) I restored the align=mac68k directive to both
npapi.h and nsplugindefs.h (as the latter contains structures that
mirror those in the former).  2) I got rid of 'ctx->Paint();', which
paints over what the plugin has just drawn!

There are a few places in my patch where I'm not sure I've done the
right thing -- I marked them with 'XXX' comments.

In particular I don't know whether we should handle QuickDraw printing
differently from CoreGraphics printing.  The WebKit currently handles
both the same way.  But FF 2.0 has code (compiled only in pre-Carbon
builds) that uses a different "protocol" to send platform-specific
information to the plugin via NPP_Print().  In my current patch, I've
chosen (rather arbitrarily) to follow this "protocol" with plugins
that use the QuickDraw drawing mode.  The question seems currently to
be moot -- since there don't appear to be any QuickDraw plugins that
support printing.  But it may not stay that way.

Needless to say, I've found no documentation for either printing
"protocol" -- the QuickDraw one or the CoreGraphics one :-(

A tryserver build will follow in a few hours.

Comment 42

[Steven Michaud [:smichaud] (Retired)]

Attachment: Utility to check the effects of different align= directives (updated)

Here's an updated version of my utility from comment #33.

It tests the effect using the align=mac68k directive (or its absence)
on public structures from both npapi.h and nsplugindefs.h.  The
structures from nsplugindefs.h mirror those from npapi.h and need to
be aligned the same way.

Once again, I found that using or not using align=mac68k only makes a
difference to structures used in printing:

  NPPrint
  NPFullPrint
  nsPluginPrint

As I mentioned above, the align=mac68k directives were removed from
Mozilla code by the patch for bug 281889 (landed on 2006-07-13 12:47).
In fact they were removed from three different files -- npapi.h,
nsplugindefs.h and npupp.h.  But they only ever wrapped two structures
in npupp.h (NPPluginFuncs and NPNetscapeFuncs).  And using or not
using the directive can't make any difference to either of these
structures, or to any likely future additions to them (assuming the
future additions will come at the end and be pointers).  So my patch
doesn't restore the align=mac68k directive to npupp.h.

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my v2.0 patch from comment #41:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla191046-v20/bugzilla191046-v20-macosx.dmg

Comment 44

[Matthew Gregan [:kinetik]]

Comment on attachment 387086 [details] [diff] [review]
Patch v2.0

I haven't tested this yet, but it looks good.

It feels a bit weird packing these structures after shipping a header that left them unpacked for a couple of years.  On the other hand, we're returning to old behaviour (and WebKit's current behaviour), and it fixes printing for the only plugin that we've found that supports it.

A couple of comments:

The gfxContext should be wrapped with a gfxContextAutoSaveRestore (and the XXX gfxContext restore? comments can go).  Also, where we return early due to error, EndNativeDrawing should be called.

I wonder if it's a good idea to introduce a stack disciplined auto class for gfxNativeDrawing like we have for gfxContext.  There are a lot of early returns in PaintPlugin that could be cleaned up by using an auto cleanup class.  gfxNativeDrawingAutoEnd?

Given that we can't test the QuickDraw printing code, I'm not sure if it's worth having.  The brave new plugin future is CoreGraphics anyway, so it seems unlikely that QuickDraw plugins are going to start being able to print in the future.

We should check for errors and bail out if CGColroSpaceCreateWithName, CGBitmapContextCreate, and CGBitmapContextCreateImage fail.

This was all wrong in my original code, so you can blame me while you're updating the patch. :-)

Comment 45

[Andy]

Hi Steven,

I tried your build with my Flash test files and it looks great, good work and thanks for implementing this.


(In reply to comment #43)
> Here's a tryserver build made with my v2.0 patch from comment #41:
> https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla191046-v20/bugzilla191046-v20-macosx.dmg

Comment 46

[Steven Michaud [:smichaud] (Retired)]

Andy, glad to hear it, and you're welcome!

I'll shortly be posting another patch that follows Matthew's
suggestions.  I'll wait to post another tryserver build until we're
close to (or at) the final version of the patch.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch v2.1 (follow Matthew's suggestions)

> The gfxContext should be wrapped with a gfxContextAutoSaveRestore
> (and the XXX gfxContext restore? comments can go).  Also, where we
> return early due to error, EndNativeDrawing should be called.

> We should check for errors and bail out if
> CGColroSpaceCreateWithName, CGBitmapContextCreate, and
> CGBitmapContextCreateImage fail.

Done.  I also changed the patch to consistently use the "::"
convention for system calls.

> Given that we can't test the QuickDraw printing code, I'm not sure
> if it's worth having.  The brave new plugin future is CoreGraphics
> anyway, so it seems unlikely that QuickDraw plugins are going to
> start being able to print in the future.

I've changed my mind and now agree with you.  It helps that QuickDraw
plugins should also be able to use the GWorldPtr that gets passed to
CoreGraphics plugins.  Plugin vendors can figure out what to expect by
looking at WebKit and Mozilla.org source.

It's irritating that the way we (and WebKit) pass the GWorldPtr isn't
any kind of standard (isn't an official part of the NPAPI).  But the
GWorldPtr business could easily *become* a (defacto) standard ... at
least until the GWorldPtr type goes away with the transition to 64-bit.

> I wonder if it's a good idea to introduce a stack disciplined auto
> class for gfxNativeDrawing like we have for gfxContext.  There are a
> lot of early returns in PaintPlugin that could be cleaned up by
> using an auto cleanup class.  gfxNativeDrawingAutoEnd?

Let's save this for another bug :-)

> This was all wrong in my original code, so you can blame me while
> you're updating the patch. :-)

No problem :-)

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Does it mean it will fix bug 134002 at least for OS X?

Comment 49

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #48)

Is there a separate Print Preview in FF Mac distros?

All my tests of this bug's patch have been using the OS's print
preview (File : Print : Preview) -- since I don't have a printer.  It
works just fine.  But (from the patch at bug 134002) it doesn't seem
that's what you're talking about.

Maybe bug 134002 (which is very old) is now obsolete.

As for Java, the JEP doesn't support printing.  And I'm unlikely to be
able to add this capability anytime soon.

Comment 50

[Matthew Gregan [:kinetik]]

Comment on attachment 387268 [details] [diff] [review]
Patch v2.1 (follow Matthew's suggestions)

Awesome, thanks.  I tested the latest patch a bit and it worked fine.

One thing I found: I think we should pass kCGImageAlphaPremultipliedFirst to CGBitmapContextCreate.  When trying to print Flash with a transparent background, we end up with a black box (see http://www.webdesign.org/web/flash/tutorials/transparent-flash-animation.7824.html for an example).  I assume this is because Flash isn't trying to paint anything to print?  There's no plugin content in Safari either, but the plugin box is transparent, so at least you can see the content underneath it.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch v2.2 (use kCGImageAlphaPremultipliedFirst)

> One thing I found: I think we should pass
> kCGImageAlphaPremultipliedFirst to CGBitmapContextCreate.

You're right.  And WebKit does its equivalent -- which I hadn't
noticed before.

Thanks for the transparent example.  I didn't know it was possible to
composite a plugin with something in the browser.

I wonder why plugin compositing works in ordinary display but not in
printing.  I suppose this is a problem in the Flash plugin.

Another tryserver build will follow in a few hours.

Comment 52

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my v2.2 patch:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla191046-v22/bugzilla191046-v22-macosx.dmg

Comment 53

[Josh Aas]

Comment on attachment 387469 [details] [diff] [review]
Patch v2.2 (use kCGImageAlphaPremultipliedFirst)

> // XXX platform specific printing code

Please remove the XXX which normally indicates some sort of problem.

>+  gfxContext *ctx = aRenderingContext.ThebesContext();

Add a null check here.

Otherwise looks good, thanks.

Comment 54

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 387469 [details] [diff] [review]
Patch v2.2 (use kCGImageAlphaPremultipliedFirst)

Landed on trunk (mozilla-central) with Josh's changes:
http://hg.mozilla.org/mozilla-central/rev/860db9cc9e2b

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Should this land on the 1.9.1 branch?

(I'd like to request wanted1.9.1.X, but for some reason the UI won't let me do that.)

Comment 56

[Samuel Sidler (old account; do not CC)]

(In reply to comment #55)
> (I'd like to request wanted1.9.1.X, but for some reason the UI won't let me do
> that.)

See http://groups.google.com/group/mozilla.dev.planning/browse_frm/thread/0f439307e9db1f6b#

But if it applies and you want it, say why when asking for approval. :)

Comment 57

[Josh Aas]

It is tempting to take this but it doesn't affect security or stability. Should probably not go on 1.9.1 but we probably should take the NPAPI header updates on 1.9.1.

Steven, can you post a header-only patch and request 1.9.1?

Comment 58

[Josh Aas]

Andy - thanks for getting the ball rolling on this!

Steven - please make sure to credit Andy in your commit message.

Comment 59

[Steven Michaud [:smichaud] (Retired)]

> Steven - please make sure to credit Andy in your commit message.

Too late for that -- the patch has already landed.

> Andy - thanks for getting the ball rolling on this!

Yes, Andy.  You built a fire under our asses :-)

Comment 60

[Steven Michaud [:smichaud] (Retired)]

Attachment: Headers only patch for 1.9.1 branch

Comment 61

[Steven Michaud [:smichaud] (Retired)]

Attachment: Headers only patch for 1.9.0 branch

It's unlikely this bug's patch (attachment 387469 [details] [diff] [review]) will ever be landed
on the 1.9.1 or 1.9.0 branches (i.e. in FF 3.0.X or FF 3.5.X).  But it
contains changes to a couple of public NPAPI header files that clean
up the mess I described in comment #32.  These changes should be
landed on all of our "live" branches (ones from which we're still
making releases).

These header-only patches won't currently make any functional
difference on the 1.9.1 and 1.9.0 branches.  But they make our public
NPAPI header files uniform across all our "live" branches.  And they
forestall possible trouble in the future.

As I said in comment #32, we shouldn't have removed the align=mac68k
macros -- at least without bumping the NPAPI version number (and some
sort of public discussion).  We may not have realized that doing so
would make any difference.  And by chance it only does for structures
used in printing -- see comment #33 and comment #42.

But now that we've started supporting plugin printing on OS X, this
issue has come back to bite us.  We might be able to get away with
using the align=mac68k macros to wrap just those structures for which
they make a difference.  But that's dangerous -- what happens if we
add new structures that fall within the scope of the align-mac68k
macros in our old NPAPI header files (and those the WebKit uses), but
outside their scope in our own NPAPI header files?  So it's best to
(mostly) restore these macros to their original scope, which is also
the scope they have in the NPAPI header files used by WebKit.

As I noted in comment #42, I haven't restored the align=mac68k macros
to npupp.h.  But they only ever wrapped two structures in npupp.h
(NPPluginFuncs and NPNetscapeFuncs).  And using or not using the them
can't make any difference to either of these structures, or to any
likely future additions to them (assuming the future additions will
come at the end and be pointers).

We're very fortunate that, for now, the presence or absence of the
align=mac68k macros only makes a difference to printing, that we
haven't supported printing before now, and that (as far as we know)
only one plugin (the Flash plugin) supports printing on OS X.  So,
though the mess described in comment #32 was created three years ago,
it's still possible to clean it up now without (as far as we know)
causing any incompatibilities.

Comment 62

[Daniel Veditz [:dveditz]]

Comment on attachment 389705 [details] [diff] [review]
Headers only patch for 1.9.0 branch

All API changes require super-review now.

Those are compelling arguments for cleaning things up even on old branches. Since we can't keep changing this I'd like an explicit sr that yes, it is OK to change the headers mid-stream on the old branches, and yes, this is exactly the change we want.

Measure twice, cut once.

Comment 63

[Johnny Stenback (:jst)]

Comment on attachment 389705 [details] [diff] [review]
Headers only patch for 1.9.0 branch

Given the mess we ended up in with these structs, this seems like the best thing we can do for branches. sr=jst

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Checking the fix with the most recent trunk nightly build on OS X shows that some flash content is still not printed or as the wrong size. See the following page: http://news.bbc.co.uk/2/hi/africa/8173717.stm. The first flash object is missing and the second one is printed much smaller as the reserved space which results in a black background around the content. I believe it should be covered in another bug. Is there already on filed?

Comment 65

[Steven Michaud [:smichaud] (Retired)]

The same problems happen with this URL in Safari.  So they must (presumably) be the fault of the Flash plugin.

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Steven, looks like that http://wetter.rtl.de/redaktion/wettercockpit/index.php?md5=ed44c5fabc392705befd14fa9a885699 is also a bad testcase. Each Flash object is disturbed in the preview output. Is that a new bug?

Comment 67

[Steven Michaud [:smichaud] (Retired)]

Once again, the same problem happens in Safari.  So it clearly isn't our bug.

This problem does seem to be different from the one your reported in comment #64, though.

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You are right. Sorry, I missed that. Finally I was able to verify the fix on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090802 Minefield/3.6a1pre ID:20090802031829

http://edition.cnn.com/2009/WORLD/meast/08/04/iran.americans.profile/index.html

I'll update the litmus tests when we have branched the test-runs.

Comment 69

[Daniel Veditz [:dveditz]]

Comment on attachment 389704 [details] [diff] [review]
Headers only patch for 1.9.1 branch

Approved for 1.9.1.3, a=dveditz for release-drivers

Comment 70

[Daniel Veditz [:dveditz]]

Comment on attachment 389705 [details] [diff] [review]
Headers only patch for 1.9.0 branch

Approved for 1.9.0.14, a=dveditz for release-drivers

Comment 71

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389704 [details] [diff] [review]
Headers only patch for 1.9.1 branch

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6eb8f520ea3b

Comment 72

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389705 [details] [diff] [review]
Headers only patch for 1.9.0 branch

Landed on the 1.9.0 branch:
Checking in modules/plugin/base/public/npapi.h;
/cvsroot/mozilla/modules/plugin/base/public/npapi.h,v  <--  npapi.h
new revision: 3.49; previous revision: 3.48
done
Checking in modules/plugin/base/public/nsplugindefs.h;
/cvsroot/mozilla/modules/plugin/base/public/nsplugindefs.h,v  <--  nsplugindefs.h
new revision: 1.34; previous revision: 1.33
done

Comment 73

[Steven Michaud [:smichaud] (Retired)]

I didn't add "fixed1.9.1.3" or "fixed1.9.0.14" keywords, since the
headers-only patches don't actually fix this bug.

Should I have done so?

Comment 74

[Samuel Sidler (old account; do not CC)]

Yes. We need a way to track that the checkin has happened.

Comment 75

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.1 and 1.9.0 based on check-ins and no regressions.

Comment 76

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Updated Litmus tests 4021, 5979, and 8022 for the remaining part in bug 134002.