Convert PDF to PWG Raster using utility process.

Convert PDF to PWG Raster using utility process.

BUG=318373
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236959

[Vitaly Buka (NO REVIEWS), 2013-11-21 18:19:44]

Fixed snapshot

[gene, 2013-11-21 18:47:27]

lgtm
tiny nit below

https://codereview.chromium.org/79363002/diff/180001/chrome/browser/local_dis...
File chrome/browser/local_discovery/pwg_raster_converter.cc (right):

https://codereview.chromium.org/79363002/diff/180001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:86:
base::PLATFORM_FILE_READ,
indentaion

[Vitaly Buka (NO REVIEWS), 2013-11-21 19:39:38]

https://codereview.chromium.org/79363002/diff/180001/chrome/browser/local_dis...
File chrome/browser/local_discovery/pwg_raster_converter.cc (right):

https://codereview.chromium.org/79363002/diff/180001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:86:
base::PLATFORM_FILE_READ,
On 2013/11/21 18:47:28, gene wrote:
> indentaion

Done.

[Lei Zhang, 2013-11-21 20:51:04]

https://codereview.chromium.org/79363002/diff/240001/chrome/browser/local_dis...
File chrome/browser/local_discovery/pwg_raster_converter.cc (right):

https://codereview.chromium.org/79363002/diff/240001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:16: #include
"printing/units.h"
not needed?

https://codereview.chromium.org/79363002/diff/240001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:99: class
PwgUtilityProcessHostClient : public content::UtilityProcessHostClient {
Might be good to document what parts live on what thread, and if anything is
accessed on multiple threads.

https://codereview.chromium.org/79363002/diff/240001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/79363002/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:642:
data.resize(base::ReadPlatformFile(pdf_file, 0, &data[0], data.size()));
Can't ReadPlatformFile() fail for some reason and return -1?

https://codereview.chromium.org/79363002/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:670:
!base::WritePlatformFile(bitmap_file, 0, &pwg_page.front(),
Doesn't this overwrite |bitmap_file| on every loop?

[Cris Neckar, 2013-11-21 22:48:27]

per vitaly's request.

IPC changes LGTM with Lei's comments

[Vitaly Buka (NO REVIEWS), 2013-11-22 00:13:00]

Thanks for review.

https://codereview.chromium.org/79363002/diff/240001/chrome/browser/local_dis...
File chrome/browser/local_discovery/pwg_raster_converter.cc (right):

https://codereview.chromium.org/79363002/diff/240001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:16: #include
"printing/units.h"
On 2013/11/21 20:51:04, Lei Zhang wrote:
> not needed?

Done.

https://codereview.chromium.org/79363002/diff/240001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:99: class
PwgUtilityProcessHostClient : public content::UtilityProcessHostClient {
On 2013/11/21 20:51:04, Lei Zhang wrote:
> Might be good to document what parts live on what thread, and if anything is
> accessed on multiple threads.

Done.

https://codereview.chromium.org/79363002/diff/240001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/79363002/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:642:
data.resize(base::ReadPlatformFile(pdf_file, 0, &data[0], data.size()));
On 2013/11/21 20:51:04, Lei Zhang wrote:
> Can't ReadPlatformFile() fail for some reason and return -1?

Done.

https://codereview.chromium.org/79363002/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:670:
!base::WritePlatformFile(bitmap_file, 0, &pwg_page.front(),
On 2013/11/21 20:51:04, Lei Zhang wrote:
> Doesn't this overwrite |bitmap_file| on every loop?
File is opened in APPEND mode, so offset is ignored.

but WritePlatformFileAtCurrentPos without APPEND is more appropriate here.

[Lei Zhang, 2013-11-22 01:52:49]

lgtm

https://codereview.chromium.org/79363002/diff/340001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/79363002/diff/340001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:645: data.resize(data_size);
Isn't this a no-op?

[Lei Zhang, 2013-11-22 01:57:24]

https://codereview.chromium.org/79363002/diff/340001/chrome/browser/local_dis...
File chrome/browser/local_discovery/pwg_raster_converter.cc (right):

https://codereview.chromium.org/79363002/diff/340001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:196:
utility_process_host_.reset();
Actually, is this suppose to be there?

[Vitaly Buka (NO REVIEWS), 2013-11-22 08:10:05]

https://codereview.chromium.org/79363002/diff/340001/chrome/browser/local_dis...
File chrome/browser/local_discovery/pwg_raster_converter.cc (right):

https://codereview.chromium.org/79363002/diff/340001/chrome/browser/local_dis...
chrome/browser/local_discovery/pwg_raster_converter.cc:196:
utility_process_host_.reset();
On 2013/11/22 01:57:24, Lei Zhang wrote:
> Actually, is this suppose to be there?

I guess it's safer. it's weak ptr, so it does not control lifetime but after
this point this client should not touch host.

https://codereview.chromium.org/79363002/diff/340001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/79363002/diff/340001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:645: data.resize(data_size);
On 2013/11/22 01:52:50, Lei Zhang wrote:
> Isn't this a no-op?

Done.

[commit-bot: I haz the power, 2013-11-22 08:10:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/79363002/430001

[commit-bot: I haz the power, 2013-11-22 08:26:35]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-22 18:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/79363002/680001

[commit-bot: I haz the power, 2013-11-22 19:17:42]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-22 23:43:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/79363002/1060001

[commit-bot: I haz the power, 2013-11-23 00:33:52]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-11-23 01:00:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/79363002/1230001

[commit-bot: I haz the power, 2013-11-23 20:27:27]

Change committed as 236959