[webkit-reviews] review denied: [Bug 3527] Allow Safari to open postscript files in browser windows as well : [Attachment 6279] Patch v3

[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 6279: Patch v3
http://bugzilla.opendarwin.org/attachment.cgi?id=6279&action=edit

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