Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349753003:
Improve linearized pdf load/show time. (Closed)
DescriptionImprove linearized pdf load/show time.
Reduce Pdf Plugin's count of reconnects.
Add tests for PDFPlugin DocumentLoader.
DocumentLoader was splitted into separate components, and missing tests was added for them.
The main ideas in this CL are:
1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position.
2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind)
3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing).
4) Fill this logic by tests.
Example URL:
http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509ec1ef2b31.pdf
Comparison of changes:
https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing
Committed: https://crrev.com/7fd7423cdee0dba84faf480d10dd66dcb57110d9
Cr-Commit-Position: refs/heads/master@{#427752}
Patch Set 1 #Patch Set 2 : remove useless code. #
Total comments: 31
Patch Set 3 : Fix compilation and review issues. #Patch Set 4 : Fix compilation and review issues. #Patch Set 5 : Fix compilation on linux. #Patch Set 6 : Fix heap use after free. #
Total comments: 14
Patch Set 7 : Fix review issues. #Patch Set 8 : Some improvement. #
Total comments: 7
Patch Set 9 : Update tests.. #Patch Set 10 : Update tests.. #
Total comments: 12
Patch Set 11 : Fix review issues. #
Total comments: 102
Patch Set 12 : fix review issues. #
Total comments: 20
Patch Set 13 : fix review issues. #
Total comments: 29
Patch Set 14 : fix review issues and compilation. #
Total comments: 6
Patch Set 15 : fix review issues. #
Total comments: 2
Patch Set 16 : fix compilation on linux. #Patch Set 17 : fix compilation on linux. #Patch Set 18 : rebase #
Messages
Total messages: 131 (65 generated)
Description was changed from ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ========== to ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ==========
art-snake@yandex-team.ru changed reviewers: + jochen@chromium.org, rsesek@chromium.org
The CQ bit was checked by art-snake@yandex-team.ru 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
art-snake@yandex-team.ru changed reviewers: + raymes@chromium.org
Anybody?
On 2016/09/20 15:19:20, snake wrote: > Anybody? I don't think you Pubish+Mailed this CL out for review, since I got the initial CL message just now. (Just adding reviewers without sending mail won't get people to see it). That said, I'm not an appropriate reviewer for this.
rsesek@chromium.org changed reviewers: - rsesek@chromium.org
rsesek@chromium.org changed reviewers: + thestig@chromium.org
+thestig for PDF, and -rsesek I think you'll want to split this up into smaller, more easily-reviewed parts, though. +2841,-743 is a huge diffstat.
On 2016/09/20 19:10:25, Robert Sesek wrote: > +thestig for PDF, and -rsesek > > I think you'll want to split this up into smaller, more easily-reviewed parts, > though. +2841,-743 is a huge diffstat. And ~2000 from its are tests, which was not exists before.
thestig@chromium.org changed reviewers: + spelchat@chromium.org
+spelchat if interested.
On 2016/09/21 22:20:11, snake wrote: Is that interesting for anybody?
Description was changed from ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ========== to ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ==========
can you please send a design doc first (https://www.chromium.org/developers/new-features)?
It might be easier to review if you do the chrome_pdf::DocumentLoader splitting in 1 CL, and the improvements in a separate CL.
On 2016/09/28 23:21:33, Lei Zhang wrote: > It might be easier to review if you do the chrome_pdf::DocumentLoader splitting > in 1 CL, and the improvements in a separate CL. I can not do this, because splitting is part of improvements. I did change the logic while split DocumentLoader.
The CQ bit was checked by thestig@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/09/30 14:36:28, snake wrote: > On 2016/09/28 23:21:33, Lei Zhang wrote: > > It might be easier to review if you do the chrome_pdf::DocumentLoader > splitting > > in 1 CL, and the improvements in a separate CL. > > I can not do this, because splitting is part of improvements. I did change the > logic while split DocumentLoader. Are there really no opportunities to make changes in an incremental manner? For instance, how about URLLoaderWrapperImpl? Some of the code comes out of document_loader.cc. Can you create a version of URLLoaderWrapperImpl that works with the existing document_loader.cc first? Then evolve URLLoaderWrapperImpl into the one you have here, rather than make one giant leap?
I'd be happy to run your patch sets through CQ. I'm not sure what build environment you have locally, but as you can see from the patch set 2 CQ dry run, Clang compiler + chromium-style plugin does not like the code as written. https://codereview.chromium.org/2349753003/diff/20001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/document_loader.h#n... pdf/document_loader.h:12: #include <map> Not needed? https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:842: PDFiumEngine* p_this = static_cast<PDFiumEngine*>(param); s/p_this/engine/ ? https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:993: URLLoaderWrapperImpl* loader_wrapper_ptr = Why bother with the raw pointer? Just: auto loader_wrapper = base::MakeUnique< URLLoaderWrapperImpl>(GetPluginInstance(), loader); loader_wrapper->SetResponseHeaders(headers_); ... https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:997: doc_loader_.reset(new DocumentLoader(this)); More base::MakeUnique? https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1096: if (!doc_) { In general, if (!foo) { bar(); } else { qux(); } may make more sense when flipped back to if (foo) { qux(); } else { bar(); }; https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1104: FinishLoadingDocument(); Or how about just: if (doc_) { FinishLoadingDocument(); return; } .... https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:733: bool should_process_pending_request_complete_ = true; Comment to explain what this does? Maybe "process_when_pending_request_complete_" is easier to understand. https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set.h File pdf/range_set.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set.h#newcode15 pdf/range_set.h:15: class RangeSet { In general, it would be nice to have a description of what a class does. https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set.h#newcode56 pdf/range_set.h:56: int Size() const { return static_cast<int>(ranges_.size()); } Can this just return size_t and not cast? https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set_unittest.cc File pdf/range_set_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set_unittest.... pdf/range_set_unittest.cc:14: EXPECT_EQ(range_set.ToString(), "{}"); In general, the parameters here are EXPECT_EQ(expected, actual); e.g. EXPECT_EQ(the_answer, 42); with the_answer = 19 will print out: expected: the_answer, which is 19 actual: 42 https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set_unittest.... pdf/range_set_unittest.cc:50: EXPECT_TRUE(range_set_1 == RangeSet(gfx::Range(0, 60))); In general, can EXPECT_TRUE(expected == actual) be written as EXPECT_EQ(expected, actual) ? https://codereview.chromium.org/2349753003/diff/20001/pdf/run_all_unittests.cc File pdf/run_all_unittests.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/run_all_unittests.c... pdf/run_all_unittests.cc:21: int res = base::LaunchUnitTests( Why bother with the variable? https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h#newcode15 pdf/timer.h:15: class Timer { I only see one class inherit from Timer. Is it going to get reused and/or is it important enough that you want to keep it separate? https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h#newcode35 pdf/timer.h:35: TestTimerRunner(); I don't see this class getting instantiated anywhere. https://codereview.chromium.org/2349753003/diff/20001/pdf/url_loader_wrapper_... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/url_loader_wrapper_... pdf/url_loader_wrapper_impl.cc:37: base::snprintf(buf, kBufSize, "Range: bytes=%d-%d", position, Use base::StringPrintf() and don't worry about a fixed length buffer?
https://codereview.chromium.org/2349753003/diff/20001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/document_loader.h#n... pdf/document_loader.h:12: #include <map> On 2016/10/05 07:18:07, Lei Zhang wrote: > Not needed? Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:842: PDFiumEngine* p_this = static_cast<PDFiumEngine*>(param); On 2016/10/05 07:18:08, Lei Zhang wrote: > s/p_this/engine/ ? Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:993: URLLoaderWrapperImpl* loader_wrapper_ptr = On 2016/10/05 07:18:07, Lei Zhang wrote: > Why bother with the raw pointer? Just: > > auto loader_wrapper = base::MakeUnique< > URLLoaderWrapperImpl>(GetPluginInstance(), loader); > loader_wrapper->SetResponseHeaders(headers_); > ... Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:997: doc_loader_.reset(new DocumentLoader(this)); On 2016/10/05 07:18:08, Lei Zhang wrote: > More base::MakeUnique? Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1096: if (!doc_) { On 2016/10/05 07:18:08, Lei Zhang wrote: > In general, if (!foo) { bar(); } else { qux(); } may make more sense when > flipped back to if (foo) { qux(); } else { bar(); }; Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1104: FinishLoadingDocument(); On 2016/10/05 07:18:07, Lei Zhang wrote: > Or how about just: > > if (doc_) { > FinishLoadingDocument(); > return; > } > > .... Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:733: bool should_process_pending_request_complete_ = true; On 2016/10/05 07:18:08, Lei Zhang wrote: > Comment to explain what this does? Maybe > "process_when_pending_request_complete_" is easier to understand. Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set.h File pdf/range_set.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set.h#newcode15 pdf/range_set.h:15: class RangeSet { On 2016/10/05 07:18:08, Lei Zhang wrote: > In general, it would be nice to have a description of what a class does. Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set.h#newcode56 pdf/range_set.h:56: int Size() const { return static_cast<int>(ranges_.size()); } On 2016/10/05 07:18:08, Lei Zhang wrote: > Can this just return size_t and not cast? Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set_unittest.cc File pdf/range_set_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set_unittest.... pdf/range_set_unittest.cc:14: EXPECT_EQ(range_set.ToString(), "{}"); On 2016/10/05 07:18:08, Lei Zhang wrote: > In general, the parameters here are EXPECT_EQ(expected, actual); > > e.g. EXPECT_EQ(the_answer, 42); with the_answer = 19 will print out: > expected: the_answer, which is 19 > actual: 42 Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/range_set_unittest.... pdf/range_set_unittest.cc:50: EXPECT_TRUE(range_set_1 == RangeSet(gfx::Range(0, 60))); On 2016/10/05 07:18:08, Lei Zhang wrote: > In general, can EXPECT_TRUE(expected == actual) be written as > EXPECT_EQ(expected, actual) ? Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/run_all_unittests.cc File pdf/run_all_unittests.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/run_all_unittests.c... pdf/run_all_unittests.cc:21: int res = base::LaunchUnitTests( On 2016/10/05 07:18:08, Lei Zhang wrote: > Why bother with the variable? Done. https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h#newcode15 pdf/timer.h:15: class Timer { On 2016/10/05 07:18:08, Lei Zhang wrote: > I only see one class inherit from Timer. Is it going to get reused and/or is it > important enough that you want to keep it separate? I want reuse it in next CLs for PDF plugin. https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h#newcode35 pdf/timer.h:35: TestTimerRunner(); On 2016/10/05 07:18:08, Lei Zhang wrote: > I don't see this class getting instantiated anywhere. This will be needs for tests in next CLs. https://codereview.chromium.org/2349753003/diff/20001/pdf/url_loader_wrapper_... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/url_loader_wrapper_... pdf/url_loader_wrapper_impl.cc:37: base::snprintf(buf, kBufSize, "Range: bytes=%d-%d", position, On 2016/10/05 07:18:08, Lei Zhang wrote: > Use base::StringPrintf() and don't worry about a fixed length buffer? Done.
On 2016/10/05 07:15:32, Lei Zhang wrote: > On 2016/09/30 14:36:28, snake wrote: > > On 2016/09/28 23:21:33, Lei Zhang wrote: > > > It might be easier to review if you do the chrome_pdf::DocumentLoader > > splitting > > > in 1 CL, and the improvements in a separate CL. > > > > I can not do this, because splitting is part of improvements. I did change the > > logic while split DocumentLoader. > > Are there really no opportunities to make changes in an incremental manner? For > instance, how about URLLoaderWrapperImpl? Some of the code comes out of > document_loader.cc. Can you create a version of URLLoaderWrapperImpl that works > with the existing document_loader.cc first? Then evolve URLLoaderWrapperImpl > into the one you have here, rather than make one giant leap? We need tests to ensure correct changes. But current implementation of document loader have not tests. As result I should be write much of code for tests for current implementation (~1000 lines (JFYI: this is half of this CL)), which will be fully changed in this CL.
The CQ bit was checked by thestig@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: linux_chromium_asan_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 art-snake@yandex-team.ru 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/10/05 14:21:36, snake wrote: > On 2016/10/05 07:15:32, Lei Zhang wrote: > > On 2016/09/30 14:36:28, snake wrote: > > > On 2016/09/28 23:21:33, Lei Zhang wrote: > > > > It might be easier to review if you do the chrome_pdf::DocumentLoader > > > splitting > > > > in 1 CL, and the improvements in a separate CL. > > > > > > I can not do this, because splitting is part of improvements. I did change > the > > > logic while split DocumentLoader. > > > > Are there really no opportunities to make changes in an incremental manner? > For > > instance, how about URLLoaderWrapperImpl? Some of the code comes out of > > document_loader.cc. Can you create a version of URLLoaderWrapperImpl that > works > > with the existing document_loader.cc first? Then evolve URLLoaderWrapperImpl > > into the one you have here, rather than make one giant leap? > > We need tests to ensure correct changes. But current implementation of document > loader have not tests. As result I should be write much of code for tests for > current implementation (~1000 lines (JFYI: this is half of this CL)), which will > be fully changed in this CL. Compilations should be fixed. Can you run TryBots?
The CQ bit was checked by thestig@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/10/06 12:34:14, snake wrote: > Compilations should be fixed. Can you run TryBots? Done. Please note the linux_chromium_asan_rel_ng bot found some problems with this code.
On 2016/10/06 20:39:00, Lei Zhang wrote: > On 2016/10/06 12:34:14, snake wrote: > > Compilations should be fixed. Can you run TryBots? > > Done. Please note the linux_chromium_asan_rel_ng bot found some problems with > this code. Fixed
On 2016/10/07 10:03:24, snake wrote: > On 2016/10/06 20:39:00, Lei Zhang wrote: > > On 2016/10/06 12:34:14, snake wrote: > > > Compilations should be fixed. Can you run TryBots? > > > > Done. Please note the linux_chromium_asan_rel_ng bot found some problems with > > this code. > > Fixed What else should I do to commit this CL?
The CQ bit was checked by thestig@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...
On 2016/10/11 15:34:25, snake wrote: > What else should I do to commit this CL? It's great that you have a video to demonstrate the improvement, but as jochen@ mentioned, it may be helpful to explain how your design works.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
I've just started looking at this. I think I have a better idea of what the code does and should have more comments tomorrow. Note that I don't usually work on Chrome so I won't be able to comment much on style and Chrome best practices. FWIW the use of a ui class (ui/gfx/range/range.h) seems odd in chunk_stream. That said the Range class seems completely generic, so arguably it shouldn't be in gfx... I'll assume this is fine. https://codereview.chromium.org/2349753003/diff/100001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:71: (eof_pos_ && eof_pos_ < range.end())) I may be wrong (other reviewers, please correct me), but I think the preferred style is eof_pos_ != 0 (i.e. don't use the implicit conversion from integer types to bool). https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:264: return ReadComplete(); Can you restore the comment that was lost here? It's not obvious here that we're handling the error case correctly. E.g. // An error occurred. The renderer will detect that // we're missing data and will display a message. https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; Any reason why this is public?
I've just started looking at this. I think I have a better idea of what the code does and should have more comments tomorrow. Note that I don't usually work on Chrome so I won't be able to comment much on style and Chrome best practices. FWIW the use of a ui class (ui/gfx/range/range.h) seems odd in chunk_stream. That said the Range class seems completely generic, so arguably it shouldn't be in gfx... I'll assume this is fine. https://codereview.chromium.org/2349753003/diff/100001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:71: (eof_pos_ && eof_pos_ < range.end())) I may be wrong (other reviewers, please correct me), but I think the preferred style is eof_pos_ != 0 (i.e. don't use the implicit conversion from integer types to bool). https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:264: return ReadComplete(); Can you restore the comment that was lost here? It's not obvious here that we're handling the error case correctly. E.g. // An error occurred. The renderer will detect that // we're missing data and will display a message. https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; Any reason why this is public?
https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:68: if (type == "content-type: text/plain") { How can this be "content-type: text/plain"? IIUC this will be "text/plain". https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:182: chunk_stream_.total_chunks_count())); Can you split this in smaller chunks? It's a little hard to read. e.g. uint32_t range_start = pending_requests_.IsEmpty() ? 0 : pending_requests_.First().start() RangeSet next_pending_requests(gfx::Range(range_start, chunk_stream_.total_chunks_count())); https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:185: gfx::Range pending_request = next_pending_requests.First(); I would add DCHECK(pending_request.Contains(pending_requests_.First())); for documentation https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:189: // reading. That comment confused me slightly. pending_request has already been enlarged on the right side. If you're missing chunks [10,19] and the renderer requested [12,15], (i.e. pending_requests_.First() is [12,15]), then I think pending_request is [12,19]. This if branch also extends it toward the left if it's still smaller than kChunkCloseDistance. Is this correct? In particular does this mean that if the renderer requests [100,120] and you're missing [2,10000] then the next request that will go through will be for chunks [100,10000]. That request won't be cancelled if the renderer suddenly needs [2,5]? Can this unbounded expansion toward the right be a problem?
https://codereview.chromium.org/2349753003/diff/100001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:71: (eof_pos_ && eof_pos_ < range.end())) On 2016/10/12 00:35:31, spelchat wrote: > I may be wrong (other reviewers, please correct me), but I think the preferred > style is eof_pos_ != 0 (i.e. don't use the implicit conversion from integer > types to bool). Done https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:68: if (type == "content-type: text/plain") { On 2016/10/13 18:26:54, spelchat wrote: > How can this be "content-type: text/plain"? IIUC this will be "text/plain". Fixed https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:182: chunk_stream_.total_chunks_count())); On 2016/10/13 18:26:54, spelchat wrote: > Can you split this in smaller chunks? It's a little hard to read. > > e.g. > > uint32_t range_start = pending_requests_.IsEmpty() ? 0 : > pending_requests_.First().start() > RangeSet next_pending_requests(gfx::Range(range_start, > chunk_stream_.total_chunks_count())); Done https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:185: gfx::Range pending_request = next_pending_requests.First(); On 2016/10/13 18:26:54, spelchat wrote: > I would add > > DCHECK(pending_request.Contains(pending_requests_.First())); > > for documentation pending_requests_ can be empty, but pending_request should not be empty anywhere. if pending_requests_ are empty, we are using gaps as pending_request. https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:189: // reading. On 2016/10/13 18:26:54, spelchat wrote: > That comment confused me slightly. > > pending_request has already been enlarged on the right side. > > If you're missing chunks [10,19] and the renderer requested [12,15], (i.e. > pending_requests_.First() is [12,15]), then I think pending_request is [12,19]. > This if branch also extends it toward the left if it's still smaller than > kChunkCloseDistance. This happens, only in case when requested data is beside to the and of non filled chunks (it is beside to end of last gap). For example: document have 100 chunks. kChunkCloseDistance = 10 Do request [97,98]-> Start Loading [89,99] // this case. Load Data [89,99] // Enlarged to left and right both Do request [50,55]-> Start Loading [50,89] // not this case (length > kChunkCloseDistance), Load Data [50,55] // Enlarged only to right. Do request [86,87]-> Start Loading [78,88] // this case. Load Data [78,88] // Enlarged to left and right both Do request [46,47]-> Start Loading [46,49] // not this case (next_pending_requests.Size()>1 i.e. next_pending_requests=[46,49],[56,77] ), Load Data [46,49]// Enlarged only to right. ... > > Is this correct? In particular does this mean that if the renderer requests > [100,120] and you're missing [2,10000] then the next request that will go > through will be for chunks [100,10000]. Correct. This request will be started, but can be canceled, and renderer will be notifyed when received exactly [100,120] (not [100,10000]). > That request won't be cancelled if the > renderer suddenly needs [2,5]? Incorrect. If renderer initiate new pending request, the current request will be canceled after download current chunk. (See DocumentLoader::ShouldCancelLoading) > > Can this unbounded expansion toward the right be a problem? > No. Сonversely it is main part of optimization, which decrease the reconnection count. For linearized documents we will read data linearly forward ( in common case), and will be still use same connection (Ideally browser initiated connection). For non linearized documents, main optimization is expansion toward the left, when we request data in back order. ( But i will enable async parsing for non linearized documents in next CLs, because needs more changes for it (this will be true fix for BUG=59400)) https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.cc... pdf/document_loader.cc:264: return ReadComplete(); On 2016/10/12 00:35:31, spelchat wrote: > Can you restore the comment that was lost here? It's not obvious here that we're > handling the error case correctly. > > E.g. > // An error occurred. The renderer will detect that > // we're missing data and will display a message. Done https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/100001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; On 2016/10/12 00:35:31, spelchat wrote: > Any reason why this is public? It is used in tests. More easy, that declare "friend test", and with same safe, beacause it is static constant.
https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader.cc... pdf/document_loader.cc:180: RangeSet candidates_for_request( readability is fixed. renamed: next_pending_requests --> candidates_for_request pending_request --> next_request
https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... pdf/document_loader_unittest.cc:234: } This code desperately needed tests, thanks for that. Can you break that test in individual tests for each scope you created here? Ideally giving them a name indicating what they test. It's a little hard to keep track of the state and see what each scope is testing. e.g. TEST_F(DocumentLoaderTest, PartialLoadingDisabledNoAcceptRangeBytes) { { TestClient client; client.full_page_loader_data()->content_encoded = false; client.full_page_loader_data()->accept_ranges_bytes = false; client.full_page_loader_data()->content_length = 1 * 1024 * 1024; DocumentLoader loader(&client); loader.Init(client.CreateFullPageLoader(), "http://url.com"); loader.RequestData(1000000, 1); EXPECT_FALSE(loader.is_partial_loader_active()); // Always send initial data from FullPageLoader. client.full_page_loader_data()->CallReadCallback( DocumentLoader::kDefaultRequestSize); EXPECT_FALSE(loader.is_partial_loader_active()); } You can always use helper functions to remove the boilerplate if you want. https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... pdf/document_loader_unittest.cc:292: // While we have not requests, we shold not start partial loading. not requests -> no requests shold -> should https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... pdf/document_loader_unittest.cc:321: // While we have not requests, we shold not start partial loading. not requests -> no requests shold -> should
https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... pdf/document_loader_unittest.cc:234: } On 2016/10/14 23:43:37, spelchat wrote: > This code desperately needed tests, thanks for that. > > Can you break that test in individual tests for each scope you created here? > Ideally giving them a name indicating what they test. It's a little hard to keep > track of the state and see what each scope is testing. > > e.g. > > TEST_F(DocumentLoaderTest, PartialLoadingDisabledNoAcceptRangeBytes) { > { > TestClient client; > client.full_page_loader_data()->content_encoded = false; > client.full_page_loader_data()->accept_ranges_bytes = false; > client.full_page_loader_data()->content_length = 1 * 1024 * 1024; > DocumentLoader loader(&client); > loader.Init(client.CreateFullPageLoader(), "http://url.com"); > loader.RequestData(1000000, 1); > EXPECT_FALSE(loader.is_partial_loader_active()); > // Always send initial data from FullPageLoader. > client.full_page_loader_data()->CallReadCallback( > DocumentLoader::kDefaultRequestSize); > EXPECT_FALSE(loader.is_partial_loader_active()); > } > > You can always use helper functions to remove the boilerplate if you want. Done. https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... pdf/document_loader_unittest.cc:292: // While we have not requests, we shold not start partial loading. On 2016/10/14 23:43:37, spelchat wrote: > not requests -> no requests > > shold -> should Done. https://codereview.chromium.org/2349753003/diff/140001/pdf/document_loader_un... pdf/document_loader_unittest.cc:321: // While we have not requests, we shold not start partial loading. On 2016/10/14 23:43:37, spelchat wrote: > not requests -> no requests > > shold -> should Done.
Just minor grammar nits. Overall this lgtm. It would be nice to have another pair of eyes look at this carefully. This code has been very brittle in the past and this change is huge. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:279: TEST_F(DocumentLoaderTest, PartialLoadingRealyDisabledRequestFromMiddle) { Realy -> Really https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:515: // Current request should continue loading, because not other requests queued. because not other requests queued. -> because no other request queued. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:518: // Requests queue is processed only on data receving. on receiving data. Same below. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:521: // new requset with in close distance from current loading. Loading do not "New request within close distance from the one currently loading. Loading isn't restarted." Same below. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:730: // Partial loading should not started for already available data. should not have started https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:841: // partial loader is used to load whole page, like full page loader. Partial loader is used to load the whole page...
https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:279: TEST_F(DocumentLoaderTest, PartialLoadingRealyDisabledRequestFromMiddle) { On 2016/10/18 16:12:44, spelchat wrote: > Realy -> Really Done. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:515: // Current request should continue loading, because not other requests queued. On 2016/10/18 16:12:44, spelchat wrote: > because not other requests queued. > -> > because no other request queued. Done. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:518: // Requests queue is processed only on data receving. On 2016/10/18 16:12:44, spelchat wrote: > on receiving data. > > Same below. Done. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:521: // new requset with in close distance from current loading. Loading do not On 2016/10/18 16:12:44, spelchat wrote: > "New request within close distance from the one currently loading. Loading isn't > restarted." > > Same below. Done. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:730: // Partial loading should not started for already available data. On 2016/10/18 16:12:44, spelchat wrote: > should not have started Done. https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... pdf/document_loader_unittest.cc:841: // partial loader is used to load whole page, like full page loader. On 2016/10/18 16:12:44, spelchat wrote: > Partial loader is used to load the whole page... Done.
The CQ bit was checked by art-snake@yandex-team.ru 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by art-snake@yandex-team.ru
The CQ bit was checked by art-snake@yandex-team.ru 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/10/18 17:00:58, snake wrote: > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > File pdf/document_loader_unittest.cc (right): > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > pdf/document_loader_unittest.cc:279: TEST_F(DocumentLoaderTest, > PartialLoadingRealyDisabledRequestFromMiddle) { > On 2016/10/18 16:12:44, spelchat wrote: > > Realy -> Really > > Done. > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > pdf/document_loader_unittest.cc:515: // Current request should continue loading, > because not other requests queued. > On 2016/10/18 16:12:44, spelchat wrote: > > because not other requests queued. > > -> > > because no other request queued. > > Done. > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > pdf/document_loader_unittest.cc:518: // Requests queue is processed only on data > receving. > On 2016/10/18 16:12:44, spelchat wrote: > > on receiving data. > > > > Same below. > > Done. > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > pdf/document_loader_unittest.cc:521: // new requset with in close distance from > current loading. Loading do not > On 2016/10/18 16:12:44, spelchat wrote: > > "New request within close distance from the one currently loading. Loading > isn't > > restarted." > > > > Same below. > > Done. > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > pdf/document_loader_unittest.cc:730: // Partial loading should not started for > already available data. > On 2016/10/18 16:12:44, spelchat wrote: > > should not have started > > Done. > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > pdf/document_loader_unittest.cc:841: // partial loader is used to load whole > page, like full page loader. > On 2016/10/18 16:12:44, spelchat wrote: > > Partial loader is used to load the whole page... > > Done. I did try to run "try bots" with dry run, but see error message: " CQ rejected the patch Verification failed: No L-G-T-M from a valid reviewer yet. " :(
On 2016/10/18 17:12:13, snake wrote: > On 2016/10/18 17:00:58, snake wrote: > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > File pdf/document_loader_unittest.cc (right): > > > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > pdf/document_loader_unittest.cc:279: TEST_F(DocumentLoaderTest, > > PartialLoadingRealyDisabledRequestFromMiddle) { > > On 2016/10/18 16:12:44, spelchat wrote: > > > Realy -> Really > > > > Done. > > > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > pdf/document_loader_unittest.cc:515: // Current request should continue > loading, > > because not other requests queued. > > On 2016/10/18 16:12:44, spelchat wrote: > > > because not other requests queued. > > > -> > > > because no other request queued. > > > > Done. > > > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > pdf/document_loader_unittest.cc:518: // Requests queue is processed only on > data > > receving. > > On 2016/10/18 16:12:44, spelchat wrote: > > > on receiving data. > > > > > > Same below. > > > > Done. > > > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > pdf/document_loader_unittest.cc:521: // new requset with in close distance > from > > current loading. Loading do not > > On 2016/10/18 16:12:44, spelchat wrote: > > > "New request within close distance from the one currently loading. Loading > > isn't > > > restarted." > > > > > > Same below. > > > > Done. > > > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > pdf/document_loader_unittest.cc:730: // Partial loading should not started for > > already available data. > > On 2016/10/18 16:12:44, spelchat wrote: > > > should not have started > > > > Done. > > > > > https://codereview.chromium.org/2349753003/diff/180001/pdf/document_loader_un... > > pdf/document_loader_unittest.cc:841: // partial loader is used to load whole > > page, like full page loader. > > On 2016/10/18 16:12:44, spelchat wrote: > > > Partial loader is used to load the whole page... > > > > Done. > > I did try to run "try bots" with dry run, but see error message: > " > CQ rejected the patch > Verification failed: No L-G-T-M from a valid reviewer yet. > " > :( Yes, I'm not a Chromium committer (I'm reviewer on this CL because made changes to document_loader.cc before, I don't normally work on Chrome), so my lgtm has no real value. You'll have to wait for a Chromium committer to lgtm too.
Lei Zhang, The main ideas in this CL are: 1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position. 2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind) 3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing). 4) Fill this logic by tests.
> Yes, I'm not a Chromium committer (I'm reviewer on this CL because made changes > to document_loader.cc before, I don't normally work on Chrome), so my lgtm has > no real value. You'll have to wait for a Chromium committer to lgtm too. Thank you.
On 2016/10/18 17:33:08, snake wrote: > Lei Zhang, > > The main ideas in this CL are: > > 1) Do not reset browser initiated connection at start (includes case when we can > use range requests), if we request data near current downloading position. > 2) Request as much data as we can on each request, and continue loading data > using current range request. (like tape rewind) > 3) Isolate RangeRequest logic into DocumentLoader. Method > OnPendingRequestComplete is called, when we receive requested data (main > connection, or Range connection). (like tape playing without rewing). > 4) Fill this logic by tests. Ping.
On 2016/10/20 11:38:19, snake wrote: > Ping. Pong. I will look at this CL today/tomorrow.
On 2016/10/18 17:33:08, snake wrote: > Lei Zhang, > > The main ideas in this CL are: > > 1) Do not reset browser initiated connection at start (includes case when we can > use range requests), if we request data near current downloading position. > 2) Request as much data as we can on each request, and continue loading data > using current range request. (like tape rewind) > 3) Isolate RangeRequest logic into DocumentLoader. Method > OnPendingRequestComplete is called, when we receive requested data (main > connection, or Range connection). (like tape playing without rewing). > 4) Fill this logic by tests. Can you put that in the CL description? Here, it is buried among all the messages.
https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/20001/pdf/timer.h#newcode35 pdf/timer.h:35: TestTimerRunner(); On 2016/10/05 14:14:10, snake wrote: > On 2016/10/05 07:18:08, Lei Zhang wrote: > > I don't see this class getting instantiated anywhere. > > This will be needs for tests in next CLs. So can you add this in the next CL, when it is needed? Also, do you really need a new Timer class? There already exists base::Timer and base::MockTimer. https://codereview.chromium.org/2349753003/diff/200001/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/BUILD.gn#newcode7 pdf/BUILD.gn:7: import("//testing/test.gni") nit: alphabetical order https://codereview.chromium.org/2349753003/diff/200001/pdf/BUILD.gn#newcode101 pdf/BUILD.gn:101: "//ui/gfx", only "//ui/gfx/range" is needed? https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:22: template <size_t N> Is there any way to avoid making this a template class? https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:31: void SetChunkData(int chunk_index, std::unique_ptr<ChunkData> data) { Should |chunk_index| be unsigned? https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream_unitt... File pdf/chunk_stream_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream_unitt... pdf/chunk_stream_unittest.cc:16: return std::unique_ptr<TestChunkStream::ChunkData>( nit: base::MakeUnique ? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:21: const int kChunkCloseDistance = 10; Can you document what this is / why this value was chose? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:80: loader_ = std::move(loader); Should |url_| be set here instead of above? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:119: return chunk_stream_.ReadData(gfx::Range(position, position + size), buf); Can "position + size" overflow? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:239: int start_pos, end_pos; nit: 1 declaration per line please. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:267: } else if (result == 0) { nit: no else if after a return. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:292: count_of_bytes_received_ += input_size; Can this overflow? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:321: if (chunk_saved) { if (!chunk_saved) return false; // do things return true; https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:359: return chunk_stream_.filled_chunks_count() * 1. / Can you static_cast to a float instead of "* 1." ? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; Can this be a constexpr instead? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h#... pdf/document_loader.h:76: typedef ChunkStream<kDefaultRequestSize> DataStream; nit: use "using" keyword instead. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:23: namespace { nit: blank line after the beginning of the namespace, and before the end. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:26: struct LoaderData { This need to be a class. https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:74: pp::CompletionCallback did_open_callback_; Does SetOpenCallback() always pass in a |open_callback| that has ope-n_callback.is_null() == false? If yes, then can you get rid of |wait_open_| and use |did_open_callback_|'s is_null() check instead? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:77: char* buffer_ = nullptr; Can this be a std::vector<char> instead? And std::vector all around, instead of char* + size value. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:134: }; Please use DISALLOW_COPY_AND_ASSIGN(ClassName) liberally in classes. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:139: ~TestClient() override {} nit: blank line after https://codereview.chromium.org/2349753003/diff/200001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/pdf_engine.h#newco... pdf/pdf_engine.h:186: virtual void CancelBrowserDownload() = 0; Document please. https://codereview.chromium.org/2349753003/diff/200001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1013: result.reset(new URLLoaderWrapperImpl(GetPluginInstance(), Can you assign instead of using reset(), perhaps with base::MakeUnique, or base::WrapUnique() ? Or better yet, can you just return directly? Maybe you need a "return std::move(...)"; https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.cc File pdf/range_set.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.cc#newco... pdf/range_set.cc:117: --start; // start now points to the key equal or lower than nit: if (cond) { // long comments // that may wrap do_something; } https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.cc#newco... pdf/range_set.cc:180: std::list<gfx::Range> new_ranges; Just use a std::vector? Is there an advantage to using a std::list? https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h File pdf/range_set.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode31 pdf/range_set.h:31: void Union(const gfx::Range& range); Put this with the other RangeSet modifiers. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode47 pdf/range_set.h:47: void Xor(const gfx::Range& range); Is this ever going to get used in anything but unit tests? Can we omit it? https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode55 pdf/range_set.h:55: std::string ToString() const; Are ToString() and operator<< only used in one unit test file? If yes, move them into that file? https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode63 pdf/range_set.h:63: typedef std::set<gfx::Range, range_compare> RangesContainer; not: using foo = bar, instead of typedef bar foo. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set_unittest.cc File pdf/range_set_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set_unittest... pdf/range_set_unittest.cc:200: EXPECT_EQ("{}", range_set.ToString()); Just check IsEmpty() instead? https://codereview.chromium.org/2349753003/diff/200001/pdf/run_all_unittests.cc File pdf/run_all_unittests.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/run_all_unittests.... pdf/run_all_unittests.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no "(c)", same in other newly added files. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper.h File pdf/url_loader_wrapper.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:17: virtual ~URLLoaderWrapper() {} nit: blank line after. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:18: virtual int GetContentLength() const = 0; Please document virtual methods. e.g. Does this return negative values on error? https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:25: virtual bool GetByteRange(int* start, int* end) const = 0; If this returns false, does |start| and |end| get some predetermined values, or are do they remain potentially uninitialized? Is it [start, end] or [start, end) ? https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:48: if (base::StartsWith(range, "bytes", base::CompareCase::INSENSITIVE_ASCII)) { How about early returns instead? if (!base::StartsWith(content_range_str, "bytes", base::CompareCase::INSENSITIVE_ASCII)) { return false; } std::string range = content_range_str.substr(strlen("bytes")); // everything gets less indenting return true; https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:56: *start = atoi(range.c_str()); What happens if |range| contains garbage? https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:133: PP_DCHECK(start); You can just regular DCHECK(). https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:173: read_starter_.reset(new ReadStarter(this)); base:MakeUnique https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:203: return; nit: I like blank lines after returns, because that is a natural separator between different parts of the code. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:255: if (result > 0) { Again, early return if <= 0? https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:289: if (headers_var.is_string()) { SetResponseHeaders(headers_var.is_string() ? headers_var.AsString() : ""); https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:40: nit: No blank line. Keep all the overrides together. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:54: class ReadStarter; nit: blank line after https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:62: pp::Instance* plugin_instance_; pp::Instance* const? https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:74: char* buffer_ = nullptr; Also std::vector?
Description was changed from ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ========== to ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. The main ideas in this CL are: 1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position. 2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind) 3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing). 4) Fill this logic by tests. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ==========
https://codereview.chromium.org/2349753003/diff/200001/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/BUILD.gn#newcode7 pdf/BUILD.gn:7: import("//testing/test.gni") On 2016/10/21 09:33:08, Lei Zhang wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/BUILD.gn#newcode101 pdf/BUILD.gn:101: "//ui/gfx", On 2016/10/21 09:33:08, Lei Zhang wrote: > only "//ui/gfx/range" is needed? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:22: template <size_t N> On 2016/10/21 09:33:08, Lei Zhang wrote: > Is there any way to avoid making this a template class? No, this is increasing difficulty of it. https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:31: void SetChunkData(int chunk_index, std::unique_ptr<ChunkData> data) { On 2016/10/21 09:33:09, Lei Zhang wrote: > Should |chunk_index| be unsigned? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream_unitt... File pdf/chunk_stream_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/chunk_stream_unitt... pdf/chunk_stream_unittest.cc:16: return std::unique_ptr<TestChunkStream::ChunkData>( On 2016/10/21 09:33:09, Lei Zhang wrote: > nit: base::MakeUnique ? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:21: const int kChunkCloseDistance = 10; On 2016/10/21 09:33:09, Lei Zhang wrote: > Can you document what this is / why this value was chose? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:80: loader_ = std::move(loader); On 2016/10/21 09:33:09, Lei Zhang wrote: > Should |url_| be set here instead of above? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:119: return chunk_stream_.ReadData(gfx::Range(position, position + size), buf); On 2016/10/21 09:33:09, Lei Zhang wrote: > Can "position + size" overflow? yes (bacause this method is called from outside), but this is checking in ChunkStream::ReadData, and will return false in this case ( position > position+size) https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:239: int start_pos, end_pos; On 2016/10/21 09:33:09, Lei Zhang wrote: > nit: 1 declaration per line please. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:267: } else if (result == 0) { On 2016/10/21 09:33:09, Lei Zhang wrote: > nit: no else if after a return. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:292: count_of_bytes_received_ += input_size; On 2016/10/21 09:33:09, Lei Zhang wrote: > Can this overflow? No, count_of_bytes_received_ can not be great that the file size, and for it we was/is using uint32_t https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:321: if (chunk_saved) { On 2016/10/21 09:33:09, Lei Zhang wrote: > if (!chunk_saved) > return false; > > // do things > return true; Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:359: return chunk_stream_.filled_chunks_count() * 1. / On 2016/10/21 09:33:09, Lei Zhang wrote: > Can you static_cast to a float instead of "* 1." ? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; On 2016/10/21 09:33:09, Lei Zhang wrote: > Can this be a constexpr instead? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h#... pdf/document_loader.h:76: typedef ChunkStream<kDefaultRequestSize> DataStream; On 2016/10/21 09:33:09, Lei Zhang wrote: > nit: use "using" keyword instead. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:23: namespace { On 2016/10/21 09:33:09, Lei Zhang wrote: > nit: blank line after the beginning of the namespace, and before the end. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:26: struct LoaderData { On 2016/10/21 09:33:09, Lei Zhang wrote: > This need to be a class. > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:74: pp::CompletionCallback did_open_callback_; On 2016/10/21 09:33:09, Lei Zhang wrote: > Does SetOpenCallback() always pass in a |open_callback| that has > ope-n_callback.is_null() == false? If yes, then can you get rid of |wait_open_| > and use |did_open_callback_|'s is_null() check instead? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:77: char* buffer_ = nullptr; On 2016/10/21 09:33:09, Lei Zhang wrote: > Can this be a std::vector<char> instead? And std::vector all around, instead of > char* + size value. No, this buffer passed into SetReadCallback https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:134: }; On 2016/10/21 09:33:09, Lei Zhang wrote: > Please use DISALLOW_COPY_AND_ASSIGN(ClassName) liberally in classes. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader_un... pdf/document_loader_unittest.cc:139: ~TestClient() override {} On 2016/10/21 09:33:09, Lei Zhang wrote: > nit: blank line after Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/pdf_engine.h#newco... pdf/pdf_engine.h:186: virtual void CancelBrowserDownload() = 0; On 2016/10/21 09:33:09, Lei Zhang wrote: > Document please. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1013: result.reset(new URLLoaderWrapperImpl(GetPluginInstance(), On 2016/10/21 09:33:09, Lei Zhang wrote: > Can you assign instead of using reset(), perhaps with base::MakeUnique, or > base::WrapUnique() ? > > Or better yet, can you just return directly? Maybe you need a "return > std::move(...)"; Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.cc File pdf/range_set.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.cc#newco... pdf/range_set.cc:117: --start; // start now points to the key equal or lower than On 2016/10/21 09:33:10, Lei Zhang wrote: > nit: > > if (cond) { > // long comments > // that may wrap > do_something; > } Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.cc#newco... pdf/range_set.cc:180: std::list<gfx::Range> new_ranges; On 2016/10/21 09:33:10, Lei Zhang wrote: > Just use a std::vector? Is there an advantage to using a std::list? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h File pdf/range_set.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode31 pdf/range_set.h:31: void Union(const gfx::Range& range); On 2016/10/21 09:33:10, Lei Zhang wrote: > Put this with the other RangeSet modifiers. What do you mean? https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode47 pdf/range_set.h:47: void Xor(const gfx::Range& range); On 2016/10/21 09:33:10, Lei Zhang wrote: > Is this ever going to get used in anything but unit tests? Can we omit it? I want to use it in my next CLs. Also, it is part of standard operations on mathematical set. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode55 pdf/range_set.h:55: std::string ToString() const; On 2016/10/21 09:33:10, Lei Zhang wrote: > Are ToString() and operator<< only used in one unit test file? If yes, move them > into that file? It will be used in other unit test files. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode63 pdf/range_set.h:63: typedef std::set<gfx::Range, range_compare> RangesContainer; On 2016/10/21 09:33:10, Lei Zhang wrote: > not: using foo = bar, instead of typedef bar foo. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set_unittest.cc File pdf/range_set_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set_unittest... pdf/range_set_unittest.cc:200: EXPECT_EQ("{}", range_set.ToString()); On 2016/10/21 09:33:10, Lei Zhang wrote: > Just check IsEmpty() instead? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/run_all_unittests.cc File pdf/run_all_unittests.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/run_all_unittests.... pdf/run_all_unittests.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/10/21 09:33:10, Lei Zhang wrote: > nit: no "(c)", same in other newly added files. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper.h File pdf/url_loader_wrapper.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:17: virtual ~URLLoaderWrapper() {} On 2016/10/21 09:33:10, Lei Zhang wrote: > nit: blank line after. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:18: virtual int GetContentLength() const = 0; On 2016/10/21 09:33:10, Lei Zhang wrote: > Please document virtual methods. e.g. Does this return negative values on error? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:25: virtual bool GetByteRange(int* start, int* end) const = 0; On 2016/10/21 09:33:10, Lei Zhang wrote: > If this returns false, does |start| and |end| get some predetermined values, or > are do they remain potentially uninitialized? > > Is it [start, end] or [start, end) ? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:48: if (base::StartsWith(range, "bytes", base::CompareCase::INSENSITIVE_ASCII)) { On 2016/10/21 09:33:10, Lei Zhang wrote: > How about early returns instead? > > if (!base::StartsWith(content_range_str, "bytes", > base::CompareCase::INSENSITIVE_ASCII)) { > return false; > } > > std::string range = content_range_str.substr(strlen("bytes")); > // everything gets less indenting > return true; Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:56: *start = atoi(range.c_str()); On 2016/10/21 09:33:10, Lei Zhang wrote: > What happens if |range| contains garbage? No proplems with this before: https://cs.chromium.org/chromium/src/pdf/document_loader.cc?rcl=0&l=41 https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:133: PP_DCHECK(start); On 2016/10/21 09:33:10, Lei Zhang wrote: > You can just regular DCHECK(). Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:173: read_starter_.reset(new ReadStarter(this)); On 2016/10/21 09:33:10, Lei Zhang wrote: > base:MakeUnique Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:203: return; On 2016/10/21 09:33:10, Lei Zhang wrote: > nit: I like blank lines after returns, because that is a natural separator > between different parts of the code. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:255: if (result > 0) { On 2016/10/21 09:33:10, Lei Zhang wrote: > Again, early return if <= 0? no just return, additionaly we should call did_read_callback_. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:289: if (headers_var.is_string()) { On 2016/10/21 09:33:10, Lei Zhang wrote: > SetResponseHeaders(headers_var.is_string() ? headers_var.AsString() : ""); Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:40: On 2016/10/21 09:33:10, Lei Zhang wrote: > nit: No blank line. Keep all the overrides together. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:54: class ReadStarter; On 2016/10/21 09:33:10, Lei Zhang wrote: > nit: blank line after Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:62: pp::Instance* plugin_instance_; On 2016/10/21 09:33:10, Lei Zhang wrote: > pp::Instance* const? Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:74: char* buffer_ = nullptr; On 2016/10/21 09:33:10, Lei Zhang wrote: > Also std::vector? No, it was passed from ReadResponseBody
The CQ bit was checked by thestig@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: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I previously had a question about the new Timer class vs base::Timer / base::MockTimer. Can you respond? https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; On 2016/10/21 15:13:15, snake wrote: > On 2016/10/21 09:33:09, Lei Zhang wrote: > > Can this be a constexpr instead? > > Done. Thanks, though I literally meant "constexpr" - http://en.cppreference.com/w/cpp/language/constexpr https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h File pdf/range_set.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode31 pdf/range_set.h:31: void Union(const gfx::Range& range); On 2016/10/21 15:13:15, snake wrote: > On 2016/10/21 09:33:10, Lei Zhang wrote: > > Put this with the other RangeSet modifiers. > > What do you mean? What I mean is, you have Contains() and Intersects(), which are const methods, and then you have non-const methods like Union(), Intersect(), Subtract() and Xor(). Union() is alone by itself, but it would make more sense to group it together with Intersect() and friends. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode47 pdf/range_set.h:47: void Xor(const gfx::Range& range); On 2016/10/21 15:13:15, snake wrote: > On 2016/10/21 09:33:10, Lei Zhang wrote: > > Is this ever going to get used in anything but unit tests? Can we omit it? > I want to use it in my next CLs. Also, it is part of standard operations on > mathematical set. Ok, got it. If you are going to use it soon, then sure. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:56: *start = atoi(range.c_str()); On 2016/10/21 15:13:16, snake wrote: > On 2016/10/21 09:33:10, Lei Zhang wrote: > > What happens if |range| contains garbage? > > No proplems with this before: > https://cs.chromium.org/chromium/src/pdf/document_loader.cc?rcl=0&l=41 Got it. You are just moving code around. I will make a note of this and re-examine it later, separately. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:255: if (result > 0) { On 2016/10/21 15:13:16, snake wrote: > On 2016/10/21 09:33:10, Lei Zhang wrote: > > Again, early return if <= 0? > > no just return, additionaly we should call did_read_callback_. What I mean is: if (result <= 0) { did_read_callback_.Run(result); return; } ... Yes, this requires calling did_read_callback_.Run() in a second location, but it feels like an ok trade-off. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:74: char* buffer_ = nullptr; On 2016/10/21 15:13:16, snake wrote: > On 2016/10/21 09:33:10, Lei Zhang wrote: > > Also std::vector? > > No, it was passed from ReadResponseBody Even though ReadResponseBody() takes a pointer and a size value, that doesn't prevent the caller from using std::vector. With a std::vector, there's one less variable to keep track of, and on Linux in debug mode for instance, you get bounds checking when you access the vector using operator[]. You can still use std::vector::data() and size() to interface with ReadResponseBody(). https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader.cc... pdf/document_loader.cc:22: // The distance from last received chunk, when we wait requestings data, using typos: s/requestings/requesting/ here and I'm not sure what "type plaing" is suppose to mean on the next line. https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... pdf/document_loader_unittest.cc:31: int content_length = -1; Ok, so I admit it is a bit painful, but once you have a class, you need getters and setters. https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... pdf/document_loader_unittest.cc:63: PP_DCHECK(IsWaitOpen()); More regular DCHECK(). https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper.h File pdf/url_loader_wrapper.h (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:19: // Returns lenght of content, will be -1, if it is unknown. typo: s/lenght/length/ https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:21: // Returns is response headers containt "accept-ranges". typo: s/containt/contains/ I'd say 'Returns if the response headers contains "accept-ranges".' (Same for some of the methods below that return boolean values) https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:25: // Returns is response content type. grammar: Returns the response content type. (same for some of the methods that return non-boolean value) https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:52: // Progress only refers to the response body and does not include the headers. Same as GetByteRange() - are the out parameters defined if this returns false? https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.h (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:25: URLLoaderWrapperImpl(pp::Instance* const plugin_instance, It was fine before. Even though we made |plugin_instance_| "pp::Instance* const", the parameter doesn't have to be and is not commonly written this way.
https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... pdf/document_loader_unittest.cc:484: static_cast<int>(loader.GetDocumentSize())); For the try bot failure, you can get rid of the cast.
https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.h#... pdf/document_loader.h:27: enum { kDefaultRequestSize = 65536 }; On 2016/10/25 06:01:49, Lei Zhang wrote: > On 2016/10/21 15:13:15, snake wrote: > > On 2016/10/21 09:33:09, Lei Zhang wrote: > > > Can this be a constexpr instead? > > > > Done. > > Thanks, though I literally meant "constexpr" - > http://en.cppreference.com/w/cpp/language/constexpr Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h File pdf/range_set.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/range_set.h#newcode31 pdf/range_set.h:31: void Union(const gfx::Range& range); On 2016/10/25 06:01:49, Lei Zhang wrote: > On 2016/10/21 15:13:15, snake wrote: > > On 2016/10/21 09:33:10, Lei Zhang wrote: > > > Put this with the other RangeSet modifiers. > > > > What do you mean? > > What I mean is, you have Contains() and Intersects(), which are const methods, > and then you have non-const methods like Union(), Intersect(), Subtract() and > Xor(). Union() is alone by itself, but it would make more sense to group it > together with Intersect() and friends. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:255: if (result > 0) { On 2016/10/25 06:01:49, Lei Zhang wrote: > On 2016/10/21 15:13:16, snake wrote: > > On 2016/10/21 09:33:10, Lei Zhang wrote: > > > Again, early return if <= 0? > > > > no just return, additionaly we should call did_read_callback_. > > What I mean is: > > if (result <= 0) { > did_read_callback_.Run(result); > return; > } > > ... > > > Yes, this requires calling did_read_callback_.Run() in a second location, but it > feels like an ok trade-off. Done. https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.h (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:74: char* buffer_ = nullptr; On 2016/10/25 06:01:49, Lei Zhang wrote: > On 2016/10/21 15:13:16, snake wrote: > > On 2016/10/21 09:33:10, Lei Zhang wrote: > > > Also std::vector? > > > > No, it was passed from ReadResponseBody > > Even though ReadResponseBody() takes a pointer and a size value, that doesn't > prevent the caller from using std::vector. With a std::vector, there's one less > variable to keep track of, and on Linux in debug mode for instance, you get > bounds checking when you access the vector using operator[]. You can still use > std::vector::data() and size() to interface with ReadResponseBody(). Its are passed into URLLoaderWrapperImpl::ReadResponseBody in documentLoader (it is already container (std::array)), but i need store this variables, to using its in post task, to transfer into URLLoader::ReadResponseBody. https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader.cc... pdf/document_loader.cc:22: // The distance from last received chunk, when we wait requestings data, using On 2016/10/25 06:01:49, Lei Zhang wrote: > typos: s/requestings/requesting/ here and I'm not sure what "type plaing" is > suppose to mean on the next line. Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... pdf/document_loader_unittest.cc:31: int content_length = -1; On 2016/10/25 06:01:49, Lei Zhang wrote: > Ok, so I admit it is a bit painful, but once you have a class, you need getters > and setters. Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... pdf/document_loader_unittest.cc:63: PP_DCHECK(IsWaitOpen()); On 2016/10/25 06:01:49, Lei Zhang wrote: > More regular DCHECK(). Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/document_loader_un... pdf/document_loader_unittest.cc:484: static_cast<int>(loader.GetDocumentSize())); On 2016/10/25 06:04:29, Lei Zhang wrote: > For the try bot failure, you can get rid of the cast. Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/timer.h#newcode15 pdf/timer.h:15: class Timer { > Also, do you really need a new Timer class? There already exists > base::Timer and base::MockTimer. base::Timer is not working in pepper plugins. We have not base::MessageLoop in them, on which based base::Timer. This is Timer implementation based on pp::Core::CallOnMainThread. https://codereview.chromium.org/2349753003/diff/220001/pdf/timer.h#newcode33 pdf/timer.h:33: class TestTimerRunner { > So can you add this in the next CL, when it is needed? Ok, I will add this in next CL. :( https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper.h File pdf/url_loader_wrapper.h (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:19: // Returns lenght of content, will be -1, if it is unknown. On 2016/10/25 06:01:50, Lei Zhang wrote: > typo: s/lenght/length/ Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:21: // Returns is response headers containt "accept-ranges". On 2016/10/25 06:01:49, Lei Zhang wrote: > typo: s/containt/contains/ > > I'd say 'Returns if the response headers contains "accept-ranges".' (Same for > some of the methods below that return boolean values) Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:25: // Returns is response content type. On 2016/10/25 06:01:50, Lei Zhang wrote: > grammar: Returns the response content type. (same for some of the methods that > return non-boolean value) Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper.h:52: // Progress only refers to the response body and does not include the headers. On 2016/10/25 06:01:50, Lei Zhang wrote: > Same as GetByteRange() - are the out parameters defined if this returns false? Done. https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.h (right): https://codereview.chromium.org/2349753003/diff/220001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.h:25: URLLoaderWrapperImpl(pp::Instance* const plugin_instance, On 2016/10/25 06:01:50, Lei Zhang wrote: > It was fine before. Even though we made |plugin_instance_| "pp::Instance* > const", the parameter doesn't have to be and is not commonly written this way. Done.
The CQ bit was checked by thestig@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...
I'm still thinking about making ChunkStream a template class. I will play with the code a bit. https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader_un... pdf/document_loader_unittest.cc:251: } // namespace nit: blank line above this please. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode8 pdf/timer.h:8: #include <set> Not used. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode15 pdf/timer.h:15: class Timer { Please add a description for this class. Looks like this is a repeating timer? It will be helpful to mention why we can't use base::Timer. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode17 pdf/timer.h:17: explicit Timer(int delay); What unit is |delay| in? Rename to "delay_in_milliseconds" to match the CallOnMainThread() parameter? https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode23 pdf/timer.h:23: friend class TestTimerRunner; Add this in the next CL. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:272: if ((buffer_[i - 1] == '\n' && buffer_[i - 2] == '\n') || Adding an IsEndLine(const char* buffer, size_t size) helper function can help make this a bit more readable. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:275: int start_pos, end_pos; One declaration per line please.
https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:19: namespace { Another place where it would be good to have blank lines for anonymous namespaces. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:23: pp::URLRequestInfo MakeRangeRequest(pp::Instance* const plugin_instance, No need for const here either. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:31: request.SetFollowRedirects(true); Also, this will conflict with https://codereview.chromium.org/2409423004/ which is already in CQ - I'm setting it to false.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader_un... File pdf/document_loader_unittest.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader_un... pdf/document_loader_unittest.cc:251: } // namespace On 2016/10/25 18:24:52, Lei Zhang wrote: > nit: blank line above this please. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode8 pdf/timer.h:8: #include <set> On 2016/10/25 18:24:52, Lei Zhang wrote: > Not used. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode15 pdf/timer.h:15: class Timer { On 2016/10/25 18:24:52, Lei Zhang wrote: > Please add a description for this class. Looks like this is a repeating timer? > It will be helpful to mention why we can't use base::Timer. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode17 pdf/timer.h:17: explicit Timer(int delay); On 2016/10/25 18:24:52, Lei Zhang wrote: > What unit is |delay| in? Rename to "delay_in_milliseconds" to match the > CallOnMainThread() parameter? Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/timer.h#newcode23 pdf/timer.h:23: friend class TestTimerRunner; On 2016/10/25 18:24:52, Lei Zhang wrote: > Add this in the next CL. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:19: namespace { On 2016/10/25 18:32:32, Lei Zhang wrote: > Another place where it would be good to have blank lines for anonymous > namespaces. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:23: pp::URLRequestInfo MakeRangeRequest(pp::Instance* const plugin_instance, On 2016/10/25 18:32:32, Lei Zhang wrote: > No need for const here either. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:31: request.SetFollowRedirects(true); On 2016/10/25 18:32:32, Lei Zhang wrote: > Also, this will conflict with https://codereview.chromium.org/2409423004/ which > is already in CQ - I'm setting it to false. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:272: if ((buffer_[i - 1] == '\n' && buffer_[i - 2] == '\n') || On 2016/10/25 18:24:52, Lei Zhang wrote: > Adding an IsEndLine(const char* buffer, size_t size) helper function can help > make this a bit more readable. In this we should check double endl (with or without "\r"), which is headers end. Additional helper function increasing difficulty for read of this check. For example: IsEndLineWithoutR(buffer_[i-1]) && IsEndLineWithoutR(buffer_[i-2]) || (i>=4 && IsEndLineWithR(buffer_+i-4, 2) && IsEndLineWithR(buffer_+i-2, 2)) JFYI: This just moved from: https://cs.chromium.org/chromium/src/pdf/document_loader.cc?q=document_loader... https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:275: int start_pos, end_pos; On 2016/10/25 18:24:52, Lei Zhang wrote: > One declaration per line please. Done.
I'm not sure why the Linux trybots are failing. I can reproduce it locally, but it is not obvious why. I thought it was a component vs static build issue, i.e. needs the equivalent to BASE_EXPORT, but that is not it. https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:119: return chunk_stream_.ReadData(gfx::Range(position, position + size), buf); On 2016/10/21 15:13:15, snake wrote: > On 2016/10/21 09:33:09, Lei Zhang wrote: > > Can "position + size" overflow? > > yes (bacause this method is called from outside), but this is checking in > ChunkStream::ReadData, and will return false in this case ( position > > position+size) Can we stop integer overflows here, i.e. earlier rather than later? You can use base/numerics/safe_math.h to make the checking a bit easier. Same for other places in this file. https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:25: static const uint32_t kChunkSize = N; constexpr? https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:26: typedef typename std::array<unsigned char, N> ChunkData; using ChunkData = typename std::array<unsigned char, N>; https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader.cc... pdf/document_loader.cc:304: chunk_.chunk_data.reset(new DataStream::ChunkData()); MakeUnique.
The CQ bit was checked by thestig@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...
https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:272: if ((buffer_[i - 1] == '\n' && buffer_[i - 2] == '\n') || On 2016/10/25 19:16:27, snake wrote: > On 2016/10/25 18:24:52, Lei Zhang wrote: > > Adding an IsEndLine(const char* buffer, size_t size) helper function can help > > make this a bit more readable. > > In this we should check double endl (with or without "\r"), which is headers > end. Additional helper function increasing difficulty for read of this check. > > For example: > > IsEndLineWithoutR(buffer_[i-1]) && IsEndLineWithoutR(buffer_[i-2]) || > (i>=4 && IsEndLineWithR(buffer_+i-4, 2) && IsEndLineWithR(buffer_+i-2, 2)) Yes, I understand we are checking more than one thing. To better and more correctly state my thoughts, I'm proposing: bool IsDoubleEndLine(const char* buffer, int index) { if (index < 2) return false; if (buffer_[i - 1] == '\n' && buffer_[i - 2] == '\n') return true; if (index < 4) return false; return buffer_[i - 1] == '\n' && buffer_[i - 2] == '\r' && buffer_[i - 3] == '\n' && buffer_[i - 4] == '\r'); } I'm just writing it out in an expanded form for clarity. You can write it more IsDoubleEndLine() compactly. Then in this function, we can just make a single helper function call that is easier to understand than a big blob of comparisons. > JFYI: This just moved from: > https://cs.chromium.org/chromium/src/pdf/document_loader.cc?q=document_loader... Understood, but I think the original code can use the same refactoring. If you like to leave it as is, that's fine too. Someone else can clean it up later. https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h#... pdf/document_loader.h:27: static const uint32_t kDefaultRequestSize = 65536; Is this an attempt to fix the Linux build on the try bots? I made this same change locally, but it still fails. I believe the last time you had green Linux try bots, this was written as "enum { kDefaultRequestSize = 65536 };" https://codereview.chromium.org/2349753003/diff/260001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/260001/pdf/timer.h#newcode14 pdf/timer.h:14: // We can not use base::Timer for plugins, because they have not grammar: they have no base::MessageLoop
https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/200001/pdf/document_loader.cc... pdf/document_loader.cc:119: return chunk_stream_.ReadData(gfx::Range(position, position + size), buf); On 2016/10/25 19:26:00, Lei Zhang wrote: > On 2016/10/21 15:13:15, snake wrote: > > On 2016/10/21 09:33:09, Lei Zhang wrote: > > > Can "position + size" overflow? > > > > yes (bacause this method is called from outside), but this is checking in > > ChunkStream::ReadData, and will return false in this case ( position > > > position+size) > > Can we stop integer overflows here, i.e. earlier rather than later? You can use > base/numerics/safe_math.h to make the checking a bit easier. > > Same for other places in this file. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:26: typedef typename std::array<unsigned char, N> ChunkData; On 2016/10/25 19:26:01, Lei Zhang wrote: > using ChunkData = typename std::array<unsigned char, N>; Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/document_loader.cc... pdf/document_loader.cc:304: chunk_.chunk_data.reset(new DataStream::ChunkData()); On 2016/10/25 19:26:01, Lei Zhang wrote: > MakeUnique. Done. https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:272: if ((buffer_[i - 1] == '\n' && buffer_[i - 2] == '\n') || On 2016/10/25 19:40:37, Lei Zhang wrote: > On 2016/10/25 19:16:27, snake wrote: > > On 2016/10/25 18:24:52, Lei Zhang wrote: > > > Adding an IsEndLine(const char* buffer, size_t size) helper function can > help > > > make this a bit more readable. > > > > In this we should check double endl (with or without "\r"), which is headers > > end. Additional helper function increasing difficulty for read of this check. > > > > For example: > > > > IsEndLineWithoutR(buffer_[i-1]) && IsEndLineWithoutR(buffer_[i-2]) || > > (i>=4 && IsEndLineWithR(buffer_+i-4, 2) && IsEndLineWithR(buffer_+i-2, 2)) > > Yes, I understand we are checking more than one thing. To better and more > correctly state my thoughts, I'm proposing: > > bool IsDoubleEndLine(const char* buffer, int index) { > if (index < 2) > return false; > > if (buffer_[i - 1] == '\n' && buffer_[i - 2] == '\n') > return true; > > if (index < 4) > return false; > > return buffer_[i - 1] == '\n' && buffer_[i - 2] == '\r' && buffer_[i - 3] == > '\n' && buffer_[i - 4] == '\r'); > } > > I'm just writing it out in an expanded form for clarity. You can write it more > IsDoubleEndLine() compactly. > > Then in this function, we can just make a single helper function call that is > easier to understand than a big blob of comparisons. > > > JFYI: This just moved from: > > > https://cs.chromium.org/chromium/src/pdf/document_loader.cc?q=document_loader... > > Understood, but I think the original code can use the same refactoring. If you > like to leave it as is, that's fine too. Someone else can clean it up later. Done. https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h#... pdf/document_loader.h:27: static const uint32_t kDefaultRequestSize = 65536; On 2016/10/25 19:40:37, Lei Zhang wrote: > Is this an attempt to fix the Linux build on the try bots? I made this same > change locally, but it still fails. > > I believe the last time you had green Linux try bots, this was written as "enum > { kDefaultRequestSize = 65536 };" Yes. Can I return enum? https://codereview.chromium.org/2349753003/diff/260001/pdf/timer.h File pdf/timer.h (right): https://codereview.chromium.org/2349753003/diff/260001/pdf/timer.h#newcode14 pdf/timer.h:14: // We can not use base::Timer for plugins, because they have not On 2016/10/25 19:40:37, Lei Zhang wrote: > grammar: they have no base::MessageLoop Done.
https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h#... pdf/document_loader.h:27: static const uint32_t kDefaultRequestSize = 65536; On 2016/10/25 20:07:17, snake wrote: > On 2016/10/25 19:40:37, Lei Zhang wrote: > > Is this an attempt to fix the Linux build on the try bots? I made this same > > change locally, but it still fails. > > > > I believe the last time you had green Linux try bots, this was written as > "enum > > { kDefaultRequestSize = 65536 };" > > Yes. Can I return enum? Yes, please. If I ever figure out why it's broken on Linux in the future, I will try do make it a constexpr again.
https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h File pdf/chunk_stream.h (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/chunk_stream.h#new... pdf/chunk_stream.h:25: static const uint32_t kChunkSize = N; On 2016/10/25 19:26:01, Lei Zhang wrote: > constexpr? Done. https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/2349753003/diff/260001/pdf/document_loader.h#... pdf/document_loader.h:27: static const uint32_t kDefaultRequestSize = 65536; On 2016/10/25 20:42:04, Lei Zhang wrote: > On 2016/10/25 20:07:17, snake wrote: > > On 2016/10/25 19:40:37, Lei Zhang wrote: > > > Is this an attempt to fix the Linux build on the try bots? I made this same > > > change locally, but it still fails. > > > > > > I believe the last time you had green Linux try bots, this was written as > > "enum > > > { kDefaultRequestSize = 65536 };" > > > > Yes. Can I return enum? > > Yes, please. If I ever figure out why it's broken on Linux in the future, I will > try do make it a constexpr again. I found the workaround.
https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... File pdf/url_loader_wrapper_impl.cc (right): https://codereview.chromium.org/2349753003/diff/240001/pdf/url_loader_wrapper... pdf/url_loader_wrapper_impl.cc:31: request.SetFollowRedirects(true); On 2016/10/25 19:16:27, snake wrote: > On 2016/10/25 18:32:32, Lei Zhang wrote: > > Also, this will conflict with https://codereview.chromium.org/2409423004/ > which > > is already in CQ - I'm setting it to false. > > Done. The above CL landed, so be sure to sync one more time or the patch for this CL may not apply. https://codereview.chromium.org/2349753003/diff/280001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/280001/pdf/document_loader.cc... pdf/document_loader.cc:125: if (position > position + size) { Can we stick with the safe math library for these, so we handle overflows consistently? In this case, this check works. However if |position| and |size| were signed integers, then if position + size overflows, it is undefined behavior. Which in turn means compilers can optimize it however it likes and that can lead to unintended surprises.
On 2016/10/25 20:44:59, snake wrote: > I found the workaround. Great. I think we just need to do the integer overflow check using the safe math library. That's all I got. Thanks for being patient.
https://codereview.chromium.org/2349753003/diff/280001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2349753003/diff/280001/pdf/document_loader.cc... pdf/document_loader.cc:125: if (position > position + size) { On 2016/10/25 20:51:58, Lei Zhang wrote: > Can we stick with the safe math library for these, so we handle overflows > consistently? > > In this case, this check works. However if |position| and |size| were signed > integers, then if position + size overflows, it is undefined behavior. Which in > turn means compilers can optimize it however it likes and that can lead to > unintended surprises. Done.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
lgtm I started more try bots...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
BTW, the new pdf_unittests binary probably won't run automatically on bots. We need to change bot configurations. Once this lands, I'll file a bug for that and CC you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/25 22:20:25, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. And submitting...
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from spelchat@chromium.org Link to the patchset: https://codereview.chromium.org/2349753003/#ps340001 (title: "rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thestig@chromium.org changed reviewers: + danakj@chromium.org
thestig@chromium.org changed reviewers: + rsesek@chromium.org - danakj@chromium.org
On 2016/10/25 22:35:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) +rsesek for ui/gfx/range/OWNERS - new pdf/DEPS entry.
DEPS lgtm
The CQ bit was checked by thestig@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 checked by art-snake@yandex-team.ru
The CQ bit was unchecked by art-snake@yandex-team.ru
Message was sent while issue was closed.
Description was changed from ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. The main ideas in this CL are: 1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position. 2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind) 3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing). 4) Fill this logic by tests. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ========== to ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. The main ideas in this CL are: 1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position. 2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind) 3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing). 4) Fill this logic by tests. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. The main ideas in this CL are: 1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position. 2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind) 3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing). 4) Fill this logic by tests. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing ========== to ========== Improve linearized pdf load/show time. Reduce Pdf Plugin's count of reconnects. Add tests for PDFPlugin DocumentLoader. DocumentLoader was splitted into separate components, and missing tests was added for them. The main ideas in this CL are: 1) Do not reset browser initiated connection at start (includes case when we can use range requests), if we request data near current downloading position. 2) Request as much data as we can on each request, and continue loading data using current range request. (like tape rewind) 3) Isolate RangeRequest logic into DocumentLoader. Method OnPendingRequestComplete is called, when we receive requested data (main connection, or Range connection). (like tape playing without rewing). 4) Fill this logic by tests. Example URL: http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e... Comparison of changes: https://drive.google.com/file/d/0BzWfMBOuik2QNGg0SG93Y3lpUlE/view?usp=sharing Committed: https://crrev.com/7fd7423cdee0dba84faf480d10dd66dcb57110d9 Cr-Commit-Position: refs/heads/master@{#427752} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/7fd7423cdee0dba84faf480d10dd66dcb57110d9 Cr-Commit-Position: refs/heads/master@{#427752}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/2458493002/ by thestig@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/5423/steps/co... FAILED: obj/pdf/pdf_unittests/document_loader_unittest.obj pdf\document_loader_unittest.cc(631): error C2131: expression did not evaluate to a constant pdf\document_loader_unittest.cc(631): note: failure was caused by call of undefined function or one not declared 'constexpr' pdf\document_loader_unittest.cc(631): note: see usage of 'chrome_pdf::DocumentLoader::default_request_size' .
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 427752 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2016/10/26 19:20:41, Lei Zhang wrote: > A revert of this CL (patchset #18 id:340001) has been created in > https://codereview.chromium.org/2458493002/ by mailto:thestig@chromium.org. > > The reason for reverting is: > https://build.chromium.org/p/chromium/builders/Win%20x64/builds/5423/steps/co... > > FAILED: obj/pdf/pdf_unittests/document_loader_unittest.obj > pdf\document_loader_unittest.cc(631): error C2131: expression did not evaluate > to a constant > pdf\document_loader_unittest.cc(631): note: failure was caused by call of > undefined function or one not declared 'constexpr' > pdf\document_loader_unittest.cc(631): note: see usage of > 'chrome_pdf::DocumentLoader::default_request_size' > . The reason this passed on the Windows try bots, but not the main waterfall Windows bots, is because the try bots never built pdf/document_loader_unittest.cc. The try bots build a long list of targets, none of which depends on pdf/document_loader_unittest.cc. The main waterfall bots builds the "all" target. If we take smaller steps instead a bigger leap, we may have been able to avoid this. e.g. 1) Add the pdf_unittests build target, with some simple tests in one CL. 2) Change build bot configs to build and run pdf_unittests. (Requires maybe a few CLs to do the bot configuration?) 3) Land the rest of this CL. At which point, there will actually be proper coverage for pdf_unittests.
Message was sent while issue was closed.
On 2016/10/26 22:16:24, Lei Zhang wrote: > If we take smaller steps instead a bigger leap, we may have been able to avoid > this. e.g. ... instead _of_ a bigger ... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
