Closed Bug 468568 Opened 16 years ago Closed 12 years ago

[@font-face] printing pages with downloadable fonts doesn't render all fonts on the page

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: jtd, Assigned: smaug)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(16 files, 16 obsolete files)

Console output and backtrace from GDB
15.29 KB, text/plain
Details
GDB backtrace from gfxUserFontSet constructor
6.58 KB, text/plain
Details
Console output including tree frames
16.45 KB, text/plain
Details
Frame dump before reconstruction and after reflow
14.02 KB, text/plain
Details
View tree dump
1.35 KB, text/plain
Details
Reflow analysis - where is the RootView set?
5.02 KB, text/plain
Details
WIP 6 - toolkit part
1.10 KB, patch
Details | Diff | Splinter Review
Updated test files
148.08 KB, application/zip
Details
Sample printout showing the issue
376.90 KB, image/jpeg
Details
small testcase
325 bytes, text/html
Details
Additional gfxDWriteFontList patch
6.93 KB, patch
Details | Diff | Splinter Review
Part 0.6: Assert when ScheduleViewManagerFlush is called on a non-root
1.94 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Part 0.7: Fix nsPagePrintTimer inheritance
1.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 0.8: Remove invalid assertion about matching containers
1.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Updated main patch
34.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part 0.5 v2
5.87 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review 
It appears that in some cases, fonts on a page containing a number of downloadable fonts doesn't print correctly, the printed page is rendered using fallback fonts instead.

Steps:

1. Open URL
2. Wait for the page to finish loading
3. Select Print
4. Click on the Preview button

Result: most fonts on the page render, some do not
Flags: blocking1.9.1?
Summary: printing pages with downloadable fonts doesn't render all fonts on the page → [@font-face] printing pages with downloadable fonts doesn't render all fonts on the page
Hrm.  While this is a crappy bug, it sounds like something that we could ship with if it came to that -- John, what do you think?  Seems like the kind of thing that should go to the back of the queue.
This is pretty hard to fix since we don't really support incremental reflow of paginated layouts. We can't just wait for fonts to load before reflowing, since it's reflow that triggers textrun construction and thus font loading.

A good approach would be to reflow once, triggering loads of all needed fonts, and then once all fonts have loaded, blow away the frame tree and reflow again from scratch. But that would not be easy at this stage and I agree we can ship with this not fixed.
I think we can work around this by setting up a simple cache of activated fonts, a set of references to <font url, font handles>.  In this case, we wouldn't need to rely on reflow to make printing work since the cached fonts would already be around and we could make the font lookup succeed immediately.  This is also a good thing for sites that use the same set of downloadable fonts across all their pages, the download would only need to occur on the load of the first page, navigating to subsequent pages would pull in the already activated fonts.
Note: this is what Webkit does, although I'm not clear on the details of when a font resource gets deactivated.
Yes, we want that cache. I was even thinking of mentioning it here. However, it won't solve the printing problem completely since it's possible for pages to use CSS media rules to specify different fonts for printing. In fact, that sounds quite plausible.

BTW I don't think the cache can be as simple as a map from URLs to font handles. There are issues with access control, and also I think we should perform HTTP cache validation before we use a cached font. The latter needs to work like the image cache does (and I don't know exactly how that works).
Assignee: nobody → jdaggett
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: mozilla1.9.1b3 → ---
Moving this to a P1 wanted based on discussion with John.
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Priority: P3 → P1
I think last 2 "dupes" aren't dupes of this bug, they have nothing about printing and multiple fonts, they are about cached fonts "appearing" a few seconds later than fallback font does.
When the fix in bug 487667 lands, we can implement the fix in comment #2. That would be better than the workaround in comment #3 because if someone uses @font-face in a rule that's specific to print media, then the workaround in comment #3 wouldn't help but with the approach in comment #2 we'd be able to load and use the font.
Depends on: 487667
blocking2.0: --- → ?
Won't block the release on this, since we've released with it before, but we should probably look into it.
blocking2.0: ? → -
Hello,

Is there any progress on this? Would be very useful to be able to print web fonts.
OS: Mac OS X → All
Michael, would you be interested in taking this on?
What you need to do here is, after the reflow check if there are any pending font loads. If there are, instead of printing immediately, we need to wait until all pending font loads are finished, and then reconstruct frames for the document and print.
So I've been spending all day reading font code, which is complex to say the least, and I'd like some clarifications.

gfxMixedFontFamily contains all fonts on a page?
It has a method IsLoaded() which checks if there are no proxies. All proxies eventually become proper entries, even if the loading fails? In printing, should I respect the use of fallback fonts if the loading fails/is slow?

I haven't yet found the link between gfxMixedFontFamily and a given document.

