[webkit-reviews] review granted: [Bug 133944] [iOS][wk2] Use ImageDocument to display subframe PDFs : [Attachment 233251] new patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 133944: [iOS][wk2] Use ImageDocument to display subframe PDFs
https://bugs.webkit.org/show_bug.cgi?id=133944

Attachment 233251: new patch
https://bugs.webkit.org/attachment.cgi?id=233251&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233251&action=review


This patch seems sane to me. Feel free to ask a domain expert to review this
patch if you're looking for a thorough review.

> Source/WebCore/dom/DOMImplementation.cpp:319
> +    if ((!frame || !frame->isMainFrame()) &&
MIMETypeRegistry::isPDFMIMEType(type) && (frame &&
frame->settings().useImageDocumentForSubframePDF()))

This expression can be simplified to:

if (frame && !frame->isMainFrame() && MIMETypeRegistry::isPDFMIMEType(type) &&
frame->settings().useImageDocumentForSubframePDF())

Notice that "!frame && frame" always evaluates to false by the truth table of
the logical AND (&&) operator.

On another note, in practice is frame ever null in this function. I assume it
can be from looking at the code in this function, especially the explicit null
check around the logic for assigning a value to allowedPluginTypes. However, we
don't null check frame before dereferencing it when we call
ImageDocument::create() below (line 336).

> Source/WebCore/platform/MIMETypeRegistry.cpp:573
> +

This empty line seems extraneous. One empty line seems sufficient to provide
visual separation between the end of this function and the start of the next
function.