Bug 3527 – Allow Safari to open postscript files in browser windows as well

• Component: Plug-ins
• Status: VERIFIED FIXED

[Jordan Breeding]

Apple Bug: <rdar://problem/4106061>

Allow Safari to open postscript files in a browser window, as it does for pdf files.

[Chris Petersen]

Confirming request.

[John Sullivan]

This would require a new WebDocumentView implementation parallel to WebPDFView. That one was fairly 
straightforward to implement because another framework had done much of the work for us (PDFKit). I 
don't know if there's any existing code floating around to display postscript files.

This doesn't seem anywhere near as useful as displaying pdf files, since (at least on Mac OS X) pdf files are 
quite pervasive but raw postscript files are much less commonly encountered. On the other hand, it 
doesn't seem like it would cause any trouble either.

[Jordan Breeding]

(In reply to comment #2)
> This would require a new WebDocumentView implementation parallel to
WebPDFView. That one was fairly 
> straightforward to implement because another framework had done much of the
work for us (PDFKit). I 
> don't know if there's any existing code floating around to display postscript
files.
> 

Preview uses PDFKit now correct?  Preview can display pdf and postscript, does
it just pass the postscript to PDFKit, or does it translate the document to pdf
and then use PDFKit?

I would be fine if Safari ultimately translated postscript files to pdf and then
displayed the result in a window, reusing existing code as much as possible.  I
just still run into postscript files enough that being able to quickly view them
in Safari would be very nice.

[Oliver Hunt]

As far as i'm aware preview convert postscript to pdf (it even leaves the
pdfafied file) and displays the pdf.

[Jordan Breeding]

(In reply to comment #4)
> As far as i'm aware preview convert postscript to pdf (it even leaves the
> pdfafied file) and displays the pdf.

That's what I thought happend, so the question is: can WebPDFView be made to handle files with .ps 
extensions and simply do the same ps->pdf conversion before displaying them?

[Jordan Breeding]

(In reply to comment #0)
> Apple Bug: <rdar://problem/4106061>
> 
> Allow Safari to open postscript files in a browser window, as it does for pdf files.

Any chance that this will get added to PDFKit/PDFView?

[David Kilzer (:ddkilzer)]

Created attachment 5798 [details]
My favorite sample PS file

[David Kilzer (:ddkilzer)]

According to this developer documentation, Preview uses a Quartz 2D API to do the conversion, then display the PDF.

http://developer.apple.com/documentation/GraphicsImaging/Conceptual/drawingwithquartz2d/dq_ps_convert/chapter_16_section_1.html

It should be possible to do the same thing in WebKit and reuse the existing PDF viewing code.

[David Kilzer (:ddkilzer)]

Created attachment 5835 [details]
Patch v1 (initial hack)

This patch is basically a first-pass hack at viewing of PostScript files work in Safari.  Looking for feedback on the following:

- Class design.  I basically copied WebPDFRepresentation.h/m to WebPostScriptRepresentation.h/m, changed the class names, and added code to the finishedLoadingWithDataSource() method.  Is there a better way to do this?  Should I create a base class for them?  Or combine them into a single class and conditionalize the PS-to-PDF conversion (not very OO-design-worthy, though)?

- Am I doing anything stupid with the Core Graphics APIs or not releasing references that I should in the PS-to-PDF code?  Specifically, am I using "toll-free" casts?

- The CGDataConsumerCreateWithCFData() call was added in 10.4.  Is this an issue (backward compatibility with 10.3)?

- Is there a better way to check the value of 'success'?

[David Kilzer (:ddkilzer)]

Here are a couple of Google queries to find PostScript (or EPS) files out in the "wild":

http://www.google.com/search?hl=en&q=filetype%3Aps+postscript

http://www.google.com/search?hl=en&lr=&q=filetype%3Aeps+eps

Oh, one more review question:  The PS-to-PDF conversion process can take a bit of time where the browser appears to be just taking lots of CPU.  Is there a way to alert the user of this time (like the dialog that appears in Preview when it converts PS files)?

[Darin Adler]

(In reply to comment #9)
> - Class design.  I basically copied WebPDFRepresentation.h/m to
> WebPostScriptRepresentation.h/m, changed the class names, and added code to the
> finishedLoadingWithDataSource() method.  Is there a better way to do this? 
> Should I create a base class for them?  Or combine them into a single class and
> conditionalize the PS-to-PDF conversion (not very OO-design-worthy, though)?

Combining into a single class might be the right thing to do, since we're planning on changing around the whole representation/view thing later. This first cut seems pretty neat, though, so no reason to redo it.

> - Am I doing anything stupid with the Core Graphics APIs or not releasing
> references that I should in the PS-to-PDF code?  Specifically, am I using
> "toll-free" casts?

I think there are some missing release calls.

> - The CGDataConsumerCreateWithCFData() call was added in 10.4.  Is this an
> issue (backward compatibility with 10.3)?

Not an issue.

> - Is there a better way to check the value of 'success'?

I believe what you're really asking is: "What should the representation class do if there's a failure?" That leads to the questions:

    1) When can this fail?
    2) What do we want the user to see if it does fail?