I also notice that font sets use a generational counter, what is that used for?
(In reply to comment #20)
> So I've been spending all day reading font code, which is complex to say the
> least, and I'd like some clarifications.
> 
> gfxMixedFontFamily contains all fonts on a page?

No, gfxMixedFontFamily represents a CSS font-family that was defined using @font-face (rather than a family name that is found as an installed font on the platform). There'll be a gfxUserFontSet that contains the complete set of @font-face families that are defined, with one gfxMixedFontFamily for each, and one or more proxy or real font entries per face within each such family. When a gfxFontGroup is resolving CSS font-family names, it will check the gfxUserFontSet (if there is one) first, then look for installed platform fonts.

> It has a method IsLoaded() which checks if there are no proxies. All proxies
> eventually become proper entries, even if the loading fails?

No, if all sources fail for a given proxy, it remains a proxy - and will therefore not be used for rendering; we'll fall back to the next family in the gfxFontGroup.

> In printing,
> should I respect the use of fallback fonts if the loading fails/is slow?

I suppose so - it's better to print with fallbacks than to print nothing. You'll probably need some kind of timeout mechanism whereby you do layout (in order to initiate font loads), then wait some reasonable amount of time to allow the loads to complete, but eventually go ahead and print (using fallbacks for any fonts that still haven't loaded).

> I haven't yet found the link between gfxMixedFontFamily and a given document.

The gfxUserFontSet.

> I also notice that font sets use a generational counter, what is that used
> for?

To track whether the font set has changed (e.g. rules added/removed via script) since a gfxFontGroup resolved its font-family names; if so, we need to rebuild the group's array of fonts as the CSS family names may resolve to different @font-face or installed families.
Thanks John, I'm starting to get the hang of the code now.

Perhaps the easiest way of fixing is for me to poll gfxFontGroup's ShouldSkipDrawing(), waiting for that to become false. But unless I'm missing something, font groups are only associated with a frame, not the document?

If so, then the other option is to get the gfxUserFontSet from the pres context, then write a function checking that there are either no proxies or they're all set to fallback. I still don't understand the user font set code very well, I'm hoping there's a function already that can tell me the first font entry that we are either waiting for, or can definitely use now.
(In reply to comment #22)
> Perhaps the easiest way of fixing is for me to poll gfxFontGroup's
> ShouldSkipDrawing(), waiting for that to become false. But unless I'm
> missing something, font groups are only associated with a frame, not the
> document?

That sounds right, as a gfxFontGroup is in effect the concrete realization of a particular collection of CSS font-* properties, and these come from the frame's style.

> If so, then the other option is to get the gfxUserFontSet from the pres
> context, then write a function checking that there are either no proxies or
> they're all set to fallback. I still don't understand the user font set code
> very well, I'm hoping there's a function already that can tell me the first
> font entry that we are either waiting for, or can definitely use now.

I don't think there's anything like that ready-made. But it should be feasible to iterate over the gfxUserFontSet's families, and for each family, iterate over its faces, checking the loading status of each one that's a proxy (rather than a "real" fontentry).

Note that there may be families/faces in the set that are not used anywhere in the document, in which case they'll remain as proxies and never even start to load. So you'd want to ignore those; you only care if there are proxies whose loading state is STARTED or ALMOST_DONE, indicating that they were actually required during reflow, but haven't either finished loading or timed out yet.
Can't we track the pending font loads in nsUserFontSet and provide a method to check whether there are any? Plus have it notify something when all pending font loads are done?
We'll need a test to ensure that documents containing subframes which use downloadable fonts print correctly.
(In reply to comment #25)
> We'll need a test to ensure that documents containing subframes which use
> downloadable fonts print correctly.

...We can have tests for printing?

Also, do user font sets run across subframes or are they associated with the document they're in?
They're per prescontext, therefore per document.
Maybe this would be hard to test, I guess.
So where I'm leaning is to write code that checks the status of all user font proxies, and if all fonts are OK to display, call PresShell::ReconstructFrames() then continue the printing.

Since font sets are per-document, tough, I'm not sure how to check for font sets across subframes. I wouldn't be surprised if there's a function I'm missing that would make this easy...
s/tough/though
(In reply to comment #29)
> So where I'm leaning is to write code that checks the status of all user
> font proxies, and if all fonts are OK to display, call
> PresShell::ReconstructFrames() then continue the printing.
> 
> Since font sets are per-document, tough, I'm not sure how to check for font
> sets across subframes. I wouldn't be surprised if there's a function I'm
> missing that would make this easy...

The printing code has code to walk subframes to set up the print presentations.
OK, now this is getting frustrating, not only is print preview not async (and I'm still trying to figure out if it is possible), but PresShell::ReconstructFrames doesn't seem to do anything. I wonder if I'm missing something.
As noted in duplicate bug 678658, this can happen even when there's only one downloadable font in use.

For reference, the self-contained testcase on that bug is attachment 552913 [details]
Instead of trying to reconstruct frames, could we instead try to ensure that the font loads are triggered prior to reflow, and then just delay reflow until they've loaded?
I suppose we could walk the frame tree to do that. Wouldn't that be more code though?
Any update on if this will be solved for the next release. The community is really starting to hammer on us web font services regarding not allowing pages to be printed.

Thanks
Blocks: 698375
fwiw printing in this test case
http://oli.jp/bugs/mozilla/font-face-printing.html
works *if local() URLs are included and the fonts are installed*. Doesn’t help site visitors, but it did when I wanted to make a PDF archive :)
Blocks: mathjax
No longer blocks: mathml-in-mathjax
This bug is blocking the new printing infrastructure for PDF.JS that is based on the new mozPrintCallback. (With the mozPrintCallback, the canvas/pages for PDF.JS are painted while the printing takes place. But as the fonts are not available, the printed output doesn't look right.)

I don't understand why downloaded fonts, that has finished downloading before the printing start, don't show up while printing. Are the fonts missed out when creating the static document?

Can someone give me pointers what code I should look at for make the copy fonts to static document work?
(In reply to Julian Viereck from comment #41)
> I don't understand why downloaded fonts, that has finished downloading
> before the printing start, don't show up while printing. Are the fonts
> missed out when creating the static document?

It's because we kick off new loads for each font in every presentation that uses the font. Those loads normally hit in Necko's cache, but they still happen.

> Can someone give me pointers what code I should look at for make the copy
> fonts to static document work?

That would be complicated to do and wouldn't fully solve the problem, since in general there could be fonts loaded in @media print CSS rules, so the original document wouldn't have loaded them yet.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
>
> > Can someone give me pointers what code I should look at for make the copy
> > fonts to static document work?
> 
> That would be complicated to do and wouldn't fully solve the problem, since
> in general there could be fonts loaded in @media print CSS rules, so the
> original document wouldn't have loaded them yet.

The total solution is to implement the reflow stuff you've mentioned in comment#2, but that look like a complicated job (and it took  Michael Ventnor  some time to get going...), that's why I want to make multiple small steps/bugs out of this bug and fix one after the other.

Can we solve the problem for fonts, that are ready to use before the printing starts, first and then as a second iteration handle the cases of finish loading fonts first/start loading of the fonts used in @media section?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> (In reply to Julian Viereck from comment #41)
> > I don't understand why downloaded fonts, that has finished downloading
> > before the printing start, don't show up while printing. Are the fonts
> > missed out when creating the static document?
> 
> It's because we kick off new loads for each font in every presentation that
> uses the font. Those loads normally hit in Necko's cache, but they still
> happen.

I've rebased my patches against current MC. Fonts are loaded using a data:url in PDF.JS. Loadig data-urls from fonts happen sync since some time now. However, the fonts still don't show up during printing. 

To test this, I've added a div that uses one of the loaded fonts. In the browser I see the text has different font applied, but during print/pre-view the font is not used.

If the fonts are loaded sync, why are they still missing during printing/in print-preview?
I don't know.

Maybe we can get John or Jonathan to fix this bug :-).
I've debugged the font loading code today. The data:urls are rejected because of some origin problems. If I remove the last if check in nsFontFaceLoader::CheckLoadAllowed:

    http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#274

then the data:url fonts show up correctly in preview. Looking into why that fails for preview tomorrow.
Hacky workaround to get data:url fonts working. This tests if the font-url is a data-url and don't perform the NS_CheckContentLoadPolicy check in this case.

Is such a hacky fix for data-urls "landable"? This is required for the PDF.JS printing use case.

I tried to debug why NS_CheckContentLoadPolicy rejects the font-url during print/print-preview but don't know how to do the debugging :/ If someone gives me some pointers, I might be able to look into it.
Attachment #624183 - Flags: feedback?
Attachment #624183 - Flags: feedback? → feedback?(dbaron)
Attachment #624183 - Attachment is patch: true
Comment on attachment 624183 [details] [diff] [review]
Hacky workaround to get data:url fonts working

>+  aTargetURI->GetSpec(spec);
[...]
>+  // XXX Workaround: Don't check policy for data:url-fonts.
>+  if (spec.Find("data:font") != 0) {

Assuming that we're on-board with the high-level idea of whitelisting data URIs, as a stopgap:

Generally, it's better to check for URI flags rather than for the specific "data" protocol.  I suspect you want to use the same policy that we used in bug 693940 for flagging which resources are loadable in SVG Images.  In particular: you'd want to allow local URIs that also satisfy one of URI_INHERITS_SECURITY_CONTEXT or URI_LOADABLE_BY_SUBSUMERS.  (bz can confirm whether this makes sense)

See attachment 574113 [details] [diff] [review] -- you likely want to add a very similar check to the one added there.
(In reply to Julian Viereck from comment #46)
> I've debugged the font loading code today. The data:urls are rejected
> because of some origin problems. If I remove the last if check in
> nsFontFaceLoader::CheckLoadAllowed:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.
> cpp#274
> 
> then the data:url fonts show up correctly in preview. Looking into why that
> fails for preview tomorrow.

I do not think putting in a hack like this makes sense.  There's either a problem with the existing code or you need to work out how to set up the principal correctly.  I do not think hacking around the check load policy for data url's is correct at all.

Given that data url's work in normal pages, I think you need to figure out why that check is failing only in the pdf.js case and determine whether the check is inappropriate or whether there's something that pdf.js isn't doing correctly.

I also think you need to open another bug and not use this one to implement a solution that's specific for pdf.js.
Attachment #624183 - Flags: feedback-
(In reply to John Daggett (:jtd) from comment #49)
>
> Given that data url's work in normal pages, I think you need to figure out
> why that check is failing only in the pdf.js case and determine whether the
> check is inappropriate or whether there's something that pdf.js isn't doing
> correctly.
> 
> I also think you need to open another bug and not use this one to implement
> a solution that's specific for pdf.js.

Using data-url fonts are not showing up in print/print-preview in general. This is not related to pdf.js and I'm sorry if my statements implied that.  

I'm using a data-url font from PDF.JS and a data-url font from the bug for implementing sync-loading of data-url fonts from here:

    https://bug512566.bugzilla.mozilla.org/attachment.cgi?id=605275

Both don't show up in print/print-preview.

If we can't accept the hack, I will look into why the policy is not accepted.
I talk to :smaug about this loading-fonts-get-blocked issue on IRC:

  > smaug: jviereck: contentpolicies should prevent all resource loading on print documents. print documents are data document and there is nsDataDocumentContentPolicy
  [...]
  > smaug: jviereck: well, you can't really load anything, at least not in current setup

That would explain why loading any fonts fails :/

As far as I can see there are two possible ways to fix this:
a) allow static/print documents to make network requests
b) while cloing/creating the static document, clone the already loaded fonts to the new documents as well, such that no network request needs to be done.

As the document is static, it makes sense to prevent network requests and therefore implementing a) might not be the best idea.

Therefore I've looked into b). Here is a short list of steps how I would try to solve this:

1) Inside nsIDocument::CreateStaticClone[1] add another for-loop, that iterates over all the fontFamilies in the current document's gfxUserFontSet's mFontFamilies entries.
2) for all the fontFamilies entries in mFontFamilies, add a loop over all fontEntries
3) If the fontEntrie is loaded and ready to use, then the same fontEntrie reference is added to the new gfxUserFontSet on the clonedDocument. If there is no fontFamilie on the cloned gfxUserFontSet for the font, it is created
4) After the two loops are done, a new flag |bool mIgnoreAddFontFace| is raised on the cloned gfxUserFontSet. This causes an early quit in the gfxUserFontSet::AddFontFace function, such that adding any other fontFace, then the ones during the clone process, to the cloned document are prevents. Otherwise the fontFaces get added while processing the cloned style sheets twice.

Does this plan make sense? Things I'm worried about:

1) Is reusing the same gfxFontEntry object for the original and cloned document the right decision?
2) If there is no loading step and the fontEntries get added right away, does that break some logic in the css-style code? E.g. the code in  nsUserFontSet::ReplaceFontEntry[2] won't run, as there is no ProxyFontEntry to be replaced by an gfxFontEntry.

[1]: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8084
[2]: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#699
FWIW, I definitely like the solution of *not* having to hit the network better. That way we also don't have to worry about waiting for the load to finish before doing the layout.
Assignee: jdaggett → nobody
Julian, as mentioned in comments 3, 4, 5 I think a better way to fix this is to keep around a cache of activated fonts, mapping URL to the activated resource.  That way the font load will just work for the cloned document case and it will improve performance for cases where the same font is used across a set of pages.

That won't solve the case where @media rules lead to different fonts being loaded in the print case, but that's a bit of an edge case.
I think allowing print documents to make network requests is the only reasonable approach here. This affects not only fonts but anything else loaded by CSS (background images, for example). It has to be done.
I concur - adding separate caches for printing will end with tons of extra code and tears. Without an overwhelming reason not to allow network loads from print documents, that's what we should do.
Comment on attachment 624183 [details] [diff] [review]
Hacky workaround to get data:url fonts working

I agree with comment 49.
Attachment #624183 - Flags: feedback?(dbaron) → feedback-
... and also comment 54, now that I see the newer comments.
I've started working on a proper fix. Network requests are now allowed and happen -> fonts get loaded. However, it's about to figure out that all resources are loaded and then do another reflow.

To listen to the network progress, I've implemented the nsIWebProgressListener interface for the nsPrintEngine and then within the nsPrintEngine::ReflowPrintObject added the listener to the new created docShell:

    nsCOMPtr<nsIWebProgress> webProgress = do_QueryInterface(aPO->mDocShell);

    webProgress->AddProgressListener(
      static_cast<nsIWebProgressListener*>(this),
      nsIWebProgress::NOTIFY_STATE_REQUEST);

But the progressListener is *NEVER* called. After some long debugging and staring at object pointers it turned out, that the actual load of the resources (Fonts, Images, ...) for the print document happen in the nsDocLoader of the original document (!!!). This is also true for iFrames on the page - during printing, the resources get loaded in the former iFrame nsDocLoader object.

I don't really have an idea why we stick to the nsDocLoader of the original document. Is something going wrong when we clone the document/the style sheets? Is there a way one can force the print docShell's content to use the nsDocLoader of the docShell explicit?

Hopefully this rings a bill in someone's head :/
(In reply to Julian Viereck from comment #58)
> I don't really have an idea why we stick to the nsDocLoader of the original
> document. Is something going wrong when we clone the document/the style
> sheets? Is there a way one can force the print docShell's content to use the
> nsDocLoader of the docShell explicit?
> 
> Hopefully this rings a bill in someone's head :/

A possible thing to do:
* Query the original document's docShell and add a network listener
* Set a flag before the reflow starts and unset the flag after the reflow ends
* Record all network requests that start while the flag is raised --- these are all the network requests required for the printing document
* Continue to listen to the original docShell
* Once all recorded requests stop, do another reflow and start the actual printing

From looking at the logs of the network requests the "record all starting network requests during the reflow phase" seems okay to do and as Gecko is (mostly still) single threaded, there shouldn't any other request during the reflow get started --- do these assumption hold and does it make sense to implement the "are all network resources loaded" logic as pointed out in the bulit list?
(In reply to Julian Viereck from comment #59)
> (In reply to Julian Viereck from comment #58)
> > I don't really have an idea why we stick to the nsDocLoader of the original
> > document. Is something going wrong when we clone the document/the style
> > sheets? Is there a way one can force the print docShell's content to use the
> > nsDocLoader of the docShell explicit?
> > 
> > Hopefully this rings a bill in someone's head :/
> 
> A possible thing to do:
> * Query the original document's docShell and add a network listener
> * Set a flag before the reflow starts and unset the flag after the reflow
> ends
> * Record all network requests that start while the flag is raised --- these
> are all the network requests required for the printing document
> * Continue to listen to the original docShell
> * Once all recorded requests stop, do another reflow and start the actual
> printing
> 

Isn't it possible that additional resources may turn out to be needed as a result of some of those requests, so your initial "list of requests required" might need to grow? E.g. if one of those requests was for a linked stylesheet, it might call for additional images/fonts/etc. Or if a @font-face resource fails to load, or turns out to be unusable (invalid font data, for example), we then kick off a load for the next resource in the @font-face rule's source list.

So I don't think it's valid to assume that the complete set of resources you need are those requested during that initial reflow.

Also, a separate issue but you'll need to have some strategy in place to deal with the case where one (or more) of the network requests fails, or is taking an unreasonably long time. This should be rare - in most cases, the resources will already be cached and so the requests will complete very quickly - but we need to be prepared to handle it somehow.
> I don't really have an idea why we stick to the nsDocLoader of the original document.

When we clone the document, it looks like we set the clone's loadgroup to that of the original document (see nsDocument::CloneDocHelper).

If we have a separate docshell/loadgroup we want to be using, then we should set the static clone's loadgroup to that docshell's loadgroup.  That's what determines where progress notifications about loads for the document are sent.
No longer blocks: mathjax, 698375
No longer depends on: 460037, 487667
And in particular, the loadgroup passed to Reset or ResetToURI should be the one we really want the loads to end up in.

Note that there are likely to be various security implications there, btw (think things like the loadgroup being used to detect who's doing the load or to find the window to use as a parent for security dialogs or whatnot).  So we need to double-check our print docshell setup, likely.
Blocks: mathjax, 698375
Depends on: 460037, 487667
(In reply to Boris Zbarsky (:bz) from comment #61)
>
> If we have a separate docshell/loadgroup we want to be using, then we should
> set the static clone's loadgroup to that docshell's loadgroup.  That's what
> determines where progress notifications about loads for the document are
> sent.

Seem like I got the loadGroups doing the right thing for fonts by basically doing this:

    diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
    --- a/content/base/src/nsDocument.cpp
    +++ b/content/base/src/nsDocument.cpp
    @@ -7589,26 +7592,39 @@ nsDocument::CloneDocHelper(nsDocument* c
 
       // Set URI/principal
       clone->nsDocument::SetDocumentURI(nsIDocument::GetDocumentURI());
       // Must set the principal first, since SetBaseURI checks it.
       clone->SetPrincipal(NodePrincipal());
       clone->mDocumentBaseURI = mDocumentBaseURI;
 
       if (mCreatingStaticClone) {
    -    nsCOMPtr<nsIChannel> channel = GetChannel();
    -    nsCOMPtr<nsILoadGroup> loadGroup = GetDocumentLoadGroup();
    +    printf("nsDocument::CloneDocHelper clone:%p\n", clone);
    +
    +    nsCOMPtr<nsILoadGroup> loadGroup;
    +    nsCOMPtr<nsIChannel> channel;
    +    
    +    nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocumentContainer);
    +    NS_ASSERTION(docShell, "There should be a docShell");
    +
    +    nsCOMPtr<nsIDocumentLoader> docLoader = do_QueryReferent(mDocumentContainer);
    +    NS_ASSERTION(docLoader, "There should be a docLoader");
    +
    +    docLoader->GetLoadGroup(getter_AddRefs(loadGroup));
    +    docLoader->GetDocumentChannel(getter_AddRefs(channel));
    +
         if (channel && loadGroup) {
           clone->Reset(channel, loadGroup);
         } else {
           nsIURI* uri = static_cast<const nsIDocument*>(this)->GetDocumentURI();
           if (uri) {
             clone->ResetToURI(uri, loadGroup, NodePrincipal());
           }
         }
    +
         nsCOMPtr<nsISupports> container = GetContainer();
         clone->SetContainer(container);
       }
 
       // Set scripting object
       bool hasHadScriptObject = true;
       nsIScriptGlobalObject* scriptObject =
         GetScriptHandlingObject(hasHadScriptObject);


However, the loadGroup for background-images still loads from the origin document. Any idea what to change for that?
In the (new) nsPrintEngine::OnStateChange(...) function I increment a new field |mPrt->mLoadCounter| by 1 if a new request starts and decrement it by 1 if a request ends. If the value becomes zero, then I assume all requests are done and if there are no other requests after some time, I start the final reflow followed by the printing.

Incrementing the |mPrt->mLoadCounter| variable results reproduceable to crashing FF and showing "Segmentation fault: 11". Any idea why this is the case?
> Seem like I got the loadGroups doing the right thing for fonts by basically doing this:

Er... I have no idea why that would work, actually.  mDocumentContainer should be the docshell of the non-printing document in that case, no?  So this should be setting the loadgroup to the wrong thing.

Oh, and you don't want to mess with the channel the way you did.

As far as background images, those should just use the loadgroup of the document of the prescontext involved.  Which should really be the loadgroup of the clone, I'd think.
(In reply to Boris Zbarsky (:bz) from comment #65)
> 
> Er... I have no idea why that would work, actually.  mDocumentContainer
> should be the docshell of the non-printing document in that case, no?  So
> this should be setting the loadgroup to the wrong thing.
 
See the code here nsIDocument::CreateStaticClone code: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8071

8077   // Make document use different container during cloning.
8078   nsCOMPtr<nsISupports> originalContainer = GetContainer();
8079   SetContainer(aCloneContainer);
8080   nsCOMPtr<nsIDOMNode> clonedNode;
8081   nsresult rv = domDoc->CloneNode(true, 1, getter_AddRefs(clonedNode));
8082   SetContainer(originalContainer);

That means, during the clone process, the docShell is the docShell of the printDocument, not the original one.

> Oh, and you don't want to mess with the channel the way you did.

Sorry, I'm still new to this Gecko-Business :/ What's the right way to do it?
> That means, during the clone process, the docShell is the docShell of the printDocument,

For both documents?  <sigh>

That needs some comments at both places noting the interdependency

> What's the right way to do it?

The old way: the document channel is the same for both documents.
Blocks: 748935
Attached patch WIP 1.1 (obsolete) — — Splinter Review 
WIP Patch - What this does:

* enable loading content within print context in |nsDataDocumentContentPolicy::ShouldLoad|
* fix |nsDocument::CloneDocHelper| to use docLoader of the cloned docShell --- this makes loading of fonts happen in the cloned docShell but images still load in the original docShell
* I was rewritting |nsPrintEngine::ReflowPrintObject| to reuse the presShells and stuff as I thought that all the network resources are stored in the presShell --- but as I understand things now, these kind of things are stored on the mDocShell, which stays the same during reflows. Therefore I haven't included my changes to |nsPrintEngine::ReflowPrintObject| in this WIP
* the |nsPrintEngine| implements the |nsIWebProgressListener| interface, such that it can listen for progress made while loading resources like fonts/images
* the |nsPrintEngine| implements the |nsSupportsWeakReference| interface and in |nsDocumentViewer.cpp| the type of pointer holding on the |mPrintEngine| got changed from |nsCOMPtr<nsPrintEngine>| to |nsRefPtr<nsPrintEngine>| --- there were problems when implementing the |nsIWebProgressListener| on the |nsPrintEngine| and by doing this, it got fixed (pure magic to me - see IRC log around here: krijnhoetmer.nl/irc-logs/developers/20120518#l-3902)
* |nsPrintEngine::OnStateChange| increments a counter once a request starts and decrement it ones a request finish. As pointed out in the bug comments before, this might not be best practise, but looking at the network logs from nsDocLoad and gfxUserGroupSet, the counter hits zero after all fonts are loaded, even if there are fallback fonts
* in |nsPrintEngine::DoCommonPrint|, instead of calling the functions |InstallPrintPreviewListener()| or |DocumentReadyForPrinting()| to continue the print/preview process, I do some basic setup by calling |CheckForChildFrameSets(...)| and |EnablePOsForPrinting(...)|, add the WebProgressListener and then perform a reflow using |ReflowDocList(mPrt->mPrintObject, false);| This performs a reflow on the print document and starts the network requests. Once all requests finish, I call the |InstallPrintPreviewListener()| and |DocumentReadyForPrinting()| from |nsPrintEngine::OnStateChange| again to continue the printing process.

Problem:
* Firefox crashes when opening print preview OR printing to an output device. I see where it crashes when debugging GDB but I don't understand how to fix it.
Attachment #624183 - Attachment is obsolete: true
Attached file Test page I used to test the code (obsolete) — 
Bundled as a zip file as it contains multiple files.
> I see where it crashes

Where does it crash?

With the patch, are you doing multiple reflows on a print presentation by any chance?
I'd start with those first "Recreating prescontext" asserts.  The actual crashes look like a presshell that's already had Destroy() called being notified by the refresh driver for some reason...
Based on :bz's feedback, I destroy the presShell --- and it "works" :)

= What works
* Fonts load in preview/printout

= What doesn't work
* in preview, the preview container doesn't take up the entire browser window
* in printout, not all fonts show up --- the last font that has a fallback font don't show up. Guess that's because we continue printing right after the network request finish, but finishing the fonts takes some extra time. :smaug proposed to use a runnable to add some extra time between "finished network" and "continue printing".
* the background-image defined in the "@media print" don't show up in preview/printout. But this bug is first about fonts, so one step at at time?
I've implemented nsRunnable to add some delay between "Networking is done" and "perform a second reflow". Some fonts still don't show up in the actual printout.

Looking at the logs, I see that the fonts get added again during the second reflow. I guess that's because a new presShell is created and the reflow is performed on. That means, although we now the fonts are loaded after the first reflow, we don't know they are loaded within the second reflow :/

Is the solution to this to reuse the presShell during the second reflow and make sure the Destory() operation is not called on it, that seems to cause the crash as described on comment #72?
Odd. I would have thought the first reflow would get all the fonts...

If you can reuse the presshell and just blow away the frame tree, that would nice, yes.
(In reply to Boris Zbarsky (:bz) from comment #75)
> Odd. I would have thought the first reflow would get all the fonts...

The fonts are all loaded (at least that's what I can say from the stuff printed to the console), but during the second reflow the presShell and presContext are re-created objects. 

Looking at the backtrace from the gfxUserFontSet constructor, it looks like the nsUserFontSet is stored on the nsPresContext. If that's true, it might be possible to only recreate a new instance of nsPresShell and call the IntialReflow() function on it. But as the nsPresContext stayed the same, maybe then the fonts don't get loaded a second time.

If that sounds reasonable, I take a look at it later.

> If you can reuse the presshell and just blow away the frame tree, that would
> nice, yes.

As said above, can we only reuse the presContext and still create a complete new copy of the PreShell to do a "clean" InitialReflow?
Ah, right (re the font set being on the prescontext).

You can't really recreate the presshell independently of the prescontext, and long-term we're trying to merge the two objects.

Why do you need to call InitialReflow() instead of just blowing away everything except the root frame and then creating a new frame tree and reflowing it?
(In reply to Boris Zbarsky (:bz) from comment #78)
> Why do you need to call InitialReflow() instead of just blowing away
> everything except the root frame and then creating a new frame tree and
> reflowing it?

My understanding of Gecko is very limited and I don't know what it needs to do a correct recalculation of the "looking"/layout of the page. If you say "blowing away everything except the root frame and then creating a new frame tree and reflowing it" I believe you blind :)

Can you give me some ideas/pointers how to do the "blow away and recreate it" thing? Looking at the nsPresShell.h file, I see a function like "Destory", but I guess it shouldn't kill the relation to the presShell. Should the Destory function take a new optional argument |bool aNotShutdown|, that makes the |Destory()| function terminate after the |mFrameConstructor->WillDestroyFrameTree();| here:

    http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#988

?

Then (maybe) calling another time |InitialReflow()| on the same presShell works out?

Or are there already functions on the nsPresShell that can be used to achieve this?
You need to call PresShell::ReconstructFrames. That's all.
> I don't know what it needs to do a correct recalculation

In general, it just needs to reflow them.  We do this all the time on the web, after all.  ;)

Printing is a special case, because some frames in printing don't deal with a second reflow, which is why you need to recreate the frames.

Comment 80 looks right to me.
This patch uses PresShell::ReconstructFrames as pointed out in comment #80 to do a relayout after all font data is loaded compared to before, where the entire pres/docShell was recreated.

In my test website I have some divs using downloadable-fonts and an iFrame, that also uses downloadable-fonts.

The good news: If I just call ReconstructFrames on the iFrame's presShell, then all fonts show up in the printout (YEAH!)

The bad news: If I call ReconstructFrames on the document's presShell first and then the iFrame's presShell, to reconstruct the frames for the document as well, there is nothing visible in the iFrame --- not in the Firefox Print Preview nor in the printout. Only doing the reflow on the document's presShell results in the same empty iframe. Changing the reconstruction order (first iFrame, then document) doesn't change anything either.

The code that does the reconstruction looks like this:

+    po->mPresShell->ReconstructFrames();
+    // Process the reflow event InitialReflow posted
+    po->mPresShell->FlushPendingNotifications(Flush_Layout);

where this is inside a loop that loops over all printObjects (po) on the page (in this example test page there are two printObjects: the first one is the document's printObject, the second one the iFrame's one).

Any idea why the iFrame don't show up or what I should look for to debug this?
Is the subframe constructing a useful frame tree?  Is it getting laid out correctly?  Is it hooking itself up to the right nsIView?
This is the console output of the print preview from doing the first initial reflow and how the frame tree looks after calling ReconstructFrames.

First the document's tree frame is reconstructed (nsPrintEngine::SetupToPrintContent - Reconstruct 0 - 0x108e714c0), then the tree frame of the iFrame (nsPrintEngine::SetupToPrintContent - Reconstruct 1 - 0x108e71840).

Looking at the tree frame dumps, it looks like nearly all elements have zero x/y/width/height after the reconstruction. While this explains why there is no content visible in the iFrame, this doesn't explain why the content of the document outside of the iFrame is displayed right.

Also, just calling ReconstructionFrames on the iFrame's presShell, that makes the printout look good, results in the same empty x/y/width/height frame tree as seen in this dump, but the iFrame's content is displayed correctly.

This is odd, right?
After calling ReconstructFrames() but before you reflow, all the sizes are expected to be 0.  Were you dumping the frame tree before or after reflow?
The former frame dump was taken before the reflow - actually, I didn't do any additional reflow after calling ReconstructFrames().

I've added a reflow after calling Reconstruction Frames now:

    nsSize adjSize = po->mPresContext->GetPageSize();
    po->mPresShell->ReconstructFrames();
    po->mPresShell->ResizeReflow(adjSize.width, adjSize.height);

Looking at the tree frame dump, the structure and position information stays very much the same and there is only variation at the height/width of the text. This is expected as the font is now loaded and therefore the frame for the text has to change.

The content in the iFrame still don't show up. 

As the generated frame tree looks reasonable, how can I check if "[...] it hooking itself up to the right nsIView?"
roc or tn might be able to answer that better...
Attached file View tree dump — 
Created using   

    mFrameConstructor->GetRootFrame()->GetView()->List((FILE*)stderr, 0);
In nsSubDocumentFrame::ShowViewer for non-dynamic prescontext's we just make sure the inner view is there for the subdocument frame. Whereas for dynamic prescontext's we call Show on the frameloader which (eventually) creates the root view for the subdocument and hooks it into the view tree. The comment says that the printing code takes care of this, but I guess it doesn't handle reconstructing the document's frames? smaug, do you know where the printing code might be going wrong here?
I've added some logs to the nsView::InsertChild function to see when the iFrame view is insert into the document's view tree. Doing the gdb at the "iFrame's view gets insert into document's view tree" action, it turned out, that the insertion is triggered by this line:

    http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#1969

Seems like the print code handles the relation itself. I try to refactor the function a little bit and then do the call to SetRootView after the frame reconstruction manually as well.
Attached patch WIP 3 (obsolete) — — Splinter Review 
Enclosed the current WIP patch.

This fixes the "iFrame not showing up problem". On mac, this patch makes printout show up the fonts (= also in iFrames!).

On linux however, there are some problems:
1) Changing the zoom size in Firefox Print Preview makes the content to become a white page.
2) Printing (at least to a PDF file) causes a signal 11, where this is the GDB bt:


#5  nsPrintEngine::DoPrint (this=0x7fdd8f7eafb0, aPO=0x7fdd8de26ec0) at /home/jviereck/dev/ff_font/layout/printing/nsPrintEngine.cpp:2398
2398	  NS_ASSERTION(poPresContext->Type() != nsPresContext::eContext_PrintPreview,
(gdb) list
2393	
2394	  nsIPresShell*   poPresShell   = aPO->mPresShell;
2395	  nsPresContext*  poPresContext = aPO->mPresContext;
2396	
2397	  NS_ASSERTION(poPresContext, "PrintObject has not been reflowed");
2398	  NS_ASSERTION(poPresContext->Type() != nsPresContext::eContext_PrintPreview,
2399	               "How did this context end up here?");
2400	
2401	  if (mPrt->mPrintProgressParams) {
2402	    SetDocAndURLIntoProgress(aPO, mPrt->mPrintProgressParams);

The assertion in line 2307 already fails. Haven't looked into it yet.
Attachment #625587 - Attachment is obsolete: true
Attachment #625698 - Attachment is obsolete: true
Attachment #627218 - Attachment is obsolete: true
Comment on attachment 627509 [details] [diff] [review]
WIP 3

@@ -1874,16 +2067,47 @@ nsPrintEngine::ReflowPrintObject(nsPrint
+  if (aPO->mPresShell->GetRootFrame()) {
+    // Reuse the root view that is already on the root frame.

I don't know if this is causing you any problems or not but there can still be a root view without a root frame. You can get it by aPO->mPresShell->GetViewManager()->GetRootView().
(In reply to Julian Viereck from comment #91)
> On linux however, there are some problems:
> 1) Changing the zoom size in Firefox Print Preview makes the content to
> become a white page.

The print preview code tries to get the number of pages in print preview. The code to return the number of pages in print preview (nsPrintEngine::GetPrintPreviewNumPage [1]) relays on the object |mPrtPreview| to be available. However, as the code is no "async", nsPrintEngine::FinishPrintPreview is called with some delay. Within that function, the assignment |mPrtPreview = mPrt| is performed. Until that assignment, |mPrtPreview| is null.

A simple work around is, to check if the |mPrtPreview| has a value yet and if not, then fallback to the |mPrt| object.

However, this might highlight a problem with the new code. With this fix, the number of pages is equal to the number of pages after the first reflow. But as the number of pages might change with a later reflow (when some new fonts arrive and they cause the number of pages to change), this information has to propagate to the PrintPreview UI.

I've just looked over the Print-Preview chrome code in [1] and I'm not sure if my idea to fix this is the right way to go:

1) make the nsPrintEngine emit an event "afterReflow"
2) make the code in printPreviewBindings.xml listen to the "afterReflow" event from the nsPrintEngine
3) the event callback handler for "afterReflow" in printPreviewBindings.xml calls |updateToolbar| to reflect the changes (like changing number of pages).

Does that makes sense? Should there be a callback like nsPrintEngine.onAfterReflow instead of an event?

---

[1]: http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#806
[2]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printPreviewBindings.xml#350
[3]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printPreviewBindings.xml
I think a pageCountChanged event would make sense.
Attached patch WIP 5 (obsolete) — — Splinter Review 
Based on WIP3, fixes breakage on linux. Whenever the docList gets reflowed an event is emitted that the Print Preview UI code binds into and updates the number of pages.

This patch doesn't have unit test yet. They will follow soon. As this is a requirement for PDF.JS printing support that is target for FF15 uplift, I want to get review/feedback going on this patch.

Note that this is my first Gecko patch - expect "strange" stuff.
Attachment #627509 - Attachment is obsolete: true
Attachment #627784 - Flags: review?(bugs)
Patch #1 in bug 650960 adds a "mozPrintStatus" event that can probably be used here! The event includes the total number of pages.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #96)
> Patch #1 in bug 650960 adds a "mozPrintStatus" event that can probably be
> used here! The event includes the total number of pages.

We might be able to reuse the event, but that would mean bug 650960 need to land before this one does. Given that it would be good for PDF.JS to have this bug landed in FF15, I don't want to hold back for 650960.

Can we merge the event introduced here with the "mozPrintStatus" event in a follow up bug?
You could just take the code you need from that bug.
The event for this bug doesn't need the new interface.
So, when bug 650960 lands, it could update dispatching code to use the new interface.
But the event name could be already in this bug mozPrintStatusChanged or some such generic.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #98)
> You could just take the code you need from that bug.

Looking over the patch, that means taking the first patch (1/4) and at least the nsPrintEngine::FirePrintStatusEvent function from (2/4). Taking the entire patch (2/4) won't apply simply with the patch here. Both patches have outstanding review requests.

(In reply to Olli Pettay [:smaug] from comment #99)
> The event for this bug doesn't need the new interface.
> So, when bug 650960 lands, it could update dispatching code to use the new
> interface.
> But the event name could be already in this bug mozPrintStatusChanged or
> some such generic.

For the mozPrintCallback API in [1], UX wants the print preview to close once the |abort()| is called on the print object. This could simple be done by sending another event from the printEngine to the print preview UI code.

We can use the "mozPrintStatusChanged" event for what is called "printPreviewUpdate" in the current WIP5 patch and add another one for the abort by the printCanvas.


[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=745025
Attached patch WIP 6 (obsolete) — — Splinter Review 
Mostly like WIP5 + some comments + small code changes - toolkit code.

It turned out the progessListener was catching network requests like request.name = "about:document-onload-blocker". This load broke the tests, as the test expected the document to be ready right away, but as this request happend, this wasn't 100% the case.

The way I worked out this is by adding these lines in the OnStateChange function:

  +  nsCAutoString name;
  +  aRequest->GetName(name);
  +  if (name.Equals("about:document-onload-blocker")) {
  +    return NS_OK;
  +  }

I'm pretty sure there got to be a better way of doing this. Any hints?
Attachment #627784 - Attachment is obsolete: true
Attachment #627784 - Flags: review?(bugs)
Attachment #629287 - Flags: review?(bugs)
> I'm pretty sure there got to be a better way of doing this

What exactly are you trying to do?  The request you see there is what we use to prevent onload firing when it needs to wait for something to happen....
(In reply to Boris Zbarsky (:bz) from comment #103)
> > I'm pretty sure there got to be a better way of doing this
> 
> What exactly are you trying to do? 

Assume there is a HTML document like this:

  <html><body> Hello World </body></html>

Without the work around, network requests are requested after reflowing the document the first time. Therefore, another reflow happens later. However, this is not necessary in this case, as there is nothing that makes the second reflow necessary.

> The request you see there is what we use to prevent onload firing when it needs to wait for something to happen....

Any idea why the event is triggered here? The document is cloned and therefore there is nothing to load anymore in this particular case.
There are various things that require blocking onload in the spec.  Basic things like posting an image load event, etc.

If you really don't care about it, the right way to do things is probably to call BlockOnload() on the document yourself before you start looking for network requests, since that will add the load blocker request only once and not confuse your request tracking.  And add a nice comment.

Another option would be to add a way to tell documents to ignore BlockOnload() calls.
(In reply to Boris Zbarsky (:bz) from comment #105)
> There are various things that require blocking onload in the spec.  Basic
> things like posting an image load event, etc.
> 
> If you really don't care about it, the right way to do things is probably to
> call BlockOnload() on the document yourself before you start looking for
> network requests, since that will add the load blocker request only once and
> not confuse your request tracking.  And add a nice comment.
> 
> Another option would be to add a way to tell documents to ignore
> BlockOnload() calls.

Assuming we really don't need the tracking request, is the code I mentioned:

  +  nsCAutoString name;
  +  aRequest->GetName(name);
  +  if (name.Equals("about:document-onload-blocker")) {
  +    return NS_OK;
  +  }

also an acceptable solution or should it be done in one of the two ways you mentioned?
Given that the string is arbtrary and subject to change, I'd prefer you used one of the other two approaches.
Attached patch WIP 7 (obsolete) — — Splinter Review 
Add mDocument->BlockOnload() to prevent unnecessary reflows.
Attachment #629287 - Attachment is obsolete: true
Attachment #629287 - Flags: review?(bugs)
Attachment #629639 - Flags: review?(bugs)
Attached file Updated test files — 

        Attachment #625588 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 629639 [details] [diff] [review]
WIP 7

>@@ -7535,18 +7537,29 @@ nsDocument::CloneDocHelper(nsDocument* c
> 
>   // Set URI/principal
>   clone->nsDocument::SetDocumentURI(nsIDocument::GetDocumentURI());
>   // Must set the principal first, since SetBaseURI checks it.
>   clone->SetPrincipal(NodePrincipal());
>   clone->mDocumentBaseURI = mDocumentBaseURI;
> 
>   if (mCreatingStaticClone) {
>+    nsCOMPtr<nsILoadGroup> loadGroup;
>+    
>+    // |mDocumentContainer| is the container of the document that gets cloned
>+    // and not the original container. See CreateStaticClone function().
odd comment. mDocumentContainer is the container of the clone here, not the container
of the document which is being cloned.


>+nsPrintEngine::ReconstructAndReflow(bool doSetPixelScale)
>+{
>+#if (defined(XP_WIN) || defined(XP_OS2)) && defined(EXTENDED_DEBUG_PRINTING)
>+  // We need to clear all the output files here
>+  // because they will be re-created with second reflow of the docs
>+  if (kPrintingLogMod && kPrintingLogMod->level == DUMP_LAYOUT_LEVEL) {
>+    RemoveFilesInDir(".\\");
>+    gDumpFileNameCnt   = 0;
>+    gDumpLOFileNameCnt = 0;
>+  }
>+#endif
>+
>+  for (PRUint32 i=0;i<mPrt->mPrintDocList.Length();i++) {
This code moved from elsewhere, but coding style could be still fixed.
space before and after =, and < and after ;

>+NS_IMETHODIMP
>+nsPrintEngine::OnProgressChange(nsIWebProgress *aWebProgress,
>+                                  nsIRequest *aRequest,
>+                                  PRInt32 aCurSelfProgress,
>+                                  PRInt32 aMaxSelfProgress,
>+                                  PRInt32 aCurTotalProgress,
>+                                  PRInt32 aMaxTotalProgress)
Some extra spaces

>+nsPrintEngine::OnLocationChange(nsIWebProgress *aWebProgress,
>+                                  nsIRequest *aRequest, nsIURI *aLocation,
>+                                  PRUint32 aFlags)
ditto

>+NS_IMETHODIMP
>+nsPrintEngine::OnStatusChange(nsIWebProgress *aWebProgress,
>+                                nsIRequest *aRequest, nsresult aStatus,
>+                                const PRUnichar *aMessage)
ditto


>+nsPrintEngine::OnSecurityChange(nsIWebProgress *aWebProgress,
>+                                  nsIRequest *aRequest,
>+                                  PRUint32 aState)
ditto

>+nsPrintEngine::UpdateSelectionAndShrinkPrintObject(nsPrintObject* aPO, bool documentIsTopLevel)
aDocumentIsTopLevel
>+nsPrintEngine::SetRootView(
>+    nsPrintObject* aPO, 
>+    bool& doReturn, 
>+    bool& documentIsTopLevel, 
>+    nsSize& adjSize
>+)
aParameterName


>+nsPrintEngine::ReflowPrintObject(nsPrintObject * aPO)
>+{
>+  NS_ASSERTION(aPO, "Pointer is null!");
>+  if (!aPO) return NS_ERROR_FAILURE;
>+
>+  if (!aPO->IsPrintable())
>+    return NS_OK;
if (expr) {
  stmt;
}


This looks good. I'll test the patch and update it.

        Attachment #629639 -
        Flags: review?(bugs) → review-

    
    

    
  
To Mats for the final touches on this bug. Please address the review comments and land these patches.
Assignee: nobody → matspal

    
    

    
  
Actually, Olli seems to be already working on that per comment 110.
Assignee: matspal → bugs

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP8 (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
I am now including this patch in my custom nightly Windows, Linux and Android builds available at http://www.wg9s.com/mozilla/firefox/ --- Windows builds will be their soon.  I am re-spinning them because of an unrelated issue.

    
    

    
  
And windows re-spins are now complete! :-)

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #115)
> And windows re-spins are now complete! :-)

I'm curious to know whether the patch could fix bug 697053.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #113)
> Created attachment 637293 [details] [diff] [review]
> WIP8

I realize this is WIP, but thought I would give results of my testing anyway.

Looks great under Linux.
Windows not so good.  Print preview looks fine, but the actual printout prints garbage.  This is worse than without the patch where it at least printed the correct text with the incorrect font.

    
    

    
  
(In reply to Frédéric Wang (:fredw) from comment #116)
> (In reply to Bill Gianopoulos [:WG9s] from comment #115)
> > And windows re-spins are now complete! :-)
> 
> I'm curious to know whether the patch could fix bug 697053.

It does fix bug 697053 except for the issue in comment 117.

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #118)
> (In reply to Frédéric Wang (:fredw) from comment #116)
> > (In reply to Bill Gianopoulos [:WG9s] from comment #115)
> > > And windows re-spins are now complete! :-)
> > 
> > I'm curious to know whether the patch could fix bug 697053.
> 
> It does fix bug 697053 except for the issue in comment 117.

Thanks, that's good to know.
Blocks: downloaded-stretchy
Blocks: 697053
No longer blocks: downloaded-stretchy

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #117)
> Windows not so good.  Print preview looks fine, but the actual printout
> prints garbage.
Hmm, not good. I don't have a Windows dev environment.

    
    

    
  
I need to go through the patch still few times (I just updated the review comments).

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #120)
> (In reply to Bill Gianopoulos [:WG9s] from comment #117)
> > Windows not so good.  Print preview looks fine, but the actual printout
> > prints garbage.
> Hmm, not good. I don't have a Windows dev environment.

Well actually this is going to be tricky.  It seems to be printer driver dependent or else I have just lost my mind.  I went from my home, where I have an HP Deskjet 3740, to my summer place, where I have an HP Deskjet 3050 J610, and can not reproduce the issue here.

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #122)
> (In reply to Olli Pettay [:smaug] from comment #120)
> > (In reply to Bill Gianopoulos [:WG9s] from comment #117)
> > > Windows not so good.  Print preview looks fine, but the actual printout
> > > prints garbage.
> > Hmm, not good. I don't have a Windows dev environment.
> 
> Well actually this is going to be tricky.  It seems to be printer driver
> dependent or else I have just lost my mind.  I went from my home, where I
> have an HP Deskjet 3740, to my summer place, where I have an HP Deskjet 3050
> J610, and can not reproduce the issue here.

I would ignore this issue until either I or someone else reproduces it.

    
    

    
  
OK.  I CAN reproduce the issue.  It is not related to the printer model.  I realized that I was using a different Laptop when this happened at home.

This seems to fail when printing From 64-bit Windows 7 using the 32-bit version of Firefox.

        Attachment #638068 -
        Attachment description: Sample prinout → Sample printout showing the issue

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #124)
> This seems to fail when printing From 64-bit Windows 7 using the 32-bit
> version of Firefox.

On a trunk build, if you download the fonts locally and create a similar test page that doesn't use @font-face but just uses the fonts directly, do you see the same bug? What if you use @font-face with src:local()?

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #126)
> (In reply to Bill Gianopoulos [:WG9s] from comment #124)
> > This seems to fail when printing From 64-bit Windows 7 using the 32-bit
> > version of Firefox.
> 
> On a trunk build, if you download the fonts locally and create a similar
> test page that doesn't use @font-face but just uses the fonts directly, do
> you see the same bug? What if you use @font-face with src:local()?

With local font it prints fine.  I have not been able to figure out how to get src:local to work, however I originally saw this issue with the testcase attached to bug 697053, which uses src:local, so I would say @font-face with source local still shows the issue.

    
    

    
  
OK, so it's not the font then, it must really be a problem with this bug.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #128)
> OK, so it's not the font then, it must really be a problem with this bug.

Well, there is also the kind of workaround fix in Bug 458160 to make @font-face work with opentype fonts at all under windows in the first place.  Might be the 2 fixes don;t work well together.

@ things that might help here:

1.  A sample page with @font-face with different formats so I could determine if it is only opentype fonts that have the issue.

2.  A 64-bit windows tryserver build so I could determine if this issue is only present in running 32-bit windows builds on 64-bit windows OS.

    
    

    
  
I tried webdemofont.html on Win7 x64 and this seems to be working fine for me.  I also tried it out with pdf.js and it seems to work fine. Further I tried out OSX and it seems fine.

Looking at Bill's picture above that looks a lot like what we see with pdf.js when hardware acceleration is causing issues with the font, but I'd expect if that were the case both the web page and printout would look the same.

    
    

    
  
(In reply to Brendan Dahl from comment #130)
> I tried webdemofont.html on Win7 x64 and this seems to be working fine for
          ^^^^^^^^^^^^^^^^

And exactly what is the URL to this demo?

    
    

    
  
(In reply to Brendan Dahl from comment #132)
> The one you linked to http://opentype.info/demo/webfontdemo.html

Oh so this prints correctly for you under a 64-bit windows system using a 32-bit Firefox build?  How strange.  Wonder what is different.

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #133)
> (In reply to Brendan Dahl from comment #132)
> > The one you linked to http://opentype.info/demo/webfontdemo.html
> 
> Oh so this prints correctly for you under a 64-bit windows system using a
> 32-bit Firefox build?  How strange.  Wonder what is different.

Oh I meant using a 32-bit build that includes the patch attached to this bug.

    
    

    
  
Olli: Do you want someone else to test on windows or is there something else holding this patch back?

    
    

    
  
Some windows testing might be good.

    
    

    
  
Try builds would be nice so I can test using a 64-bit build which I don't currently have the ability to build myself.  It would also help to verify if the issue can be duplicated with something other than my personal builds.

    
    

    
  
Setting gfx.direct2d.disabled;true in about:config avoids this issue.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP8 patch updated to curent tip (obsolete)
        — 
          — Splinter Review
      

    


  
Just fixing bit-rot in smaug's patch.

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #138)
> Setting gfx.direct2d.disabled;true in about:config avoids this issue.

I should have been more clear.  I meant disabling direct2d resolves the issue i brought up in comment 117 about builds with the WIP8 patch printing garbage under Windows, and not the original issue this bug is about.

This also explains why I was seeing this issue only on my 64-bit computers.  My 32-bit systems are running Windows/XP where we disable direct2d.

    
    

    
  
> I should have been more clear.  I meant disabling direct2d resolves the issue i brought up in comment 117 about builds with the WIP8 patch printing garbage under Windows, and not the original issue this bug is about.

Thanks for that input! 

> I meant disabling direct2d resolves [...] not the original issue this bug is about.

Is this really the case? Are the web-fonts not used in the printout when this patch is applied?

    
    

    
  
(In reply to Julian Viereck from comment #141)
> Is this really the case? Are the web-fonts not used in the printout when
> this patch is applied?

No, that is not what I meant.  Trying to clarify once again. In order for the page to print correctly under windows, using the web-fonts, it is necessary to apply the WIP8 patch and make sure that direct2 is disabled.

    
    

    
  
If direct2d is enabled by default (on Win7), then we should not land the patch before the direct2d 
issue is fixed. I don't have a Windows dev environment, nor do I know anything about font rendering, so
someone else needs to write the fix.

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #142)
> 
> No, that is not what I meant.  Trying to clarify once again. In order for
> the page to print correctly under windows, using the web-fonts, it is
> necessary to apply the WIP8 patch and make sure that direct2 is disabled.

Bill, thanks a lot for the clarify and your time you're spending on this one :)

Applying the WIP8 and turn direct2d ON, is the broken printout reproduce able using a PDF printer, that generates a PDF based on the printout?

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=c72eab40b7c8

(Didn't compile locally, so I just hope the latest patch works)

    
    

    
  
(In reply to Julian Viereck from comment #144)
> (In reply to Bill Gianopoulos [:WG9s] from comment #142)
> > 
> > No, that is not what I meant.  Trying to clarify once again. In order for
> > the page to print correctly under windows, using the web-fonts, it is
> > necessary to apply the WIP8 patch and make sure that direct2 is disabled.
> 
> Bill, thanks a lot for the clarify and your time you're spending on this one
> :)
> 
> Applying the WIP8 and turn direct2d ON, is the broken printout reproduce
> able using a PDF printer, that generates a PDF based on the printout?

It is reproducible using the XPS document Writer that comes with Windows 7.  I did not try PDF though, but I think the fact that it is reproducible creating an XPS and suing the XPS viewer kind of rules out it being specific to any printer driver.

    
    

    
  
Pinged :nattokirai on irc, see here: http://irclog.gr/#show/irc.mozilla.org/developers/292216

Summing up:
* we use different code paths to load fonts depending on if direct2d is enabled on windows or not
* he wants to look into this issue tomorrow

    
    

    
  
The odd thing is that everything works fine on print preview.  It is just the actual printing that is a complete fail.

    
    

    
  
Bas, can you take a look at the DirectWrite problem?

    
    

    
  
Just so everyone understands what the currently posted WIP patch does and what the issue is I will summarize:

1.. the patch corrects the issue where if there are multiple web-fonts displayed on a page many of the pages show in print preview and print without the web-font and use a default font.

2.. Under Windows there is an issue where ir direct2d is enabled, then this still fixes the issue for print preview, but the actual printed output does not display the correct glyphs so really looks like complete garbage.

Is there a way to bypass using direct2d for printing???

    
    

    
  
Olli, could you refresh the WIP patch?  It looks like dbaron has been doing some renames in this code and the patch doesn't apply cleanly.

http://hg.mozilla.org/mozilla-central/rev/3684a179853c

I'm guessing this is just InitialReflow ==> Initialize but just in case it's more than that...

    
    

    
  
(In reply to Bill Gianopoulos [:WG9s] from comment #150)

> Is there a way to bypass using direct2d for printing???

In a word, no.  This isn't Direct2D as much as it's *DirectWrite* and we need to figure out what the issue is there.

    
    

    
  
(In reply to Julian Viereck from comment #147)
> Pinged :nattokirai on irc, see here:
> http://irclog.gr/#show/irc.mozilla.org/developers/292216

To refute what is said in that IRC conversation, the issue is not that the printout looks ****.

This is NOT the same issue as Direct2d looks worse if you have cleartype enabled type issue.

The problem here is that if direct2d is enabled it prints entirely the wrong character form entirely the wrong language so if the test is English, you get Greek characters displayed.

This is just printing complete garbage. it is NOT just a the printout looks **** issue.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated to tip again (obsolete)
        — 
          — Splinter Review
      

    


  
This was pretty easy to do using Mercurial mq and rebase. I did "hg update -C 500dac3a151b" to go back to the revision where the patch applied cleanly, "hg qimport -P -n bug468568 ~/Download/patch" to import the patch, then "hg rebase -s qbase -d 106955" to rebase the patch to the head of my repo. There was only one merge conflict not handle automatically, and that was just a conflict over some include files where the solution was to just take the union of the lines from both versions.

        Attachment #657832 -
        Attachment is obsolete: true

    
    

    
  
It appears to me that when we print http://opentype.info/demo/webfontdemo.html with D2D on, the blocks of text that are garbled are actually not using the downloaded font. They're using some plain sans-serif font. That would explain why the text is garbled: the glyph IDs for the correct font won't produce the right glyphs when the wrong font is used.

    
    

    
  
When I try printing with this patch, there are some assertion failures, namely:

###!!! ASSERTION: Unexpected containers: 'SameCOMIdentity(debugDocContainer, debugDocShell)', file z:/roc/mozilla-central/obj-ff-debug/layout/base/../../../layout/base/nsDocumentViewer.cpp, line 2151

###!!! ASSERTION: Should only call DidPaint on root presshells: 'mPresContext->IsRoot()', file z:/roc/mozilla-central/obj-ff-debug/layout/base/../../../layout/base/nsPresShell.cpp, line 7042

###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file z:\roc\mozilla-central\obj-ff-debug\dist\include\nsCOMPtr.h, line 503

These need to be fixed.

The last one is apparently because nsPagePrintTimer inherits from nsITimerCallback and nsRunnable (hence nsIRunnable) but uses
NS_IMPL_ISUPPORTS1(nsPagePrintTimer, nsITimerCallback)
Probably it should inherit first from nsRunnable and then nsITimerCallback, and use NS_IMPL_ISUPPORTS_INHERITED1(nsPagePrintTimer, nsRunnable, nsITimerCallback).

    
    

    
  

      
      
      
      

        Attached file
          
        small testcase
        — 
      

    


  
When I print the opentype.info test page I consistently get some fonts rendering OK and some fonts not. For example quote 1 (GraublauWeb) always renders OK and quote 2 (Fertigo) doesn't. So I thought it might be something to do with the fonts.

However, here's a very simple test page where GraublauWeb and Fertigo both render incorrectly. So it's probably not the fonts.

    
    

    
  
We call _cairo_dwrite_scaled_font_create_win32_scaled_font to create a Win32 font for each DWrite font we use when printing. That seems to work OK for all the fonts.

I think the problem is where _cairo_win32_printing_surface_emit_win32_glyphs maps glyphs back to Unicode characters. On the second line, after mapping, the unicode_glyphs array looks like this:

+		[0]	{index=39 x=349.99999809265137 y=731.24998354911804 }
+		[1]	{index=198 x=414.58332896232605 y=731.24998354911804 }
+		[2]	{index=181 x=459.68749390840532 y=731.24998354911804 }
+		[3]	{index=181 x=483.02082635164265 y=731.24998354911804 }
+		[4]	{index=8719 x=506.35415879487994 y=731.24998354911804 }
+		[5]	{index=39 x=555.72915691137314 y=731.24998354911804 }
+		[6]	{index=198 x=620.31248778104782 y=731.24998354911804 }
+		[7]	{index=181 x=665.41665272712703 y=731.24998354911804 }
+		[8]	{index=181 x=688.74998517036443 y=731.24998354911804 }
+		[9]	{index=8719 x=712.08331761360171 y=731.24998354911804 }
+		[10]	{index=969 x=761.45831573009491 y=731.24998354911804 }

Since the original string was "HelloHello2", those glyph IDs do not correspond to the Unicode characters as they're supposed to.

    
    

    
  
I've tracked this into _cairo_truetype_index_to_ucs4 producing 39 for the first glyph of the second line when it definitely shouldn't. This is GraublauWeb.otf; the cmap table offset is 4544, and we're mapping glyph 10 to character 39.

I need to go home now ... someone with more cairo font knowledge than me could probably figure this out pretty quickly. I guess either the incoming glyph is wrong, or the glyph-to-character reverse map logic through the cmap is wrong in some subtle way, or Windows is giving us back corrupt CMAP tables for the D2D-to-win32 converted font.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #156) 
> ###!!! ASSERTION: Unexpected containers: 'SameCOMIdentity(debugDocContainer,
> debugDocShell)', file
> z:/roc/mozilla-central/obj-ff-debug/layout/base/../../../layout/base/
> nsDocumentViewer.cpp, line 2151
This assertion should be just removed. It is old and invalid nowadays.

    
    

    
  
I tried a simple cairo test case that creates a PDF containing the string "HelloHello2" using the GraublauWeb.otf font. Inserting a printf in _cairo_truetype_index_to_ucs4 shows the index and usc4 values are correct.

index=  10  ucs4=U+0048  ps_name="GraublauWeb-Regular"  font_name="Graublau Web"
index= 144  ucs4=U+0065  ps_name="GraublauWeb-Regular"  font_name="Graublau Web"
index= 151  ucs4=U+006c  ps_name="GraublauWeb-Regular"  font_name="Graublau Web"
index= 154  ucs4=U+006f  ps_name="GraublauWeb-Regular"  font_name="Graublau Web"
index= 550  ucs4=U+0032  ps_name="GraublauWeb-Regular"  font_name="Graublau Web"

Windows may be returning a different font. You could try putting a call to _cairo_truetype_read_font_name in _cairo_truetype_index_to_ucs4 to see what font it is as I did with this debug patch:

diff --git a/src/cairo-truetype-subset.c b/src/cairo-truetype-subset.c
index 18ee685..a344503 100644
--- a/src/cairo-truetype-subset.c
+++ b/src/cairo-truetype-subset.c
@@ -1390,6 +1390,16 @@ _cairo_truetype_index_to_ucs4 (cairo_scaled_font_t *scaled_font,
 cleanup:
     free (cmap);
 
+    {
+        char *ps_name;
+        char *font_name;
+
+        _cairo_truetype_read_font_name (scaled_font, &ps_name, &font_name);
+        printf("index=%4ld  ucs4=U+%04x  ps_name=\"%s\"  font_name=\"%s\"\n", index, *ucs4, ps_name, font_name);
+        free (ps_name);
+        free (font_name);
+    }
+
     return status;
 }

    
    

    
  
(In reply to John Daggett (:jtd) from comment #151)
> Olli, could you refresh the WIP patch?
Is a new patch still needed?

    
    

    
  
Ah, roc updated it.

    
    

    
  
Thanks Adrian!

That patch reveals that the glyph-to-Unicode mapping is being done with the Arial font! Looking into it :-).

    
    

    
  
Yeah, so I've verified that cairo_win32_font_face_create_for_logfontw just gives us back "Arial" in this case. It's probably because the user font data we give DirectWrite isn't available to GDI to resolve our synthetic font name.

    
    

    
  


  
This detects the case where GDI gives us back the wrong font. We fall back to drawing glyph outlines as paths, which isn't so bad visually. Copy-paste of XPS/PDF text using downloaded fonts won't work though.

        Attachment #661669 -
        Flags: review?(jfkthame)

    
    

    
  


  
This patch makes DWrite fonts known to GDI as well (via AddFontMemResourceEx) when loaded, so that we can actually print them using the GDI glyph printing path.

I'm not sure we want to just go ahead and do this though --- it'll add overhead to all non-printed pages that use downloaded fonts.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 0.5 v2 (obsolete)
        — 
          — Splinter Review
      

    


  
If we take the previous patch, or something like it, it turns out the fonts we get don't have the kind of Truetype name this patch is looking for. So, I'm relaxing the checks in this patch a bit. If the font doesn't have a name, we treat that as a match instead of a non-match.

        Attachment #661669 -
        Attachment is obsolete: true

        Attachment #661669 -
        Flags: review?(jfkthame)

        Attachment #661682 -
        Flags: review?(jfkthame)

    
    

    
  
BTW when I apply both of the previous patches, I see some more bugs trying to copy text using downloaded fonts from the opentype.info page:
-- Some blocks of text have fallen back to use paths instead of glyphs.
-- In the blocks of text that use glyphs, if I copy some text and paste it into a text editor, I get two copies of the text.
These bugs would have to be fixed before the patch in comment #167 is useful.

    
    

    
  


  
This was helpful for debugging the "Should only call DidPaint on root presshells" assert.

        Attachment #661688 -
        Flags: review?(matt.woodrow)

    
    

    
  


  
This prints opentype.info on Windows 7 without asserting, for me. The fonts look good but as previously noted, text copy/paste doesn't work since we fall back to drawing glyph paths directly.

The only significant change in this patch compared to the last WIP is that I fixed the "Should only call DidPaint on root presshells" assertion by having nsPrintEngine::ReflowPrintObject create an nsRootPresContext for the root of the nsPrintObject tree.

        Attachment #629639 -
        Attachment is obsolete: true

        Attachment #637293 -
        Attachment is obsolete: true

        Attachment #661072 -
        Attachment is obsolete: true

        Attachment #661691 -
        Flags: review?(bugs)

        Attachment #661688 -
        Flags: review?(matt.woodrow) → review+

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #159)
> I've tracked this into _cairo_truetype_index_to_ucs4 producing 39 for the
> first glyph of the second line when it definitely shouldn't. This is
> GraublauWeb.otf; the cmap table offset is 4544, and we're mapping glyph 10
> to character 39.
> 
> I need to go home now ... someone with more cairo font knowledge than me
> could probably figure this out pretty quickly. I guess either the incoming
> glyph is wrong, or the glyph-to-character reverse map logic through the cmap
> is wrong in some subtle way, or Windows is giving us back corrupt CMAP
> tables for the D2D-to-win32 converted font.

I'm jumping in without having studied all the background here, so ignore me if this is irrelevant... but, why are we trying to map glyphs to Unicode characters at all? Surely that's inherently problematic, as many glyphs in a truetype/opentype font may not have a corresponding Unicode value we can use to identify them.

        Attachment #661689 -
        Flags: review?(bugs) → review+

        Attachment #661691 -
        Attachment is patch: true

    
    

    
  
Comment on attachment 661690 [details] [diff] [review]
Part 0.8: Remove invalid assertion about matching containers

Thanks!

        Attachment #661690 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #174)
> I'm jumping in without having studied all the background here, so ignore me
> if this is irrelevant... but, why are we trying to map glyphs to Unicode
> characters at all? Surely that's inherently problematic, as many glyphs in a
> truetype/opentype font may not have a corresponding Unicode value we can use
> to identify them.

That is part of the fix for bug 454532. Glyphs that have no unicode mapping are output as glyphs.

    
    

    
  
Comment on attachment 661691 [details] [diff] [review]
Updated main patch

> 
>   // create the PresContext
>-  aPO->mPresContext = new nsPresContext(aPO->mDocument,
>-    mIsCreatingPrintPreview ? nsPresContext::eContext_PrintPreview:
>-                              nsPresContext::eContext_Print);
>+  nsPresContext::nsPresContextType type =
>+      mIsCreatingPrintPreview ? nsPresContext::eContext_PrintPreview:
>+                                nsPresContext::eContext_Print;
>+  aPO->mPresContext = aPO->mParent ?
>+      new nsPresContext(aPO->mDocument, type) :
>+      new nsRootPresContext(aPO->mDocument, type);
>   NS_ENSURE_TRUE(aPO->mPresContext, NS_ERROR_OUT_OF_MEMORY);

In printpreview case if there isn't parent, shouldn't you check 
DocumentViewerImpl::FindContainerView() so that we don't end up creating
rootprescontext in case pp is shown as a tab (like it usually is).


That fixed, r=me

        Attachment #661691 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #168)
> Created attachment 661682 [details] [diff] [review]
> Part 0.5 v2
> 
> If we take the previous patch, or something like it, it turns out the fonts
> we get don't have the kind of Truetype name this patch is looking for. So,
> I'm relaxing the checks in this patch a bit. If the font doesn't have a
> name, we treat that as a match instead of a non-match.

I assume that is because gfxFontUtils:RenameFont is changing the name in the font name table to ensure the name is unique. It maybe possible to store the original name in the font name table using one of the platform IDs that are reserved for user-defined platforms (240-255). Then cairo could be modified to check this platform ID first when extracting the font name. This would also ensure the original font names are used in the generated pdf.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #173)
> This prints opentype.info on Windows 7 without asserting, for me. The fonts
> look good but as previously noted, text copy/paste doesn't work since we
> fall back to drawing glyph paths directly.

You can make text copy/paste work with the glyph paths by using cairo user fonts to draw the glyphs. Using user fonts will also result in smaller pdfs that render much faster since the pdf file will have a Type 3 font containing the glyph paths allowing the renderer to cache the glyphs instead of having to draw the path every time a glyph is displayed.

    
    

    
  
That's a good point, but it's beyond the scope of this bug.

    
    

    
  
With the current patches, the page in the URL (http://opentype.info/demo/webfontdemo.html) prints as well under windows with direct2d enabled as the orginal WIP8 patch did under LINUX.

    
    

    
  
For the benefit of those who think that correct printing is more important than being able to select and cut/copy text, I am including the current patches here in might nightly builds which are available at http://www.wg9s.com/mozilla/firefox/

    
    

    
  
To re-ask what was probably a stupid question in a different way.  Does it make sense to develop a patch that we can take for 18.0 if we don't have a better fix and even possibly take for 17.0 on Aurora to find a way to keep the current behavior on direct2d (where it prints using the fallback font)  but print using the correct font in cases where we have had success,  so that we can fix this where we can without causing regressions?

    
    

    
  
(In reply to Adrian Johnson from comment #176)

> That is part of the fix for bug 454532. Glyphs that have no unicode mapping
> are output as glyphs.

Hmm, this doesn't sound right.  The only changes that were approved for that bug were the name swizzling of Courier, the other patch which I think you're referring to was not reviewed and should not have been landed.

    
    

    
  
(In reply to John Daggett (:jtd) from comment #184)
> Hmm, this doesn't sound right.  The only changes that were approved for that
> bug were the name swizzling of Courier, the other patch which I think you're
> referring to was not reviewed and should not have been landed.

Let's not try to change that as part of this bug.

    
    

    
  
Comment on attachment 661682 [details] [diff] [review]
Part 0.5 v2

Review of attachment 661682 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp
@@ +1556,5 @@
> +        cairo_int_status_t status;
> +
> +        status = _cairo_truetype_read_font_name (font, &new_ps_name, &new_font_name);
> +        if (!status) {
> +            names_match = !new_font_name ||

Treating "no new_font_name" as a match seems risky, as it means we're effectively giving up on checking that we've got the right font in the case of downloaded fonts.

I think the reason _cairo_truetype_read_font_name isn't returning a name for these fonts is that gfxFontUtils::RenameFont puts Windows-platform Unicode strings (only) into the new 'name' table it constructs:

  http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontUtils.cpp#1385

but the cairo function used here only looks for legacy MacOS Roman names:

  http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-truetype-subset.c#1362

Rather than "flying blind" by ignoring the font name in this case, I think we should fix this, and then consider a font with no checkable name as unsafe to use.

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #186)
> Rather than "flying blind" by ignoring the font name in this case, I think
> we should fix this, and then consider a font with no checkable name as
> unsafe to use.

OK. Should we make RenameFont put a MacOS Roman name in the table, or should we make _cairo_truetype_read_font_name use Windows-platform Unicode names?

    
    

    
  
(I kinda think the former, at least if the new name is ASCII.)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #188)
> (I kinda think the former, at least if the new name is ASCII.)

My inclination is the latter, actually. It's perfectly possible to have a valid font (whether downloaded or locally installed) that doesn't include MacOS Roman name strings, so the cairo function really ought to be prepared to handle this rather than just punt.

Given that _cairo_truetype_read_font_name returns the names as 8-bit C strings (of unspecified encoding? ugh), it would probably be reasonable to make it explicitly ASCII-only, returning an error code if a non-ASCII codepoint is encountered in either the MacRoman name or a Windows Unicode name (platform 3, encoding 1, language 1033).

Note that we shouldn't make it read Windows/Unicode strings *instead of* MacRoman ones, as there are also fonts in common use (on OS X) that *only* have MacRoman name strings. So _cairo_truetype_read_font_name really needs to be prepared to use either platform/encoding.

In principle, it should do more work to handle various encodings, localized names, etc. (If the font includes names in multiple languages, which one do we want? I notice that _cairo_truetype_read_font_name doesn't actually check the language at all.) But given that in practice almost all real fonts do have ASCII names, and the names we generate for downloadable fonts are guaranteed to be ASCII-only, I think it'd be reasonable to just check for any characters outside the ASCII range (in either MacRoman or Unicode strings) and skip such names. That way the "encoding conversion" needed reduces to simply dropping the high byte of the Unicode chars in a Windows name.

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #189)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #188)
> > (I kinda think the former, at least if the new name is ASCII.)
> 
> My inclination is the latter, actually. It's perfectly possible to have a
> valid font (whether downloaded or locally installed) that doesn't include
> MacOS Roman name strings, so the cairo function really ought to be prepared
> to handle this rather than just punt.

I had the same thought reading over the previous comments.

    
    

    
  
Cairo 1.12 supports reading Windows platform Unicode names from the font.

    
    

    
  


  
This is Adrian's code, taken from cairo 1.12.

        Attachment #662348 -
        Flags: review?(jfkthame)

    
    

    
  
Comment on attachment 662348 [details] [diff] [review]
Part 0.4: Backport cairo 1.12's version of _cairo_truetype_read_font_name

Review of attachment 662348 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm. I notice that this changes the behavior of _cairo_truetype_read_font_name in significant ways, in addition to introducing support for the Windows-platform strings. I think we need to consider whether this is actually the desired behavior.

Issues:
(1) The semantics of _cairo_truetype_read_font_name are changed, in that it will return the font's *family* name in the font_name_out parameter, whereas previously it returned the font's *full name*. Which is appropriate depends on what it's being used for, of course; if it's for simple display to the user, the family name is usually what you'd want, but if it's supposed to be a face-specific identifier, the full name is more useful.

(2) The function now tries to "sanitize" the name by removing a prefix of the form "XXXXXX+", as found in font subsets embedded in PDF documents. This suggests to me that the primary intended use is to support displaying the names of fonts being used, but it means the name returned will not match what's actually present in the font.

Looking at how the function is used (in the part 0.5 patch), I think (1) would be OK, as it's actually the family name we compare (but that's a problem in itself, comment to follow); however, (2) seems likely to be a problem for fonts being extracted from PDF and loaded via @font-face - such as by pdf.js - and will probably mean they'll fall back to drawing paths or bitmaps. I think pdf.js is a pretty important use-case here, so we need to make sure it's handled correctly.

A few code-level comments below.

::: gfx/cairo/cairo/src/cairo-truetype-subset.c
@@ +1339,5 @@
> +            be16_to_cpu (record->platform) == platform &&
> +            be16_to_cpu (record->encoding) == encoding &&
> +            (language == -1 || be16_to_cpu (record->language) == language)) {
> +
> +            str = malloc (be16_to_cpu (record->length) + 1);

It seems inefficient to malloc and copy the raw string data here, as in the case of Windows (Unicode) strings, it's only going to be thrown away after converting to UTF-8. It'd be better to just convert directly from the data in the 'name' table buffer.

@@ +1373,5 @@
> +            goto fail;
> +        }
> +        p = utf8;
> +        for (i = 0; i < u_len; i++)
> +            p += _cairo_ucs4_to_utf8 (be16_to_cpu(u[i]), p);

This (and the size-computing loop above) will not handle UTF-16 surrogates correctly; in the (admittedly rare!) case of a name containing a non-BMP character, it will result in invalid UTF-8 because it will process each of the surrogate code units separately.

@@ +1426,4 @@
>  cairo_int_status_t
> +_cairo_truetype_read_font_name (cairo_scaled_font_t    *scaled_font,
> +                                char                  **ps_name_out,
> +                                char                  **font_name_out)

Probably should rename this to family_name_out, given the changed semantics.

@@ +1460,4 @@
>  
> +    /* Find PS Name (name_id = 6). OT spec says PS name must be one of
> +     * the following two encodings */
> +    status = find_name (name, 6, 3, 1, 0x409, &ps_name); /* win, unicode, english-us */

The structure here, with (up to) 6 successive calls to find_name, is painfully inefficient, especially as each call does a linear search over the name records - of which there may be 1000-plus in some fonts that have multiple localizations. It'd be better to do a single iteration that looks for all the names of interest, or else make find_name do a binary search (note that the name records are sorted by platform:encoding:language:name, precisely in order to permit binary searching).

@@ +1520,5 @@
> +            }
> +        }
> +        *dst = 0;
> +        free (ps_name);
> +        ps_name = strdup (buf);

This always does an extra allocation and copy, even in the case where the PS name in the font was already perfectly acceptable (which will almost always be the case - very few fonts have malformed PS name strings).

    
    

    
  
Comment on attachment 662350 [details] [diff] [review]
Part 0.5: _cairo_dwrite_scaled_font_create_win32_scaled_font should check font names to ensure that GDI gave us back the correct font

Review of attachment 662350 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp
@@ +1549,5 @@
> +                                                    NULL, 0);
> +                WCHAR* name = new WCHAR[name_len];
> +                MultiByteToWideChar (CP_UTF8, 0, new_font_name_utf8, -1,
> +                                     name, name_len);
> +                names_match = wcscmp(name, logfont.lfFaceName) == 0;

As noted above, the revised _cairo_truetype_read_font_name now returns the family name rather than the font name; this highlights the fact that what's being compared here is in fact only the family (because that's what the lfFaceName field of LOGFONT contains), not the face-specific fullname. Which means that if (for example) we asked for Arial Narrow, and GDI gave back plain Arial, we'd incorrectly believe this was a match. This feels rather fragile...

For better assurance, I think you should to retrieve the *fullname* from both the DWrite and GDI fonts, and check that they match. Or maybe the PS name. Or check both, given that _cairo_truetype_read_font_name wants to give you two names anyway.

    
    

    
  
Comment on attachment 662350 [details] [diff] [review]
Part 0.5: _cairo_dwrite_scaled_font_create_win32_scaled_font should check font names to ensure that GDI gave us back the correct font

>+        char *new_ps_name_utf8;
>+        char *new_font_name_utf8;
>+        cairo_int_status_t status;
>+
>+        status = _cairo_truetype_read_font_name (font, &new_ps_name_utf8,
>+                                                 &new_font_name_utf8);
>+        if (!status) {
>+            if (new_font_name_utf8) {
>+                int name_len = MultiByteToWideChar (CP_UTF8, 0,
>+                                                    new_font_name_utf8, -1,
>+                                                    NULL, 0);
>+                WCHAR* name = new WCHAR[name_len];
>+                MultiByteToWideChar (CP_UTF8, 0, new_font_name_utf8, -1,
>+                                     name, name_len);
>+                names_match = wcscmp(name, logfont.lfFaceName) == 0;
>+                delete name;
>+            }
>+            free (new_ps_name_utf8);
>+            free (new_font_name);

Should this be new_font_name_utf8?  new_font_name does not exist.

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #195)
> (1) The semantics of _cairo_truetype_read_font_name are changed, in that it
> will return the font's *family* name in the font_name_out parameter, whereas
> previously it returned the font's *full name*. Which is appropriate depends
> on what it's being used for, of course; if it's for simple display to the
> user, the family name is usually what you'd want, but if it's supposed to be
> a face-specific identifier, the full name is more useful.

Cairo uses _cairo_truetype_read_font_name when embedding fonts in PDF to get the names for the /FontName and /FontFamily entries in the PDF font descriptor. The PDF specification requires these entries to be set the the PS name and family name. The content of these entries has no effect on the rendering of the PDF. Usually these entries are displayed to the user by the PDF viewer when viewing the document properties. The /FontName entry may also be used to generate the names of the PS fonts when converting the PDF to PS.

> (2) The function now tries to "sanitize" the name by removing a prefix of
> the form "XXXXXX+", as found in font subsets embedded in PDF documents. This
> suggests to me that the primary intended use is to support displaying the
> names of fonts being used, but it means the name returned will not match
> what's actually present in the font.

When cairo embeds a subsetted font in PDF it prepends a six character subset tag unique to the subset as required by the PDF specification. When printing a PDF document the font names in the PDF font descriptor may already have tagged names so cairo strips it off before adding a new tag otherwise the font name will have two tags.

You can safely remove the stripping of subset tags from the patch since cairo 1.10 does not support subsetting of previously subsetted fonts. In this case it will fall back to generating a font from the outlines and naming the font of the form CairoFont-x-y.

    
    

    
  
(In reply to Adrian Johnson from comment #198)

> Cairo uses _cairo_truetype_read_font_name when embedding fonts in PDF to get
> the names for the /FontName and /FontFamily entries in the PDF font
> descriptor.

OK, so the change from returning fullname to family name makes sense for this usage - the older code was presumably resulting in the wrong /FontFamily name in some cases.

However, this confirms that using the "font name" returned here (really the family name) to verify that we've got the desired font (face) is not appropriate. We'd be better off checking the PS name, which ought to be a unique identifier for the individual face.

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #196)
> For better assurance, I think you should to retrieve the *fullname* from
> both the DWrite and GDI fonts, and check that they match. Or maybe the PS
> name. Or check both, given that _cairo_truetype_read_font_name wants to give
> you two names anyway.

If we're going to rely on the ability to read tables from both the DWrite and GDI fonts, could we do something simple like grab both NAME tables and require them to be byte-identical?

    
    

    
  
Unfortunately that doesn't work; the check always fails, because we can't read the NAME table from the DirectWrite font.

    
    

    
  
That seems odd - any idea why not? It should be there! Is _cairo_dwrite_load_truetype_table broken?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 0.5 v2
        — 
          — Splinter Review
      

    


  
Ah, it's because _cairo_dwrite_load_truetype_table has always been completely broken. Windows wants the first character of the tag in the low-order byte, but cairo passes it in the high-order byte.

With that fixed, this patch actually works. Both local and (with the patch in comment #167) downloaded fonts pass the test. I was afraid either GDI or DirectWrite or both might munge the name table, but they don't seem to.

        Attachment #662348 -
        Attachment is obsolete: true

        Attachment #662350 -
        Attachment is obsolete: true

        Attachment #662348 -
        Flags: review?(jfkthame)

        Attachment #662350 -
        Flags: review?(jfkthame)

        Attachment #662533 -
        Flags: review?(jfkthame)

    
    

    
  
Comment on attachment 662533 [details] [diff] [review]
part 0.5 v2

Review of attachment 662533 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, that would certainly explain the problem! I guess nobody's been using that function until now...

This looks much cleaner and more reliable now, thanks.

::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp
@@ +1491,5 @@
> +    cairo_int_status_t status1;
> +    cairo_int_status_t status2;
> +    unsigned char *buffer1;
> +    unsigned char *buffer2;
> +    cairo_bool_t result = 0;

It'd be more readable to use "false" here (and in the various early returns below), rather than 0.

@@ +1578,5 @@
>      }
>  
> +    if (!_name_tables_match (font, scaled_font) ||
> +        _cairo_win32_scaled_font_is_type1 (font) ||
> +        _cairo_win32_scaled_font_is_bitmap (font)) {

We probably don't need the _cairo_win32_scaled_font_is_* tests any more, as I'd expect load_truetype_table to fail on such fonts anyway. But if you want to keep them, then let's put them first as they're trivially cheap compared to the name-table comparison.

        Attachment #662533 -
        Flags: review?(jfkthame) → review+

    
    

    
  
(In reply to Jonathan Kew (:jfkthame) from comment #204)
> It'd be more readable to use "false" here (and in the various early returns
> below), rather than 0.

OK.

> We probably don't need the _cairo_win32_scaled_font_is_* tests any more, as
> I'd expect load_truetype_table to fail on such fonts anyway. But if you want
> to keep them, then let's put them first as they're trivially cheap compared
> to the name-table comparison.

I'll remove them.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #177)
> In printpreview case if there isn't parent, shouldn't you check 
> DocumentViewerImpl::FindContainerView() so that we don't end up creating
> rootprescontext in case pp is shown as a tab (like it usually is).

Done.

    
  
Depends on: 793473

    
  
Depends on: 793833
Depends on: 794579
Depends on: 794684
Depends on: 802476

    
  
Blocks: 880854

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: