[webkit-reviews] review denied: [Bug 3527] Allow Safari to open postscript files in browser windows as well : [Attachment 5835] Patch v1 (initial hack)

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied David D. Kilzer
<ddkilzer at kilzer.net>'s request for review:
Bug 3527: Allow Safari to open postscript files in browser windows as well
http://bugzilla.opendarwin.org/show_bug.cgi?id=3527

Attachment 5835: Patch v1 (initial hack)
http://bugzilla.opendarwin.org/attachment.cgi?id=5835&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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!