Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2425863002:
Allow image documents in MHT if there is only one resource. (Closed)
DescriptionAllow image documents in MHT if there is only one resource.
MHTML could not show image documents as the main frame; this adds a check
for suitable image mime types and allows them as the main frame. Also
reorders the logic for clarity.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel
BUG=584866
Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850
Committed: https://crrev.com/d86f027d196d3b1e52101aaaefc3280217e962ae
Cr-Original-Commit-Position: refs/heads/master@{#428193}
Cr-Commit-Position: refs/heads/master@{#428435}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove the png tests. #Patch Set 3 : Rebaselined. #Patch Set 4 : Fix test expectations file #Messages
Total messages: 51 (36 generated)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,linux_precise_blink_dbg,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,win10_blink_rel,linux_trusty_blink_rel BUG=584866 ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,linux_precise_blink_dbg,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,win10_blink_rel,linux_trusty_blink_rel BUG=584866 ==========
dewittj@chromium.org changed reviewers: + dimich@chromium.org, lukasza@chromium.org
PTAL, thanks! This adds a layout test for just image docs and also changes expectations for another test which had previously relied on the fact that we didn't render mht files with just an image resource.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac10.9_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac10.9_blink_rel/builds...)
LGTM https://codereview.chromium.org/2425863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right): https://codereview.chromium.org/2425863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:95: // be considered the main resource. I thought that MHTML spec requires the first part to be the main resource, but after rereading rfc2557, section 7 I am not so sure. At any rate, I guess keeping the old behavior (i.e. treating the first text/html part as the main resource) makes sense (i.e. I don't see a compelling reason to start questioning this old behavior).
LGTM, the tests seem still failing? Looks good to create a single-resource-which-is-image case, since this is the only reasonable variant the image documents can be serialized to.
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac10.10_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac10.10_blink_rel/build...)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dewittj@chromium.org changed reviewers: + eae@chromium.org
eae@chromium.org: PTAL, thanks!
RS LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win10_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win10_blink_rel/builds/1227)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win10_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win10_blink_rel/builds/1235)
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org, dimich@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2425863002/#ps40001 (title: "Rebaselined.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win10_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,linux_precise_blink_dbg,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,win10_blink_rel,linux_trusty_blink_rel BUG=584866 ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_dbg,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 ==========
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_dbg,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 ==========
The CQ bit was checked by dewittj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2460843002/ by phoglund@chromium.org. The reason for reverting is: This changelist re-enables a bunch of tests that fail (they report Missing): https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win1... Please write explicitly that you're re-enabling tests in the CL description next time, that would have made sheriffing much easier. The CL description only mentions re-ordering some logic and not that it's also re-enabling tests..
Message was sent while issue was closed.
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193} ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193} ==========
relanding without the unintended changes to TestExpectations.
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org, dimich@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2425863002/#ps60001 (title: "Fix test expectations file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193} ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Cr-Commit-Position: refs/heads/master@{#428193} ========== to ========== Allow image documents in MHT if there is only one resource. MHTML could not show image documents as the main frame; this adds a check for suitable image mime types and allows them as the main frame. Also reorders the logic for clarity. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:win7_blink_dbg,linux_precise_blink_rel,mac10.9_blink_dbg,mac10.9_blink_rel,win7_blink_rel,mac10.10_blink_rel,mac10.11_blink_rel,mac10.11_retina_blink_rel,linux_trusty_blink_rel BUG=584866 Committed: https://crrev.com/9fd54d584c2449577e25f761c3ea5ba24a142850 Committed: https://crrev.com/d86f027d196d3b1e52101aaaefc3280217e962ae Cr-Original-Commit-Position: refs/heads/master@{#428193} Cr-Commit-Position: refs/heads/master@{#428435} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d86f027d196d3b1e52101aaaefc3280217e962ae Cr-Commit-Position: refs/heads/master@{#428435} |
