Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842653005:
WebFrameSerializerSanitizationTest validates generated MHTML. (Closed)
DescriptionWebFrameSerializerSanitizationTest validates generated MHTML.
Changes GenerateMHTMLPats into GenerateMHTML and makes it generate
full and validated MHTML contents by default. For one test it made
more sense to only generate the body parts as before so there's an
optional argument to replicate the old behavior.
BUG=672313
Review-Url: https://codereview.chromium.org/2842653005
Cr-Commit-Position: refs/heads/master@{#468520}
Committed: https://chromium.googlesource.com/chromium/src/+/0179de0d5ca89c2bbdcf24f7a583b304fc053502
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated comments. #
Total comments: 8
Patch Set 3 : Generation helper methods; changed MHTML validation failure handling. #Patch Set 4 : Rebase. #Messages
Total messages: 30 (14 generated)
The CQ bit was checked by carlosk@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...
carlosk@chromium.org changed reviewers: + jianli@chromium.org, lukasza@chromium.org
lukasza@, jianli@: PTAL. dewittj@: FYI.
I don't think WebFrameSerializerSanitizationTest is the right place to validate the MHTML since WebFrameSerializerSanitizationTest is supposed to test the serialization logic in WebFrameSerializer, which does not deal with header and footer. If we couldn't put the validation in a better place, I am fine with adding the validation in this test file, but please create another testing class focusing exclusively on this. https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:341: "image/png", true); Please comment why we only need to generate body parts for this test.
On 2017/04/25 21:41:35, jianli wrote: > I don't think WebFrameSerializerSanitizationTest is the right place to validate > the MHTML since WebFrameSerializerSanitizationTest is supposed to test the > serialization logic in WebFrameSerializer, which does not deal with header and > footer. I disagree FWIW :-). 1. I think it is desirable to validate that output of WebFrameSerializer is accepted as valid by MHTMLParser for all the different inputs currently covered by WebFrameSerializerTest.cpp. 2. I think the extra validation is fine in WebFrameSerializerTest.cpp: 2a. The extra validation is applicable to the body/parts of the MHTML that are being generated by WebFrameSerializer (i.e. I don't think the validation should be moved to a separate test suite). 2b. I think the new validation is fine layering-wise - I think that it is fine for web/tests/WebFrameSerializerTest.cpp to depend on platform/mhtml.
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:231: // This boundary is as good as any other. Plus it gets used in almost I'd rewrite this comment, since it's not obvious why you are justifying this exactly. Is it important that it is constant rather than randomly generated as in real files? Is its length important? etc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for all comments. On 2017/04/25 22:16:07, Łukasz A. wrote: > On 2017/04/25 21:41:35, jianli wrote: > > I don't think WebFrameSerializerSanitizationTest is the right place to > validate > > the MHTML since WebFrameSerializerSanitizationTest is supposed to test the > > serialization logic in WebFrameSerializer, which does not deal with header and > > footer. > > I disagree FWIW :-). > > 1. I think it is desirable to validate that output of WebFrameSerializer is > accepted as valid by MHTMLParser for all the different inputs currently covered > by WebFrameSerializerTest.cpp. > > 2. I think the extra validation is fine in WebFrameSerializerTest.cpp: > > 2a. The extra validation is applicable to the body/parts of the MHTML that are > being generated by WebFrameSerializer (i.e. I don't think the validation should > be moved to a separate test suite). > > 2b. I think the new validation is fine layering-wise - I think that it is fine > for web/tests/WebFrameSerializerTest.cpp to depend on platform/mhtml. My mains objective here is to check that changes in WebFrameSerializer will not create invalid MHTML output. So even though I had to add a little cruft to test MHTML generation, that allows the use of MHTMLParser which offers the simplest way to get that validation done. I didn't change WebFrameSerializerTest because those tests are comparing expectations directly with the raw output. So as long as the expectations are valid -- and I'm assuming they are -- the output should be valid too. https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:231: // This boundary is as good as any other. Plus it gets used in almost On 2017/04/25 22:32:33, dewittj wrote: Context: this is a copy/paste from MHTMLTest::GenerateMHTMLData where I found this comment. As I would have to promote the unnamed parameter into a named value here to be used for all calls I thought it was better to get the "justified" value and its explanation. > I'd rewrite this comment, since it's not obvious why you are justifying this > exactly. Is it important that it is constant rather than randomly generated as > in real files? Is its length important? etc. Rephrased. LMKWDYT. https://codereview.chromium.org/2842653005/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:341: "image/png", true); On 2017/04/25 21:41:34, jianli wrote: > Please comment why we only need to generate body parts for this test. Done. I left the (currently unnecessary) check below in place in case extra validation is eventually added that would run even when generating only body parts.
https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:217: class WebFrameSerializerSanitizationTest : public WebFrameSerializerTest { I think we'd better move this class out of this file to become a standalone one since now we test the whole MHTML generation. This can be done in a separate patch. https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:256: if (parser.ParseArchive().IsEmpty()) { Do we have plan to validate the number of resources? https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:345: "image/png", true); I don't think you need to pass another parameter since you can simply check mime type. If it is not text/html, there is no need to generate other parts.
Thanks. https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:217: class WebFrameSerializerSanitizationTest : public WebFrameSerializerTest { On 2017/04/26 00:54:45, jianli wrote: > I think we'd better move this class out of this file to become a standalone one > since now we test the whole MHTML generation. This can be done in a separate > patch. Acknowledged. I will follow up with that. https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:256: if (parser.ParseArchive().IsEmpty()) { On 2017/04/26 00:54:45, jianli wrote: > Do we have plan to validate the number of resources? For MHTML well-formed-ness checks -- which is the only thing I wanted to check for -- we don't need to. Any MHTML validation error will cause the output to be fully empty. https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:345: "image/png", true); On 2017/04/26 00:54:45, jianli wrote: > I don't think you need to pass another parameter since you can simply check mime > type. If it is not text/html, there is no need to generate other parts. I would prefer to keep it explicit because handling mime types differently seems counter intuitive. This is difference in behavior is specific to this test to make it easier to test its results and not because the different mime type would change MHTML generation. WDYT?
https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:345: "image/png", true); On 2017/04/26 01:07:50, carlosk wrote: > On 2017/04/26 00:54:45, jianli wrote: > > I don't think you need to pass another parameter since you can simply check > mime > > type. If it is not text/html, there is no need to generate other parts. > > I would prefer to keep it explicit because handling mime types differently seems > counter intuitive. This is difference in behavior is specific to this test to > make it easier to test its results and not because the different mime type would > change MHTML generation. > > WDYT? I prefer not to adding too many arguments to complicate things. Since only one test is involved, adding one more custom argument seems to be too much. Also I think it makes sense not to generate header and foot for any non-html serialization.
Description was changed from ========== WebFrameSerializerSanitizationTest validates generated MHTML. Changes GenerateMHTMLPats into GenerateMHTML and makes it generate full and validated MHTML contents by default. For one test it made more sense to only generate the body parts as before so there's an optional argument to replicate the old behavior. BUG=672313 ========== to ========== WebFrameSerializerSanitizationTest validates generated MHTML. Changes GenerateMHTMLPats into GenerateMHTML and makes it generate full and validated MHTML contents by default. For one test it made more sense to only generate the body parts as before so there's an optional argument to replicate the old behavior. BUG=672313 ==========
dewittj@chromium.org changed reviewers: - dewittj@chromium.org
Because of a comment from dcheng@ on another CL I changed the way MHTML well-formedness failures are handled to eliminate the need of the repetitive ASSERT_FALSE(HasFailures()). Now the MHTML content is still returned even if invalid and other checks run as they should. In the end the test will still fail in case it is indeed invalid. https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right): https://codereview.chromium.org/2842653005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:345: "image/png", true); On 2017/04/26 01:12:27, jianli wrote: > On 2017/04/26 01:07:50, carlosk wrote: > > On 2017/04/26 00:54:45, jianli wrote: > > > I don't think you need to pass another parameter since you can simply check > > mime > > > type. If it is not text/html, there is no need to generate other parts. > > > > I would prefer to keep it explicit because handling mime types differently > seems > > counter intuitive. This is difference in behavior is specific to this test to > > make it easier to test its results and not because the different mime type > would > > change MHTML generation. > > > > WDYT? > > I prefer not to adding too many arguments to complicate things. Since only one > test is involved, adding one more custom argument seems to be too much. Also I > think it makes sense not to generate header and foot for any non-html > serialization. Changing the returned contents solely based on mime type still feels too surprising to me. But I added two helper methods that keep calls from tests simple, with only two parameters. WDYT?
Just sending a reminder for reviewers to PTAL. :)
lgtm
lgtm
The CQ bit was checked by carlosk@chromium.org
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org, jianli@chromium.org Link to the patchset: https://codereview.chromium.org/2842653005/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493682441850780,
"parent_rev": "6ce865fc28199a11109d3b72ad3b5eb52b3ae826", "commit_rev":
"0179de0d5ca89c2bbdcf24f7a583b304fc053502"}
Message was sent while issue was closed.
Description was changed from ========== WebFrameSerializerSanitizationTest validates generated MHTML. Changes GenerateMHTMLPats into GenerateMHTML and makes it generate full and validated MHTML contents by default. For one test it made more sense to only generate the body parts as before so there's an optional argument to replicate the old behavior. BUG=672313 ========== to ========== WebFrameSerializerSanitizationTest validates generated MHTML. Changes GenerateMHTMLPats into GenerateMHTML and makes it generate full and validated MHTML contents by default. For one test it made more sense to only generate the body parts as before so there's an optional argument to replicate the old behavior. BUG=672313 Review-Url: https://codereview.chromium.org/2842653005 Cr-Commit-Position: refs/heads/master@{#468520} Committed: https://chromium.googlesource.com/chromium/src/+/0179de0d5ca89c2bbdcf24f7a583... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0179de0d5ca89c2bbdcf24f7a583... |
