Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5141001:
Add a PDF test to load all the pdfs in a test directory, using the test serve... (Closed)
|
Created:
10 years, 1 month ago by jam Modified:
9 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, cbentzel+watch_chromium.org, pam+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd a PDF test to load all the pdfs in a test directory, using the test server. This tests regressions in loading and crashes.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66548
Patch Set 1 : '' #
Total comments: 6
Patch Set 2 : '' #
Total comments: 9
Patch Set 3 : '' #Patch Set 4 : fix race condition #
Messages
Total messages: 16 (0 generated)
btw this depends on moving the existing test data to the private dir first. I'll commit that before checking this in. On Tue, Nov 16, 2010 at 6:54 PM, <jam@chromium.org> wrote: > Reviewers: Nico, > > Description: > Add a PDF test to load all the pdfs in a test directory, using the test > server. > This tests regressions in loading and crashes. > > Please review this at http://codereview.chromium.org/5141001/ > > SVN Base: svn://chrome-svn/chrome/trunk/src/ > > Affected files: > M chrome/chrome_tests.gypi > M chrome/test/plugin/pdf_browsertest.cc > M net/tools/testserver/testserver.py > > >
driveby on testserver change http://codereview.chromium.org/5141001/diff/3001/chrome/test/plugin/pdf_brows... File chrome/test/plugin/pdf_browsertest.cc (right): http://codereview.chromium.org/5141001/diff/3001/chrome/test/plugin/pdf_brows... chrome/test/plugin/pdf_browsertest.cc:272: LOG(WARNING) << "PDFBrowserTest.Loading: " << filename; Local debugging? http://codereview.chromium.org/5141001/diff/3001/net/tools/testserver/testser... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/5141001/diff/3001/net/tools/testserver/testser... net/tools/testserver/testserver.py:704: if range and range.startswith('bytes='): Maybe a quick comment that this doesn't handle all valid byte range values (like open ended ones). http://codereview.chromium.org/5141001/diff/3001/net/tools/testserver/testser... net/tools/testserver/testserver.py:720: self.send_header('ETag', '\'' + file_path + '\'') Why did etag need to be added?
And you can continue code review without any further comments from me.
thanks for taking a look http://codereview.chromium.org/5141001/diff/3001/chrome/test/plugin/pdf_brows... File chrome/test/plugin/pdf_browsertest.cc (right): http://codereview.chromium.org/5141001/diff/3001/chrome/test/plugin/pdf_brows... chrome/test/plugin/pdf_browsertest.cc:272: LOG(WARNING) << "PDFBrowserTest.Loading: " << filename; On 2010/11/17 03:02:01, cbentzel wrote: > Local debugging? no, i wanted it to show which pdf it's starting, so that when it crashes/hangs on a buildbot, we can pinpoint the file http://codereview.chromium.org/5141001/diff/3001/net/tools/testserver/testser... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/5141001/diff/3001/net/tools/testserver/testser... net/tools/testserver/testserver.py:704: if range and range.startswith('bytes='): On 2010/11/17 03:02:01, cbentzel wrote: > Maybe a quick comment that this doesn't handle all valid byte range values (like > open ended ones). Done. http://codereview.chromium.org/5141001/diff/3001/net/tools/testserver/testser... net/tools/testserver/testserver.py:720: self.send_header('ETag', '\'' + file_path + '\'') On 2010/11/17 03:02:01, cbentzel wrote: > Why did etag need to be added? the cache code needs it
LG, but not having the ToWStringHack would be nice http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... File chrome/test/plugin/pdf_browsertest.cc (right): http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:47: FilePath GetPDFTestDir() { nit: const FilePath&? http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:271: std::string filename = WideToASCII(file_path.BaseName().ToWStringHack()); :-( Seems like the right thing is to add a GetFileURL() method to net/test/test_server.h and call that. It could call FilePath::value() on posix and do WideToASCII(file_path.value()) on windows. http://codereview.chromium.org/5141001/diff/13001/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/5141001/diff/13001/net/tools/testserver/testse... net/tools/testserver/testserver.py:717: self.send_response(200) should you send some error code if range and not range.startswith('bytes=')?
Drive-by with testing questions. Could you let me take another look before committing? http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... File chrome/test/plugin/pdf_browsertest.cc (right): http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:278: while (true) { This is weird. Is it caused by the test server? Can other tests be affected by the fact that NavigateToURL returns, but some parts of the request continue asynchronously?
http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... File chrome/test/plugin/pdf_browsertest.cc (right): http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:47: FilePath GetPDFTestDir() { On 2010/11/17 14:02:19, Nico wrote: > nit: const FilePath&? RVO will take care of this. I prefer the less verbose code, and either case, this isn't performance critical. http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:271: std::string filename = WideToASCII(file_path.BaseName().ToWStringHack()); On 2010/11/17 14:02:19, Nico wrote: > :-( > > Seems like the right thing is to add a GetFileURL() method to > net/test/test_server.h and call that. It could call FilePath::value() on posix > and do WideToASCII(file_path.value()) on windows. I'm not sure I understand why that's better? a GetFileURL function, if it existed, should really add file:// at the beginning, which is not what I want here (I want an http:// url so that byte range requests load). I also want just the basename so that I can print it before each load. http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:278: while (true) { On 2010/11/17 14:33:54, Paweł Hajdan Jr. wrote: > This is weird. Is it caused by the test server? see comment 2 lines below :) it's because of asynchronous loading of PDFs, where we cancel the main loader and start doing byte range requests. when we can cancel the first load, the browser will think navigation stopped. > > Can other tests be affected by the fact that NavigateToURL returns, but some > parts of the request continue asynchronously? http://codereview.chromium.org/5141001/diff/13001/net/tools/testserver/testse... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/5141001/diff/13001/net/tools/testserver/testse... net/tools/testserver/testserver.py:717: self.send_response(200) On 2010/11/17 14:02:19, Nico wrote: > should you send some error code if range and not range.startswith('bytes=')? given how primitive this server is, I don't think it's worth our time to start making it to be spec compliant... if chrome suddenly starts sending different byte range requests, the test I added will fail and it'll be easy to track down.
On Wed, Nov 17, 2010 at 5:53 PM, <jam@chromium.org> wrote: > > http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... > File chrome/test/plugin/pdf_browsertest.cc (right): > > http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... > chrome/test/plugin/pdf_browsertest.cc:47: FilePath GetPDFTestDir() { > On 2010/11/17 14:02:19, Nico wrote: >> >> nit: const FilePath&? > > RVO will take care of this. I prefer the less verbose code, and either > case, this isn't performance critical. > > http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... > chrome/test/plugin/pdf_browsertest.cc:271: std::string filename = > WideToASCII(file_path.BaseName().ToWStringHack()); > On 2010/11/17 14:02:19, Nico wrote: >> >> :-( > >> Seems like the right thing is to add a GetFileURL() method to >> net/test/test_server.h and call that. It could call FilePath::value() > > on posix >> >> and do WideToASCII(file_path.value()) on windows. > > I'm not sure I understand why that's better? a GetFileURL function, if > it existed, should really add file:// at the beginning, which is not > what I want here (I want an http:// url so that byte range requests > load). I also want just the basename so that I can print it before each > load. As far as I understand, ToWStringHack() is supposed to be phased out. However, converting a FilePath to ascii is somewhat involved, so I thought it'd be nice if it was in one location. But you're right, you don't want file:// urls, so it's not a good idea. So go ahead, and I'll talk to the FilePath authors to get a function added that enables this use case without having to call ToWStringHack. > > http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... > chrome/test/plugin/pdf_browsertest.cc:278: while (true) { > On 2010/11/17 14:33:54, Paweł Hajdan Jr. wrote: >> >> This is weird. Is it caused by the test server? > > see comment 2 lines below :) it's because of asynchronous loading of > PDFs, where we cancel the main loader and start doing byte range > requests. when we can cancel the first load, the browser will think > navigation stopped. > >> Can other tests be affected by the fact that NavigateToURL returns, > > but some >> >> parts of the request continue asynchronously? > > http://codereview.chromium.org/5141001/diff/13001/net/tools/testserver/testse... > File net/tools/testserver/testserver.py (right): > > http://codereview.chromium.org/5141001/diff/13001/net/tools/testserver/testse... > net/tools/testserver/testserver.py:717: self.send_response(200) > On 2010/11/17 14:02:19, Nico wrote: >> >> should you send some error code if range and not > > range.startswith('bytes=')? > > given how primitive this server is, I don't think it's worth our time to > start making it to be spec compliant... if chrome suddenly starts > sending different byte range requests, the test I added will fail and > it'll be easy to track down. > > http://codereview.chromium.org/5141001/ >
http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... File chrome/test/plugin/pdf_browsertest.cc (right): http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... chrome/test/plugin/pdf_browsertest.cc:278: while (true) { On 2010/11/17 16:53:15, John Abd-El-Malek wrote: > see comment 2 lines below :) it's because of asynchronous loading of PDFs, > where we cancel the main loader and start doing byte range requests. when we > can cancel the first load, the browser will think navigation stopped. I see. Can we wait for multiple navigations? There is also a method called ui_test_utils::NavigateToURLBlockUntilNavigations complete, which can wait for more than one navigation. Using it would be much simpler than this loop.
On 2010/11/17 17:11:52, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... > File chrome/test/plugin/pdf_browsertest.cc (right): > > http://codereview.chromium.org/5141001/diff/13001/chrome/test/plugin/pdf_brow... > chrome/test/plugin/pdf_browsertest.cc:278: while (true) { > On 2010/11/17 16:53:15, John Abd-El-Malek wrote: > > see comment 2 lines below :) it's because of asynchronous loading of PDFs, > > where we cancel the main loader and start doing byte range requests. when we > > can cancel the first load, the browser will think navigation stopped. > > I see. Can we wait for multiple navigations? There is also a method called > ui_test_utils::NavigateToURLBlockUntilNavigations complete, which can wait for > more than one navigation. Using it would be much simpler than this loop. The plugin makes the decision on whether to load a file asynchronously or not based on the file header. The test, or browser for that matter, doesn't know what will happen, so it can't wait a preset number of navigations.
I see. Thanks for explaining. Removing myself from reviewers.
I've made a small modification to out_of_proc_test_runner.cc to allow a test to override the default timeout. The issue is that the pdf loading test loads a lot of pdfs, so as we add more and more files, which is good!, it'll become slower. It's kind of the layout tests, but for pdfs. So the normal test timeout won't suffice here. I could add a new test for each file, but this seems like busy work. People adding a test might also forget to add it. So I added a way of allowing a test to increase its timeout. I've made the pdf loading test go to a minute for now.
I've also just fixed a race condition, where the LOAD_STOP notification might come during the nested message loop for the JS call.
Carlos: can you take a look please, it turns out Nico is away. On Wed, Nov 17, 2010 at 12:58 PM, <jam@chromium.org> wrote: > I've also just fixed a race condition, where the LOAD_STOP notification > might > come during the nested message loop for the JS call. > > > http://codereview.chromium.org/5141001/ >
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
