Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113893005:
Adding tests for WebContent.DownloadImage() (Closed)
|
Created:
4 years, 5 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, palmer Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding tests for WebContent.DownloadImage()
This CL adds tests to check that WebContent.DownloadImage() allows HTTP
scheme but denies file scheme.
BUG=626281
Committed: https://crrev.com/b94c2c360f20d9b2aef20b2efcf0ac406b9ccaba
Cr-Commit-Position: refs/heads/master@{#404120}
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed pkotwicz's comments #
Total comments: 2
Patch Set 3 : addressed pkotwicz's comments #
Total comments: 10
Patch Set 4 : fixed nits #
Total comments: 4
Patch Set 5 : fixed nits #Messages
Total messages: 27 (8 generated)
Description was changed from ========== Adding test for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=None ========== to ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=None ==========
zqzhang@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi pkotwicz@, can you review this CL for adding image downloader tests? I wrote another CL (https://codereview.chromium.org/2015433003/) which propagates an URL from Blink to the browser, and the URL will be passed to WebContents::DownloadImage() to fetch Bitmaps. palmer@ suggested me to add some tests to make sure the image downloader does not break the security rules (e.g. fetching file:// scheme on a http:// origin).
Thank you for adding tests. I think that it would be worth checking that the downloaded image is non empty. In particular |bitmaps| in OnFinishDownloadImage has size > 1 and has a non-null SkBitmap
Sorry, I am looking at your CL in more detail and I don't think that my previous comment (Comment #4) makes sense
On 2016/07/04 15:30:31, pkotwicz wrote: > Sorry, I am looking at your CL in more detail and I don't think that my previous > comment (Comment #4) makes sense Thanks, take your time :)
https://codereview.chromium.org/2113893005/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl_browsertest.cc:1173: public base::RefCountedThreadSafe<DownloadImageObserver> { Why is DownloadImageObserver ref counted? The test times out if the callback is not called https://codereview.chromium.org/2113893005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl_browsertest.cc:1225: embedded_test_server()->GetURL("/simple_page.html"); I don't think that the page URL affects whether the download is successful. If the page URL is a file URL and image URL is a HTTP URL the download still passes I believe that WebContents::DownloadImage() can be used to download a URL completely unrelated to the current page.
PTAL, addressed comments. https://codereview.chromium.org/2113893005/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl_browsertest.cc:1173: public base::RefCountedThreadSafe<DownloadImageObserver> { On 2016/07/04 17:17:01, pkotwicz wrote: > Why is DownloadImageObserver ref counted? Oops, I forget using base::Unretained in base::Bind, so that require some refcounting. Now problem solved. > The test times out if the callback is not called Added loop_runner_->Quit() when unmatch. https://codereview.chromium.org/2113893005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl_browsertest.cc:1225: embedded_test_server()->GetURL("/simple_page.html"); On 2016/07/04 17:17:01, pkotwicz wrote: > I don't think that the page URL affects whether the download is successful. > > If the page URL is a file URL and image URL is a HTTP URL the download still > passes > > I believe that WebContents::DownloadImage() can be used to download a URL > completely unrelated to the current page. OK, so now I just test for cases: file/http page * file/http image. I was thinking of testing cross-origin scenario but given this I don't need do that. Though I'm a bit worried that does it violate security rules if we can fetch any http url from a page? (maybe for downloadImage() is fine)
https://codereview.chromium.org/2113893005/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1223: DownloadImage_HttpPage_HttpImage) { I tried this test locally and it fails. I think that you should have just two tests: DownloadImage_HttpImage DownloadImage_Deny_FileImage You can make both tests call Shell::LoadURL(GURL("about:blank"))
PTAL. https://codereview.chromium.org/2113893005/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1223: DownloadImage_HttpPage_HttpImage) { On 2016/07/04 20:17:17, pkotwicz wrote: > I tried this test locally and it fails. > > I think that you should have just two tests: > DownloadImage_HttpImage > DownloadImage_Deny_FileImage > > You can make both tests call Shell::LoadURL(GURL("about:blank")) Done.
https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1202: OnFinishDownloadImage(_, Ne(expected_http_status), _, _, _)) Can you replace "Ne(expected_http_status)" with "_" and remove lines 1198 - 1200? https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1207: // Load test URL and start download image. Nit: Please update (or remove) the comment https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1224: Nit: Remove blank line https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1227: Nit: Remove blank line https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1235: Nit: Remove blank line
zqzhang@chromium.org changed reviewers: + jochen@chromium.org
PTAL, fixed nits. +jochen for approval. https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1202: OnFinishDownloadImage(_, Ne(expected_http_status), _, _, _)) On 2016/07/05 18:00:36, pkotwicz wrote: > Can you replace "Ne(expected_http_status)" with "_" and remove lines 1198 - > 1200? Done. https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1207: // Load test URL and start download image. On 2016/07/05 18:00:36, pkotwicz wrote: > Nit: Please update (or remove) the comment Done. https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1224: On 2016/07/05 18:00:36, pkotwicz wrote: > Nit: Remove blank line Done. https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1227: On 2016/07/05 18:00:36, pkotwicz wrote: > Nit: Remove blank line Done. https://codereview.chromium.org/2113893005/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1235: On 2016/07/05 18:00:36, pkotwicz wrote: > Nit: Remove blank line Done.
LGTM. Someone else will need to look at the GMock specific stuff though - in particular the InvokeWithoutArgs() syntax. I am not a GMock syntax expert. https://codereview.chromium.org/2113893005/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1188: using ::testing::Ne; Nit: You do not need line 1188 anymore https://codereview.chromium.org/2113893005/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1201: })); Nit: The indentation of line 1201 looks off.
https://codereview.chromium.org/2113893005/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2113893005/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1188: using ::testing::Ne; On 2016/07/05 19:05:04, pkotwicz wrote: > Nit: You do not need line 1188 anymore Done. https://codereview.chromium.org/2113893005/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1201: })); On 2016/07/05 19:05:04, pkotwicz wrote: > Nit: The indentation of line 1201 looks off. This seems to be because of the lambda. I now use the "object+class method" version of InvokeWithoutArgs, so no more indentation issues :)
have you considered adding a unit test as opposed to browser tests with mocks? what's the motivation for that CL? Is this part of some larger effort to add tests (where's the tracking bug)?
On 2016/07/06 11:26:46, jochen wrote: > have you considered adding a unit test as opposed to browser tests with mocks? The underlying implementation of DownloadImage() is in mojo, and finally fetches the image in Blink. I'm not familiar with writing tests for mojo, but at least I think we need to set up the content shell to make DownloadImage() work. > what's the motivation for that CL? Is this part of some larger effort to add > tests (where's the tracking bug)? This CL is motivated from https://codereview.chromium.org/2015433003/, which propagates some image url to the browser which will be later fetched via DownloadImage(). palmer@ suggested to write at least one test to make sure DownloadImage() does not do evil things. So I wrote some tests in that CL to make sure we don't propagate file:/// urls to the browser, and wrote this CL to make sure DownloadImage() does not accept file:/// urls. Does it make sense? I can add a new bug entry if it's needed.
lgtm
Description was changed from ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=None ========== to ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=626281 ==========
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2113893005/#ps80001 (title: "fixed nits")
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 ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=626281 ========== to ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=626281 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=626281 ========== to ========== Adding tests for WebContent.DownloadImage() This CL adds tests to check that WebContent.DownloadImage() allows HTTP scheme but denies file scheme. BUG=626281 Committed: https://crrev.com/b94c2c360f20d9b2aef20b2efcf0ac406b9ccaba Cr-Commit-Position: refs/heads/master@{#404120} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b94c2c360f20d9b2aef20b2efcf0ac406b9ccaba Cr-Commit-Position: refs/heads/master@{#404120} |