We should consider both of those.

[Darin Adler]

Comment on attachment 5835 [details]
Patch v1 (initial hack)

I don't think these new files should go into a different "group" than PDF. They are closely related and tied to the PDF classes and so should share the same group in Xcode.

In WebPostScriptRepresentation.m, it's important that WebPostScriptRepresentation.h is the first file imported. That way the Obj-C source file serves as a check that the header "stands alone". That's one of our standards although might not be mentioned in our coding standards document at the moment (and should be).

Please don't check in the commented-out MIME types. If you want to write a FIXME comment about them, that's OK, but we don't check in ifdef'd or commented-out code as a rule.

We always use 0 instead of NULL -- a debatable coding guidelines, but one I'd like you to follow.

The converter, provider, and consumer all need to be released after the call to CGPSConverterConvert so they don't leak.

The ASSERT inside an if statement is not a good style. I'd suggest just the ASSERT for now, but that's even worse since we'll get an unused variable warning in a deployment build. Consider at least adding a return statement after that assert? Lets figure out what we want to do on failure.

The NSMutableData object has to be initialized, not just allocated. I'd suggest doing the toll-free bridging in the other direction, creating a CFMutableData and then casting it to pass it to the PDFDocument. But if you want to do it in this direction you need to add the init call.

The result also leaks. Since it's allocated, you need to release it.

I think it's worth considering just building this into WebPDFRepresentation. That might be more forward-looking since we plan to phase out the representation/view separation in the near future (although we might keep it in public API for compatibility). This is the first good use of that separation I've seen!

[David Kilzer (:ddkilzer)]

Created attachment 6278 [details]
Patch v2

Updates for this patch:

- Consolidated code changes to one class--WebPDFRepresentation.m.
- Added a number of ASSERT()s (too many?).
- Removed commented-out code.
- Changed 'NULL' to '0'.
- Added releases for converter, provider, consumer, data/result.
- Initialized the NSMutableData * data structure.

Regarding the use of NSMutableData * instead of CFMutableData, I found that the following declaration would eventually cause a segmentation fault (after viewing 3-6 PS files in the browser):

    CFMutableDataRef result = CFDataCreateMutable(kCFAllocatorDefault, (CFIndex)[data length]);

if it replaced this call:

    NSMutableData *result = [[NSMutableData alloc] initWithCapacity:[data length]];

Note that when I made the above change, I also changed how the CFMutableData was released (using CFRelease()), but I still got the crash.

Finally, I'm not sure what should happen if CGPSConverterConvert() fails.  The ASSERT(success) will throw some kind of assertion exception, I suppose.

[David Kilzer (:ddkilzer)]

Created attachment 6279 [details]
Patch v3

Updates for this patch (ignore comments for Patch v2):

- Consolidated code changes to one class--WebPDFRepresentation.m.
- Added a number of ASSERT()s (too many?).
- Removed commented-out code.
- Changed 'NULL' to '0'.
- Added releases for converter, provider, consumer.
- Set result to autorelease.
- Switched from using NSMutableData * to CFMutableDataRef.

Finally, I'm not sure what should happen if CGPSConverterConvert() fails.  The
ASSERT(success) will throw some kind of assertion exception, I suppose.

[Darin Adler]

Comment on attachment 6279 [details]
Patch v3

Very nice patch. Great idea, great execution.

The code in convertPostScriptDataSourceToPDF: is incorrect for garbage collection. We can't take a CFMutableDataRef and autorelease it. We need to use WebCFAutorelease instead -- probably best to do that on the return line at the end of the function, since it also takes care of the type conversion and then there's no need to cast to (id).

