Summary: | [iOS][wk2] Use ImageDocument to display subframe PDFs | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||
Component: | Assignee: | Tim Horton <thorton> | |||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, beidson, buildbot, commit-queue, dbates, esprehn+autocc, gyuyoung.kim, japhet, kangil.han, rniwa, sam, simon.fraser | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=184161 | ||||||||||
Attachments: |
|
Description
Tim Horton
2014-06-16 11:55:16 PDT
Created attachment 233187 [details]
patch
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 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 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
Looks like both of the EWS failures are not legitimate (mac tree was red, efl is having compiler internal errors) (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. Created attachment 233251 [details]
new patch
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. (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 :) |