Bug 133944 – [iOS][wk2] Use ImageDocument to display subframe PDFs

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Tim Horton]

Since WKPDFView isn’t immediately useful in subframes, let’s get subframe PDFs at least displaying their first page by making WebCore display them as ImageDocuments.

<rdar://problem/17205983>

[Tim Horton]

Created attachment 233187 [details]
patch

[Tim Horton]

Comment on attachment 233187 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233187&action=review

> Source/WebCore/html/ImageDocument.cpp:219
> +    if (document().loader()->responseMIMEType() == "application/pdf")

this should use the mime type registry

[Build Bot]

Comment on attachment 233187 [details]
patch

Attachment 233187 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5078106113048576

New failing tests:
fast/xpath/4XPath/Borrowed/rs_20010831.html
fast/dom/domparser-parsefromstring-mimetype-support.html
fast/xpath/4XPath/Core/test_core_functions.html
fast/xpath/document-order.html
fast/xpath/4XPath/Core/test_node_test.html
fast/xsl/default-html.html
fast/xpath/empty-string-substring.html
fast/dom/allowed-children.html
fast/dom/Node/normalize-with-cdata.html
fast/html/xhtml-serialize.html
fast/dom/dom-parse-serialize-xmldecl.html
fast/xpath/4XPath/Core/test_location_path.html
fast/xpath/ambiguous-operators.html
fast/xsl/extra-lf-at-end.html
fast/xpath/4XPath/Core/test_boolean_expr.html
fast/xpath/attr-namespace.html
fast/xpath/py-dom-xpath/axes.html
fast/xpath/4XPath/Borrowed/cz_20030217.html
fast/xpath/py-dom-xpath/data.html
fast/dom/attribute-namespaces-get-set.html
fast/dom/Range/range-processing-instructions.html
fast/dom/dom-parse-serialize-display.html
fast/dom/Document/replace-child.html
fast/xpath/py-dom-xpath/expressions.html
fast/dom/XMLSerializer.html
fast/xpath/py-dom-xpath/abbreviations.html
fast/dom/dom-parse-serialize.html
fast/xpath/4XPath/Borrowed/namespace-nodes.html
fast/xsl/import-non-document-node.xhtml
fast/xpath/4XPath/Borrowed/kd_20010423.html

[Build Bot]

Created attachment 233201 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Tim Horton]

Looks like both of the EWS failures are not legitimate (mac tree was red, efl is having compiler internal errors)

[Tim Horton]

(In reply to comment #5)
> Looks like both of the EWS failures are not legitimate (mac tree was red, efl is having compiler internal errors)

Nevermind, the crashes are real.

[Tim Horton]

Created attachment 233251 [details]
new patch

[Daniel Bates]

Comment on attachment 233251 [details]
new patch

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.

[Tim Horton]

(In reply to comment #8)
> (From update of attachment 233251 [details])
> 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.

Sure!

> 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).

It can! See DOMParser::parseFromString. That said, I don’t think it can ever be null *and* try to make an ImageDocument, it will be HTML or XML.

> > 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.

Just a mistake :)

[Tim Horton]

http://trac.webkit.org/changeset/170091