Ideally I'd also like to see error handling in there too. Isn't there a chance the converter will fail? If so, maybe we can handle it gracefully. I suppose it possibly already does the right thing by returning an empty NSData. We should think about what the most useful behavior would be, and perhaps set up a test page that causes that function to fail.

+    bool success = false;
+    success = CGPSConverterConvert(converter, provider, consumer, 0);

Should collapse those into one line.

Need to test compiling Release, because I think that "success" will cause an unused variable warning. It's tricky to figure out a way to get a result and use it only for an assertion -- that's yet another reason to handle the failure case.

For CGPSConverterCreate, can you just pass 0 instead of a callbacks array that is all 0? I ask because that's how the CF classes like CFSet work.

+    NSData *data = nil;

Better to declare this right before it's set up, and no reason to set it to nil if it's always set to something else. In fact, I think this would be a nice way to have it read:

+    NSData *data = [dataSource data];
+    bool isPostScript = [postScriptMIMETypes containsObject:mimeType];
+    if (isPostScript)
+        data = [self convertPostScriptDataSourceToPDF:data];

I noticed you added a new copyright date for Apple. That means you're handing over copyright for your contribution to Apple. If that wasn't your intention, please mention yourself in a copyright line instead. If that was you intention, thank you!

[David Kilzer (:ddkilzer)]

Created attachment 6313 [details]
Patch v4

Update from Patch v3:

- Now use WebCFAutorelease() on result object at end of convertPostScriptDataSourceToPDF()

- CGPSConverterConvert() fails by returning false and leaving the result object zero-length.

- It is not possible to pass in 0 in place of the CGPSConverterCallbacks struct to CGPSConverterCreate().  (I got a Bus Error inside CGPSConverterCreate() when I tried it.)

- Restructured code per Comment #15.

- Added code to display an NSAlert exactly like Preview does when it fails to convert a PostScript file.  Not sure if that's what you want to do ultimately, but it seems to work as I expected it to!

- Return early from finishedLoadingWithDataSource() if NSData* returned from convertPostScriptDataSourceToPDF() is zero length.  (This leaves the window "darkened", though, which happens before a PDF is displayed.)  If I pass the empty NSData* through, nothing else happens except that WebKit/Safari prints this to either stdout or stderr:  "Failed to find PDF header: `%PDF' not found."

- Giving the copyright to Apple since I'm not sure what good it would do me for this small amount of code. :)

[David Kilzer (:ddkilzer)]

Created attachment 6314 [details]
Invalid PostScript file for testing

[Darin Adler]

Comment on attachment 6313 [details]
Patch v4

This is looking great, but not quite ready to land.

1)

Despite my earlier request that we report this error somehow, it's not appropriate for WebKit to put up an alert directly. We don't know what use the WebView is being put to -- for example it could be used entirely off-screen. So it's not appropriate to present an alert directly to the user.

Also, if we did want to present the alert directly to the user we'd need to localize the strings using UI_STRING.

I suggest making a version of the patch without the alert and landing it that way. We can discuss further handling of the error after landing the initial version.

Sorry for leading you astray with my earlier comment about error handling.

2)

+        data = [self convertPostScriptDataSourceToPDF:[dataSource data]];

This line should say:

+        data = [self convertPostScriptDataSourceToPDF:data];

[David Kilzer (:ddkilzer)]

Created attachment 6337 [details]
Patch v5

Update from Patch v4:

1) Removed NSAlert.  Replaced with comment about how errors are handled.
2) Fixed duplicative code.

[Darin Adler]

Comment on attachment 6337 [details]
Patch v5

OK. Looks good. Lets land it. r=me

[Darin Adler]

We could also consider adding PostScript files to the list of file types that Safari can open in the application's property list.

[David Kilzer (:ddkilzer)]

Verified fixed on r12988.

[David Kilzer (:ddkilzer)]

Committed r12679:  <https://trac.webkit.org/r12679>

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #23)
> Committed r12679:  <https://trac.webkit.org/r12679>

Removed almost 16 years later for Bug 235892 in r288922 as a side-effect from sandbox tightening:

[WP] Remove PostScript conversion code
<https://bugs.webkit.org/show_bug.cgi?id=235892>
<https://commits.webkit.org/r288922>