Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
566693002:
Use file handles to interact with utility process. (Closed)
|
Created:
6 years, 3 months ago by Vitaly Buka (NO REVIEWS) Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse file handles to interact with utility process.
Spool pages as they returned from conversion instead of waiting for entire document.
Service process loads each PDFs once per print job, instead of once per page.
BUG=408184
Committed: https://crrev.com/34cea56676e4d4cad6710906c23c5d5260d12be7
Cr-Commit-Position: refs/heads/master@{#295360}
Patch Set 1 : Fri Sep 12 15:13:21 PDT 2014 #Patch Set 2 : Fri Sep 12 21:01:56 PDT 2014 #Patch Set 3 : Mon Sep 15 03:22:54 PDT 2014 #
Total comments: 26
Patch Set 4 : Mon Sep 15 15:32:16 PDT 2014 #Patch Set 5 : Mon Sep 15 18:19:41 PDT 2014 #
Total comments: 40
Patch Set 6 : Tue Sep 16 00:49:42 PDT 2014 #Patch Set 7 : Tue Sep 16 00:58:06 PDT 2014 #Patch Set 8 : Tue Sep 16 01:11:23 PDT 2014 #
Total comments: 5
Patch Set 9 : Tue Sep 16 15:03:32 PDT 2014 #Patch Set 10 : Tue Sep 16 15:41:47 PDT 2014 #
Total comments: 3
Patch Set 11 : Tue Sep 16 16:01:51 PDT 2014 #
Total comments: 6
Patch Set 12 : Wed Sep 17 10:40:51 PDT 2014 #Messages
Total messages: 49 (16 generated)
vitalybuka@chromium.org changed reviewers: + thestig@chromium.org
Can you fix the build first? I fixed the subject line BTW.
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
On 2014/09/12 22:24:03, Lei Zhang wrote: > Can you fix the build first? > > I fixed the subject line BTW. Done.
Patchset #3 (id:170001) has been deleted
Patchset #2 (id:130014) has been deleted
Doh, I just lost all my pending review messages. :(
We also can use BATCH mode where browser controls process Browser->LoadPDF->Utility Browser<-PdfLoaded(number_of_pages)<-Utility Browser->GetPage(i)->Utility Browser<-PageDone<-Utility ... Browser->GetPage(i)->Utility Browser<-PageDone<-Utility ... BATCH mode is not implemented in service_utility_process_host only in content::UtilityProcessHost, which is content/browser. Still it could be implemented easily. On a first look it would make utility size more complicated without simplification of browser side. I'll take a look into that again. Am I correctly understood your notice from chat about state? Another problem I see that we have to duplicate a lot of conversion logic because service process can't use UtilityProcessHost. If we move UtilityProcessHost (interface only) into content/common we can reuse conversion code. Alternative solution is wrapper for UtilityProcessHost. Also looking on UtilityProcessHostImpl I have noticed that we don't need to run PdfToEmfUtilityProcessHostClient on IO thread. File operation already on file thread, and IPC already on IO thread by implementation of UtilityProcessHostImpl.
> Also looking on UtilityProcessHostImpl I have noticed that we don't need to run > PdfToEmfUtilityProcessHostClient on IO thread. File operation already on file > thread, and IPC already on IO thread by implementation of > UtilityProcessHostImpl. That's not true. We need and ProcessData from utility process utility_process_host_ which must be requested on IO thread. So moving converted on UI thread will simplify calling client callback, but complexity will be increased in other places. I'll leave as is, with IO thread, to be consistent with the rest of utility process clients.
Please take a look at pdf_to_emf_converter.cc, chrome/utility/printing_handler.cc and chrome_utility_printing_messages.h Which one do you like more, #2 or #3? Service side is not ready yet.
I like patch set 3 better. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:159: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); nit: DCHECK_CURRENTLY_ON() https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:197: if (file_->ReadAtCurrentPos(&data[0], data.size()) != size) nit: &data[0] -> data.front() https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:215: BrowserThread::PostTask( Can we just call Convert() on the IO thread from all callers? (i.e. PostTask() if needed) https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/print_job.cc (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:230: gfx::Rect content_area) { Can you DCHECK here and in the callbacks to make sure you are on the right thread. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:234: pdf_to_emf_page_size_ = page_size; This is set once here for PDF file. Is the PDF file guaranteeded to have all pages be the same size at this point? Arbitrary PDFs can have pages with different page sizes. Is the PDF only 1 page when called from PrintViewManagerBase::OnDidPrintPage() so it doesn't matter in that case? https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:256: if (pdf_to_emf_current_page_ < pdf_to_emf_page_count_) nit: wrap inner block in braces. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/print_job.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.h:95: gfx::Size page_size, nit: pass by reference? https://codereview.chromium.org/566693002/diff/210001/chrome/common/chrome_ut... File chrome/common/chrome_utility_printing_messages.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/common/chrome_ut... chrome/common/chrome_utility_printing_messages.h:124: IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_RenderPDFPagesToMetafiles, BTW, this should go in the "Utility process messages" section between lines 60-86. https://codereview.chromium.org/566693002/diff/210001/chrome/common/chrome_ut... chrome/common/chrome_utility_printing_messages.h:133: IPC_MESSAGE_CONTROL2(ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_GetPage, This should be a ChromeUtilityMsg and go in the above section. https://codereview.chromium.org/566693002/diff/210001/chrome/service/cloud_pr... File chrome/service/cloud_print/print_system.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/service/cloud_pr... chrome/service/cloud_print/print_system.h:77: public: nit: actually this is indented 1 space too many. https://codereview.chromium.org/566693002/diff/210001/chrome/service/service_... File chrome/service/service_utility_process_host.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/service/service_... chrome/service/service_utility_process_host.h:131: /*void ReplayCreateFileIfReady(); dead code? (and in .cc file) https://codereview.chromium.org/566693002/diff/210001/chrome/service/service_... chrome/service/service_utility_process_host.h:135: void OnRenderPDFPagesToMetafilesPageCount(int page_count) {}; Don't you need the page count so you know how many times to send RenderPDFPagesToMetafiles_GetPage?
https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:159: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2014/09/15 21:49:53, Lei Zhang wrote: > nit: DCHECK_CURRENTLY_ON() Done. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:197: if (file_->ReadAtCurrentPos(&data[0], data.size()) != size) On 2014/09/15 21:49:53, Lei Zhang wrote: > nit: &data[0] -> data.front() std::vector::data()? Done. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:215: BrowserThread::PostTask( I realy like this way. We need to Post anyway, but this way it's in most relevant place. On 2014/09/15 21:49:53, Lei Zhang wrote: > Can we just call Convert() on the IO thread from all callers? (i.e. PostTask() > if needed) https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/print_job.cc (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:230: gfx::Rect content_area) { On 2014/09/15 21:49:53, Lei Zhang wrote: > Can you DCHECK here and in the callbacks to make sure you are on the right > thread. Done. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:234: pdf_to_emf_page_size_ = page_size; That's looks incorrect. But I don't change behaivour in this patch. Maybe it was regressed when we switched from EMF to PDF for renderer IPC I'll file but to investigate. https://code.google.com/p/chromium/issues/detail?id=414477 On 2014/09/15 21:49:53, Lei Zhang wrote: > This is set once here for PDF file. Is the PDF file guaranteeded to have all > pages be the same size at this point? Arbitrary PDFs can have pages with > different page sizes. > > Is the PDF only 1 page when called from PrintViewManagerBase::OnDidPrintPage() > so it doesn't matter in that case? https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:256: if (pdf_to_emf_current_page_ < pdf_to_emf_page_count_) On 2014/09/15 21:49:53, Lei Zhang wrote: > nit: wrap inner block in braces. Done. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.cc:256: if (pdf_to_emf_current_page_ < pdf_to_emf_page_count_) On 2014/09/15 21:49:53, Lei Zhang wrote: > nit: wrap inner block in braces. Done. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/print_job.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/print_job.h:95: gfx::Size page_size, On 2014/09/15 21:49:53, Lei Zhang wrote: > nit: pass by reference? Done. https://codereview.chromium.org/566693002/diff/210001/chrome/common/chrome_ut... File chrome/common/chrome_utility_printing_messages.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/common/chrome_ut... chrome/common/chrome_utility_printing_messages.h:124: IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_RenderPDFPagesToMetafiles, On 2014/09/15 21:49:53, Lei Zhang wrote: > BTW, this should go in the "Utility process messages" section between lines > 60-86. Done. https://codereview.chromium.org/566693002/diff/210001/chrome/common/chrome_ut... chrome/common/chrome_utility_printing_messages.h:133: IPC_MESSAGE_CONTROL2(ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_GetPage, On 2014/09/15 21:49:53, Lei Zhang wrote: > This should be a ChromeUtilityMsg and go in the above section. Done. https://codereview.chromium.org/566693002/diff/210001/chrome/service/cloud_pr... File chrome/service/cloud_print/print_system.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/service/cloud_pr... chrome/service/cloud_print/print_system.h:77: public: On 2014/09/15 21:49:53, Lei Zhang wrote: > nit: actually this is indented 1 space too many. Done. https://codereview.chromium.org/566693002/diff/210001/chrome/service/service_... File chrome/service/service_utility_process_host.h (right): https://codereview.chromium.org/566693002/diff/210001/chrome/service/service_... chrome/service/service_utility_process_host.h:131: /*void ReplayCreateFileIfReady(); As I wrote before this part is not updated from patch #2. On 2014/09/15 21:49:53, Lei Zhang wrote: > dead code? (and in .cc file) https://codereview.chromium.org/566693002/diff/210001/chrome/service/service_... chrome/service/service_utility_process_host.h:135: void OnRenderPDFPagesToMetafilesPageCount(int page_count) {}; ditto On 2014/09/15 21:49:53, Lei Zhang wrote: > Don't you need the page count so you know how many times to send > RenderPDFPagesToMetafiles_GetPage?
Done.
Need to fix non-Win builds. https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/210001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:215: BrowserThread::PostTask( On 2014/09/15 22:33:49, Vitaly Buka wrote: > I realy like this way. We need to Post anyway, but this way it's in most > relevant place. > > On 2014/09/15 21:49:53, Lei Zhang wrote: > > Can we just call Convert() on the IO thread from all callers? (i.e. PostTask() > > if needed) > But don't you only have 1 caller? Just make the caller do the PostTask(). https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:27: typedef scoped_ptr<base::File, BrowserThread::DeleteOnFileThread> TempFilePtr; Maybe rename TempFilePtr to ScopedTempFile? https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:30: // on playback. |InitFromFile| can play metafile directly from disk, but it nit: |InitFromFile| -> Emf::InitFromFile() https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:32: // files, and to efficiently interact with sandbox process. nit: sandbox -> utility? https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:57: // 1. Clients requiquest page with file handle to temp file. typo https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:107: std::map<int, std::pair<PdfToEmfConverter::GetPageCallback, TempFilePtr> > document what the key is. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:259: base::Bind(&PdfToEmfUtilityProcessHostClient::OnTempEmfReady, Can you move the OnTempEmfReady() impl to be right below here, so it's easier to follow the "success" path? https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:316: base::Bind(&PdfToEmfUtilityProcessHostClient::OnTempPdfReady, this)); and move OnTempPdfReady to right after this. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:338: emf.reset(new LazyEmf(get_page_callbacks_.begin()->second.second.Pass())); nit: save the iterator from get_page_callbacks_.begin() and reuse it below instead of writing get_page_callbacks_.begin() everywhere. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:338: emf.reset(new LazyEmf(get_page_callbacks_.begin()->second.second.Pass())); If you are using get_page_callbacks_.begin() here, then does that mean only one callback can be in flight at any time? If so, why do we need the |get_page_callbacks_| map? Or are you taking advantage of the fact that begin() returns the lowest page # due to std::map's sorting property, combined with the expectation that the utility process convert pages in order? I'm guessing this is it, but it's not quite obvious, so please add a comment. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:373: if (!utility_process_host_ || get_page_callbacks_.count(page_number) != 1) nit: ContainsKey(get_page_callbacks_, page_number) https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.h (right): https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.h:32: virtual void Start(const scoped_refptr<base::RefCountedMemory>& data, nit: document the abstract methods. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... File chrome/browser/printing/print_job.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/print_job.cc:255: // Release converter because we don't need it any more. nit: s/because/if/ https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... File chrome/service/service_utility_process_host.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:64: private: private block looks empty, DISALLOW_COPY_AND_ASSIGN() then? https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:97: emf_files_.push_back(CreateTempFile()); Do you care if CreateTempFile() fails? https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:105: bool OnPageProcessed() { document return value https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:232: bool ServiceUtilityProcessHost::Launch(CommandLine* cmd_line, bool no_sandbox) { nit: base::CommandLine https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:303: DCHECK(waiting_for_reply_); Add "bool PdfToEmfState::has_page_count()" { return page_count_ > 0; } and fail if it's true here? https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:424: if (file.ReadAtCurrentPos(&data[0], data.size()) != size) { nit: also data.data() https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... File chrome/service/service_utility_process_host.h (right): https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.h:80: bool MetafileAvailable(double scale_factor, base::File file); What does this return?
https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:27: typedef scoped_ptr<base::File, BrowserThread::DeleteOnFileThread> TempFilePtr; On 2014/09/16 03:36:54, Lei Zhang wrote: > Maybe rename TempFilePtr to ScopedTempFile? Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:30: // on playback. |InitFromFile| can play metafile directly from disk, but it On 2014/09/16 03:36:54, Lei Zhang wrote: > nit: |InitFromFile| -> Emf::InitFromFile() Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:32: // files, and to efficiently interact with sandbox process. On 2014/09/16 03:36:54, Lei Zhang wrote: > nit: sandbox -> utility? Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:57: // 1. Clients requiquest page with file handle to temp file. On 2014/09/16 03:36:54, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:107: std::map<int, std::pair<PdfToEmfConverter::GetPageCallback, TempFilePtr> > On 2014/09/16 03:36:54, Lei Zhang wrote: > document what the key is. Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:259: base::Bind(&PdfToEmfUtilityProcessHostClient::OnTempEmfReady, On 2014/09/16 03:36:54, Lei Zhang wrote: > Can you move the OnTempEmfReady() impl to be right below here, so it's easier to > follow the "success" path? Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:316: base::Bind(&PdfToEmfUtilityProcessHostClient::OnTempPdfReady, this)); On 2014/09/16 03:36:54, Lei Zhang wrote: > and move OnTempPdfReady to right after this. Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:338: emf.reset(new LazyEmf(get_page_callbacks_.begin()->second.second.Pass())); On 2014/09/16 03:36:54, Lei Zhang wrote: > nit: save the iterator from get_page_callbacks_.begin() and reuse it below > instead of writing get_page_callbacks_.begin() everywhere. Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:338: emf.reset(new LazyEmf(get_page_callbacks_.begin()->second.second.Pass())); Actually this is a bug. Works fine with current client, but if GetPage(2), GetPage(1) would fail. Changed to queue On 2014/09/16 03:36:54, Lei Zhang wrote: > If you are using get_page_callbacks_.begin() here, then does that mean only one > callback can be in flight at any time? If so, why do we need the > |get_page_callbacks_| map? > > Or are you taking advantage of the fact that begin() returns the lowest page # > due to std::map's sorting property, combined with the expectation that the > utility process convert pages in order? I'm guessing this is it, but it's not > quite obvious, so please add a comment. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:373: if (!utility_process_host_ || get_page_callbacks_.count(page_number) != 1) On 2014/09/16 03:36:54, Lei Zhang wrote: > nit: ContainsKey(get_page_callbacks_, page_number) Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.h (right): https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.h:32: virtual void Start(const scoped_refptr<base::RefCountedMemory>& data, On 2014/09/16 03:36:54, Lei Zhang wrote: > nit: document the abstract methods. Done. https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... File chrome/browser/printing/print_job.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/browser/printing... chrome/browser/printing/print_job.cc:255: // Release converter because we don't need it any more. On 2014/09/16 03:36:54, Lei Zhang wrote: > nit: s/because/if/ Done. https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... File chrome/service/service_utility_process_host.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:64: private: On 2014/09/16 03:36:55, Lei Zhang wrote: > private block looks empty, DISALLOW_COPY_AND_ASSIGN() then? Done. https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:97: emf_files_.push_back(CreateTempFile()); I don't. Invalid handle will be passed to utility process and it will just return success=false Still I have some concerns about collisions in base::CreateTemporaryFile, so I am returning ScopedTempDir On 2014/09/16 03:36:55, Lei Zhang wrote: > Do you care if CreateTempFile() fails? https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:105: bool OnPageProcessed() { On 2014/09/16 03:36:54, Lei Zhang wrote: > document return value Done. https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:232: bool ServiceUtilityProcessHost::Launch(CommandLine* cmd_line, bool no_sandbox) { On 2014/09/16 03:36:55, Lei Zhang wrote: > nit: base::CommandLine Done. https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:303: DCHECK(waiting_for_reply_); On 2014/09/16 03:36:55, Lei Zhang wrote: > Add "bool PdfToEmfState::has_page_count()" { return page_count_ > 0; } and fail > if it's true here? Done. https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:424: if (file.ReadAtCurrentPos(&data[0], data.size()) != size) { On 2014/09/16 03:36:55, Lei Zhang wrote: > nit: also data.data() Done. https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... File chrome/service/service_utility_process_host.h (right): https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.h:80: bool MetafileAvailable(double scale_factor, base::File file); On 2014/09/16 03:36:55, Lei Zhang wrote: > What does this return? Done.
Looking good. Can you help me understand the global ScopedTempDir usage? https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... File chrome/service/service_utility_process_host.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:97: emf_files_.push_back(CreateTempFile()); On 2014/09/16 07:50:36, Vitaly Buka wrote: > Still I have some concerns about collisions in base::CreateTemporaryFile, so I > am returning ScopedTempDir I don't think CreateTemporaryFile() will have collisions and return false very often, but ScopedTempDir + CreateTemporaryFile() is fine. https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:176: // Use global object to avoid tracking all opened files. I'm slightly confused by this comment. Aren't you still using ScopedTempFile to keep track of the returned file from CreateTempFile()? Are you trying to just keep everything in its own directory? https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:265: // Store callback before any OnFailed() calles to make it called on failure. typo: calls, same on line 333
https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... File chrome/service/service_utility_process_host.cc (right): https://codereview.chromium.org/566693002/diff/250001/chrome/service/service_... chrome/service/service_utility_process_host.cc:97: emf_files_.push_back(CreateTempFile()); On 2014/09/16 19:57:36, Lei Zhang wrote: > On 2014/09/16 07:50:36, Vitaly Buka wrote: > > Still I have some concerns about collisions in base::CreateTemporaryFile, so I > > am returning ScopedTempDir > > I don't think CreateTemporaryFile() will have collisions and return false very > often, but ScopedTempDir + CreateTemporaryFile() is fine. If some process, not even Chrome leak files after system GetTempFileName, it needs just 2^16 to permanently fail. Even less required be signifficantly slowdown ::GetTempFileName function. https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:176: // Use global object to avoid tracking all opened files. On 2014/09/16 19:57:36, Lei Zhang wrote: > I'm slightly confused by this comment. Aren't you still using ScopedTempFile to > keep track of the returned file from CreateTempFile()? Are you trying to just > keep everything in its own directory? I don't want global TEMP dir, to avoid polluted dir 2^16 temp files, and collisions. Same as in service host. ScopedTempFile is just to delete base::File on FILE thread, and nothing for temp dir. We need to delete ScopedTempDir after all ScopedTempFile are gone. I can update ScopedTempDir and make it track both: base::File and ScopedTempFile. I don't like this approach because I would need to share ScopedTempDir between different calls to CreateTempFile(). It would complicate the code a little. Single temp dir for process should be good enough. I can create a patch to show how it looks. https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:265: // Store callback before any OnFailed() calles to make it called on failure. On 2014/09/16 19:57:36, Lei Zhang wrote: > typo: calls, same on line 333 Done.
https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:176: // Use global object to avoid tracking all opened files. On 2014/09/16 22:10:37, Vitaly Buka wrote: > On 2014/09/16 19:57:36, Lei Zhang wrote: > > I'm slightly confused by this comment. Aren't you still using ScopedTempFile > to > > keep track of the returned file from CreateTempFile()? Are you trying to just > > keep everything in its own directory? > > I don't want global TEMP dir, to avoid polluted dir 2^16 temp files, and > collisions. Same as in service host. > > ScopedTempFile is just to delete base::File on FILE thread, and nothing for temp > dir. > > We need to delete ScopedTempDir after all ScopedTempFile are gone. > > I can update ScopedTempDir and make it track both: base::File and > ScopedTempFile. > > I don't like this approach because I would need to share ScopedTempDir between > different calls to CreateTempFile(). It would complicate the code a little. > Single temp dir for process should be good enough. > > I can create a patch to show how it looks. Oh, is the problem that ScopedTempDir doesn't work properly on Windows because it won't actually delete the temp dir if there are files in it that are open? Thus one would need to track the files in that folder that are open, and that's a lot of work?
On 2014/09/16 22:40:00, Lei Zhang wrote: > https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... > File chrome/browser/printing/pdf_to_emf_converter.cc (right): > > https://codereview.chromium.org/566693002/diff/310001/chrome/browser/printing... > chrome/browser/printing/pdf_to_emf_converter.cc:176: // Use global object to > avoid tracking all opened files. > On 2014/09/16 22:10:37, Vitaly Buka wrote: > > On 2014/09/16 19:57:36, Lei Zhang wrote: > > > I'm slightly confused by this comment. Aren't you still using ScopedTempFile > > to > > > keep track of the returned file from CreateTempFile()? Are you trying to > just > > > keep everything in its own directory? > > > > I don't want global TEMP dir, to avoid polluted dir 2^16 temp files, and > > collisions. Same as in service host. > > > > ScopedTempFile is just to delete base::File on FILE thread, and nothing for > temp > > dir. > > > > We need to delete ScopedTempDir after all ScopedTempFile are gone. > > > > I can update ScopedTempDir and make it track both: base::File and > > ScopedTempFile. > > > > I don't like this approach because I would need to share ScopedTempDir between > > different calls to CreateTempFile(). It would complicate the code a little. > > Single temp dir for process should be good enough. > > > > I can create a patch to show how it looks. > > Oh, is the problem that ScopedTempDir doesn't work properly on Windows because > it won't actually delete the temp dir if there are files in it that are open? > Thus one would need to track the files in that folder that are open, and that's > a lot of work? Yes. Windows can't delete dirs with opened files. Please compare #9 and #10. Probably #10 is not so bad.
On 2014/09/16 22:43:48, Vitaly Buka wrote: > Yes. Windows can't delete dirs with opened files. > Please compare #9 and #10. Probably #10 is not so bad. It'll be helpful to document this in the comments. This is fresh on our minds now, but we may forget in the future. I'll look at patch set 10 now.
On 2014/09/16 22:47:22, Lei Zhang wrote: > On 2014/09/16 22:43:48, Vitaly Buka wrote: > > Yes. Windows can't delete dirs with opened files. > > Please compare #9 and #10. Probably #10 is not so bad. > > It'll be helpful to document this in the comments. This is fresh on our minds > now, but we may forget in the future. I'll look at patch set 10 now. BTW, I never tried FILE_SHARE_DELETE. Maybe this work.
lgtm https://codereview.chromium.org/566693002/diff/350001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/350001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:44: }; DISALLOW_COPY_AND_ASSIGN()
https://codereview.chromium.org/566693002/diff/350001/chrome/browser/printing... File chrome/browser/printing/pdf_to_emf_converter.cc (right): https://codereview.chromium.org/566693002/diff/350001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:30: class RefCountedTempDir Documented. https://codereview.chromium.org/566693002/diff/350001/chrome/browser/printing... chrome/browser/printing/pdf_to_emf_converter.cc:44: }; On 2014/09/16 22:52:23, Lei Zhang wrote: > DISALLOW_COPY_AND_ASSIGN() Done.
vitalybuka@chromium.org changed reviewers: + jln@chromium.org
+jln for ServiceSandboxedProcessLauncherDelegate in chrome/service/service_utility_process_host.cc and chrome/common/chrome_utility_printing_messages.h
vitalybuka@chromium.org changed reviewers: + tsepez@chromium.org
+tsepez Also for ServiceSandboxedProcessLauncherDelegate in chrome/service/service_utility_process_host.cc Here the only change is removed exposing to sandbox a temp directory. and chrome/common/chrome_utility_printing_messages.h
+wfh for windows implications in service_utility_process_host.cc I didn't see where the filenames/directory names come from. How much control does one have over these? Messages are fine. https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... File chrome/service/service_utility_process_host.cc (right): https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... chrome/service/service_utility_process_host.cc:138: base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE | nit: maybe one flag per line, also check indentation style. https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... chrome/service/service_utility_process_host.cc:174: if (!pdf_file.IsValid() || !StartProcess(false)) Do we leave temporary files lying aroung if startProcess() fails? https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... File chrome/service/service_utility_process_host.h (right): https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... chrome/service/service_utility_process_host.h:109: bool Send(IPC::Message* msg); Is this overriding IPC::Sender::Send()? If so, declare as virtual / OVERRIDE?
tsepez@chromium.org changed reviewers: + wfh@chromium.org
Actually add @wfh this time.
On 2014/09/17 16:59:24, Tom Sepez wrote: > +wfh for windows implications in service_utility_process_host.cc > > I didn't see where the filenames/directory names come from. How much control > does one have over these? Not sure what do you mean. Now I don't pass filenames or dirs, it's pure handles. All files create with base::CreateTemporaryFileInDir. > > Messages are fine. > > https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... > File chrome/service/service_utility_process_host.cc (right): > > https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... > chrome/service/service_utility_process_host.cc:138: > base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE | > nit: maybe one flag per line, also check indentation style. > > https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... > chrome/service/service_utility_process_host.cc:174: if (!pdf_file.IsValid() || > !StartProcess(false)) > Do we leave temporary files lying aroung if startProcess() fails? > > https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... > File chrome/service/service_utility_process_host.h (right): > > https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... > chrome/service/service_utility_process_host.h:109: bool Send(IPC::Message* msg); > Is this overriding IPC::Sender::Send()? If so, declare as virtual / OVERRIDE?
https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... File chrome/service/service_utility_process_host.cc (right): https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... chrome/service/service_utility_process_host.cc:138: base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE | On 2014/09/17 16:59:24, Tom Sepez wrote: > nit: maybe one flag per line, also check indentation style. Done. https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... chrome/service/service_utility_process_host.cc:174: if (!pdf_file.IsValid() || !StartProcess(false)) On 2014/09/17 16:59:24, Tom Sepez wrote: > Do we leave temporary files lying aroung if startProcess() fails? No. Files are open with base::File::FLAG_DELETE_ON_CLOSE base::File automatically close on return and file is deleted. https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... File chrome/service/service_utility_process_host.h (right): https://codereview.chromium.org/566693002/diff/370001/chrome/service/service_... chrome/service/service_utility_process_host.h:109: bool Send(IPC::Message* msg); On 2014/09/17 16:59:24, Tom Sepez wrote: > Is this overriding IPC::Sender::Send()? If so, declare as virtual / OVERRIDE? No. It's just helper non virtual method. It's can be hidden in private section.
LGTM for sandbox policies in service_utility_process_host.cc - they don't alter the security behavior, in fact removing the exposed_dir_ improves sandbox security by removing an exception.
Messages LGTM.
The CQ bit was checked by vitalybuka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/566693002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalybuka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/566693002/380001
Message was sent while issue was closed.
Committed patchset #12 (id:380001) as eb3b645e9c6996c478f51256d167f0a8e52094ae
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/34cea56676e4d4cad6710906c23c5d5260d12be7 Cr-Commit-Position: refs/heads/master@{#295360} |
