Add PDF compositor service

Add PDF compositor service

Currently, the service converts one or multiple pages encapsulated in a
SkMultiPictureDocument to a PDF file. It runs in a sandboxed utility process.

This allows Chromium to move PDF generation code into a separate
process, and eventually can support compositing content generated from
multiple processes.

BUG=455764
Review-Url: https://codereview.chromium.org/2832633002
Cr-Commit-Position: refs/heads/master@{#473644}
Committed: https://chromium.googlesource.com/chromium/src/+/fabbf757ba1f0fe03a5671eb0bb4936efbb4a890

[Wei Li, 2017-04-20 00:02:45]

Patchset #3 (id:40001) has been deleted

[Wei Li, 2017-04-20 21:15:30]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 21:21:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:21:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-04-20 22:14:22]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 22:15:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 22:17:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 22:17:34]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Wei Li, 2017-04-21 17:26:42]

Description was changed from

==========
Add PDF compositor service

Currently, the service converts one or multiple pages in the format of
SkPicture to a PDF file. It runs in a sandboxed utility process.
This allows Chromium to move PDF generation code into a separate
process, and eventually can support compositing content generated from
multiple processes.

BUG=455764
==========

to

==========
Add PDF compositor service

Currently, the service converts one or multiple pages encapsulated in a
SkMultiPictureDocument to a PDF file. It runs in a sandboxed utility process.

This allows Chromium to move PDF generation code into a separate
process, and eventually can support compositing content generated from
multiple processes.

BUG=455764
==========

[Wei Li, 2017-04-21 17:29:10]

weili@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org, thestig@chromium.org

[Wei Li, 2017-04-21 17:29:11]

Per our discussion, moved this service to components/printing/service. Pls
review, thanks!

[Lei Zhang, 2017-04-22 00:12:26]

Did we decide it was ok to put more printing bits in content/ ?

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1529: "//components/printing/service/public/interfaces",
Should this be down in the "if (enable_basic_printing || enable_print_preview)"
block?

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:142: #include
"components/printing/service/public/interfaces/pdf_compositor.mojom.h"
Also ifdef'd?

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:25: SkTime::DateTime
TimeToSkTime(base::Time time) {
Do we want to reuse the existing code in printing/pdf_metafile_skia.cc somehow?

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:83: DLOG(ERROR) <<
"OnBufferCreated: Map failed.";
Do we need to run |callback| here and anywhere else below where we fail?

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_service.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_service.cc:65: ref_factory_.reset(new
service_manager::ServiceContextRefFactory(
foo_unique_ptr.reset(new Foo()) -> foo_unique_ptr = base::MakeUnique<Foo>();

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_service.h (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_service.h:11: #include
"base/callback.h"
Not used. Can we (only) IWYU?

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_service.h:43: static
base::SingleThreadTaskRunner* io_task_runner_;
Why is this static and not per object?

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/public/cpp/pdf_compositor_client.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:45: return;
Do we need to run |callback| here?

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:266: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
Shouldn't the caller call this on the right thread?

[jam, 2017-04-24 15:18:05]

https://codereview.chromium.org/2832633002/diff/100001/content/utility/utilit...
File content/utility/utility_service_factory.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/content/utility/utilit...
content/utility/utility_service_factory.cc:31: // nogncheck because dependency
on //printing is conditional upon
content/ currently doesn't know about components/printing, since that's at the
chrome layer.

can we keep the same layering, i.e. have this printing service be registered by
src/chrome/utility?

[hal.canary, 2017-04-24 19:13:16]

halcanary@google.com changed reviewers:
+ halcanary@google.com

[hal.canary, 2017-04-24 19:13:17]

Skia-specific code in pdf_compositor_impl.cc looks good to me.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:21: #include
"third_party/skia/src/utils/SkMultiPictureDocument.h"
FYI: I am trying to get this API blessed: https://review.skia.org/11138

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:122:
wstream.copyTo(mapping.get());
`wstream.copyToAndReset(mapping.get());`
is the new way of doing this:  it frees memory as it goes and may save physical
memory use.

[Wei Li, 2017-04-26 06:34:43]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 06:35:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 06:45:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 06:45:48]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Wei Li, 2017-04-26 18:06:19]

Patchset #8 (id:160001) has been deleted

[Wei Li, 2017-04-26 18:10:46]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 18:11:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wei Li, 2017-04-26 20:42:35]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:43:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 21:35:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 21:35:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-04-27 05:34:17]

thanks for all the reviews!

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1529: "//components/printing/service/public/interfaces",
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Should this be down in the "if (enable_basic_printing ||
enable_print_preview)"
> block?

Done.

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:142: #include
"components/printing/service/public/interfaces/pdf_compositor.mojom.h"
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Also ifdef'd?

Done.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:21: #include
"third_party/skia/src/utils/SkMultiPictureDocument.h"
On 2017/04/24 19:13:17, hal.canary wrote:
> FYI: I am trying to get this API blessed: https://review.skia.org/11138

Acknowledged.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:25: SkTime::DateTime
TimeToSkTime(base::Time time) {
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Do we want to reuse the existing code in printing/pdf_metafile_skia.cc
somehow?

Done.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:83: DLOG(ERROR) <<
"OnBufferCreated: Map failed.";
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Do we need to run |callback| here and anywhere else below where we fail?

Done.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:122:
wstream.copyTo(mapping.get());
On 2017/04/24 19:13:17, hal.canary wrote:
> `wstream.copyToAndReset(mapping.get());`
> is the new way of doing this:  it frees memory as it goes and may save
physical
> memory use.

Done.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_service.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_service.cc:65: ref_factory_.reset(new
service_manager::ServiceContextRefFactory(
On 2017/04/22 00:12:26, Lei Zhang wrote:
> foo_unique_ptr.reset(new Foo()) -> foo_unique_ptr = base::MakeUnique<Foo>();

Done.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/pdf_compositor_service.h (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_service.h:11: #include
"base/callback.h"
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Not used. Can we (only) IWYU?

Done.

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/pdf_compositor_service.h:43: static
base::SingleThreadTaskRunner* io_task_runner_;
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Why is this static and not per object?

removed

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
File components/printing/service/public/cpp/pdf_compositor_client.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:45: return;
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Do we need to run |callback| here?

Changed.

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:266: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2017/04/22 00:12:26, Lei Zhang wrote:
> Shouldn't the caller call this on the right thread?

Right now, all the utility functions are called on IO thread. Ours is the first
one needed to be run on UI thread. Not sure whether there is any future needs to
do the same.

https://codereview.chromium.org/2832633002/diff/100001/content/utility/utilit...
File content/utility/utility_service_factory.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/content/utility/utilit...
content/utility/utility_service_factory.cc:31: // nogncheck because dependency
on //printing is conditional upon
On 2017/04/24 15:18:05, jam wrote:
> content/ currently doesn't know about components/printing, since that's at the
> chrome layer.
> 
> can we keep the same layering, i.e. have this printing service be registered
by
> src/chrome/utility?

Done.

[Wei Li, 2017-04-27 05:36:37]

On 2017/04/22 00:12:26, Lei Zhang wrote:
> Did we decide it was ok to put more printing bits in content/ ?
> 

No, it is the opposite. We should put code in components/printing/service since
we depend on content/

[Lei Zhang, 2017-04-27 22:20:12]

On 2017/04/27 05:36:37, Wei Li wrote:
> On 2017/04/22 00:12:26, Lei Zhang wrote:
> > Did we decide it was ok to put more printing bits in content/ ?
> > 
> 
> No, it is the opposite. We should put code in components/printing/service
since
> we depend on content/

I was wonder about what jam@ talked about in message #14. It sounds like the
answer, if no, let's not add more printing bits in content/, and try to move
them into chrome/ instead.

[Lei Zhang, 2017-04-27 22:42:22]

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:266: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2017/04/27 05:34:16, Wei Li wrote:
> On 2017/04/22 00:12:26, Lei Zhang wrote:
> > Shouldn't the caller call this on the right thread?
> 
> Right now, all the utility functions are called on IO thread. Ours is the
first
> one needed to be run on UI thread. Not sure whether there is any future needs
to
> do the same.

Whether it's the first or not, can we have callers call functions on the right
thread, instead of having functions that handle it for the callers?

https://codereview.chromium.org/2832633002/diff/220001/chrome/utility/BUILD.gn
File chrome/utility/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/chrome/utility/BUILD.g...
chrome/utility/BUILD.gn:178: if (enable_basic_printing || enable_print_preview)
{
Just enable_printing then?

https://codereview.chromium.org/2832633002/diff/220001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/220001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:293:
RegisterMojoServicesOnUIThread(registry.get());
Does this have to finish on the UI thread, before AddConnectionFilter() ?
Assuming we are on the IO thread here, likely the actual execution order will be
PostTask(), AddConnectionFilter(), then whatever
RegisterMojoServicesOnUIThread() really needs to do.

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/BUI...
File content/public/app/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/BUI...
content/public/app/BUILD.gn:184:
"//components/printing/service:pdf_compositor_manifest",
Can this be moved into chrome/ now?

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/moj...
File content/public/app/mojo/content_utility_manifest.json (right):

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/moj...
content/public/app/mojo/content_utility_manifest.json:22: "content_browser": [
Isn't this the same as line 17?

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD.gn
File printing/common/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD....
printing/common/BUILD.gn:5: source_set("common") {
Is printing/common meant to be code that can run in any process?

https://codereview.chromium.org/2832633002/diff/220001/printing/common/pdf_me...
File printing/common/pdf_metafile_utils.h (right):

https://codereview.chromium.org/2832633002/diff/220001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.h:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
No (c)

[Wei Li, 2017-04-28 05:22:55]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 05:23:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 07:37:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 07:37:17]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-28 15:49:10]

https://codereview.chromium.org/2832633002/diff/260001/content/public/app/BUI...
File content/public/app/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/260001/content/public/app/BUI...
content/public/app/BUILD.gn:184:
"//components/printing/service:pdf_compositor_manifest",
should this be in chrome?

https://codereview.chromium.org/2832633002/diff/260001/content/public/browser...
File content/public/browser/utility_process_host.h (right):

https://codereview.chromium.org/2832633002/diff/260001/content/public/browser...
content/public/browser/utility_process_host.h:95: class ConnectionFilterImpl;
why does this need to be here?

https://codereview.chromium.org/2832633002/diff/260001/content/public/utility...
File content/public/utility/utility_thread.h (right):

https://codereview.chromium.org/2832633002/diff/260001/content/public/utility...
content/public/utility/utility_thread.h:23: virtual
scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner() = 0;
since RenderThread (another class derived from ChildThread) also has this, can
you promote this method to ChildThread and remove the declaration from here and
RenderThread?

[Wei Li, 2017-04-28 20:12:22]

Patchset #13 (id:280001) has been deleted

[Wei Li, 2017-04-28 20:18:22]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 20:19:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 20:53:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 20:53:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-04-29 00:10:25]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:11:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 02:23:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-29 02:23:44]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-04-29 04:29:21]

On 2017/04/27 22:20:12, Lei Zhang wrote:
> On 2017/04/27 05:36:37, Wei Li wrote:
> > On 2017/04/22 00:12:26, Lei Zhang wrote:
> > > Did we decide it was ok to put more printing bits in content/ ?
> > > 
> > 
> > No, it is the opposite. We should put code in components/printing/service
> since
> > we depend on content/
> 
> I was wonder about what jam@ talked about in message #14. It sounds like the
> answer, if no, let's not add more printing bits in content/, and try to move
> them into chrome/ instead.

Sure, that is the plan.

[Wei Li, 2017-04-29 04:35:18]

ptal, thanks

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/100001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:266: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2017/04/27 22:42:21, Lei Zhang wrote:
> On 2017/04/27 05:34:16, Wei Li wrote:
> > On 2017/04/22 00:12:26, Lei Zhang wrote:
> > > Shouldn't the caller call this on the right thread?
> > 
> > Right now, all the utility functions are called on IO thread. Ours is the
> first
> > one needed to be run on UI thread. Not sure whether there is any future
needs
> to
> > do the same.
> 
> Whether it's the first or not, can we have callers call functions on the right
> thread, instead of having functions that handle it for the callers?

oh, now I got it. Hope it looks better now.

https://codereview.chromium.org/2832633002/diff/220001/chrome/utility/BUILD.gn
File chrome/utility/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/chrome/utility/BUILD.g...
chrome/utility/BUILD.gn:178: if (enable_basic_printing || enable_print_preview)
{
On 2017/04/27 22:42:21, Lei Zhang wrote:
> Just enable_printing then?

We only have enable_printing as buildflag, but not gn args. We can add it if
this becomes too cumbersome.

https://codereview.chromium.org/2832633002/diff/220001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/220001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:293:
RegisterMojoServicesOnUIThread(registry.get());
On 2017/04/27 22:42:21, Lei Zhang wrote:
> Does this have to finish on the UI thread, before AddConnectionFilter() ?
> Assuming we are on the IO thread here, likely the actual execution order will
be
> PostTask(), AddConnectionFilter(), then whatever
> RegisterMojoServicesOnUIThread() really needs to do.

The order shouldn't matter. I changed it anyway based on your previous comment.

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/BUI...
File content/public/app/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/BUI...
content/public/app/BUILD.gn:184:
"//components/printing/service:pdf_compositor_manifest",
On 2017/04/27 22:42:21, Lei Zhang wrote:
> Can this be moved into chrome/ now?

Yep, done in the following patch.

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/moj...
File content/public/app/mojo/content_utility_manifest.json (right):

https://codereview.chromium.org/2832633002/diff/220001/content/public/app/moj...
content/public/app/mojo/content_utility_manifest.json:22: "content_browser": [
On 2017/04/27 22:42:21, Lei Zhang wrote:
> Isn't this the same as line 17?

Good eyes, someone added before me :)

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD.gn
File printing/common/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD....
printing/common/BUILD.gn:5: source_set("common") {
On 2017/04/27 22:42:21, Lei Zhang wrote:
> Is printing/common meant to be code that can run in any process?

In general, common/ directory contains code shared by browser and renderer. This
source_set contains code independent to other parts of code except base and
skia, so it can be run in any process.

https://codereview.chromium.org/2832633002/diff/220001/printing/common/pdf_me...
File printing/common/pdf_metafile_utils.h (right):

https://codereview.chromium.org/2832633002/diff/220001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.h:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
On 2017/04/27 22:42:22, Lei Zhang wrote:
> No (c)

Done.

https://codereview.chromium.org/2832633002/diff/260001/content/public/app/BUI...
File content/public/app/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/260001/content/public/app/BUI...
content/public/app/BUILD.gn:184:
"//components/printing/service:pdf_compositor_manifest",
On 2017/04/28 15:49:10, jam wrote:
> should this be in chrome?

Done.

https://codereview.chromium.org/2832633002/diff/260001/content/public/browser...
File content/public/browser/utility_process_host.h (right):

https://codereview.chromium.org/2832633002/diff/260001/content/public/browser...
content/public/browser/utility_process_host.h:95: class ConnectionFilterImpl;
On 2017/04/28 15:49:10, jam wrote:
> why does this need to be here?

Moved to Impl.

https://codereview.chromium.org/2832633002/diff/260001/content/public/utility...
File content/public/utility/utility_thread.h (right):

https://codereview.chromium.org/2832633002/diff/260001/content/public/utility...
content/public/utility/utility_thread.h:23: virtual
scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner() = 0;
On 2017/04/28 15:49:10, jam wrote:
> since RenderThread (another class derived from ChildThread) also has this, can
> you promote this method to ChildThread and remove the declaration from here
and
> RenderThread?

Good idea. I moved it to ChildThread, but there is a subtle difference b/w the
implementation of RenderThreadImpl and ChildThreadImpl, so still leave them
there. Ideally, we can figure out why the difference and merge into one.

[jam, 2017-05-01 15:42:14]

lgtm

Ken: ptal at utility_process_host_impl.cc

[Ken Rockot(use gerrit already), 2017-05-01 15:54:45]

lgtm

[Wei Li, 2017-05-04 00:09:24]

Lei, ptal, thanks

[Lei Zhang, 2017-05-04 01:31:10]

I haven't finished looking yet, but this doesn't build at ToT anymore. Looks
like ben@ made more Mojo changes this week.

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD.gn
File printing/common/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD....
printing/common/BUILD.gn:5: source_set("common") {
On 2017/04/29 04:35:18, Wei Li wrote:
> On 2017/04/27 22:42:21, Lei Zhang wrote:
> > Is printing/common meant to be code that can run in any process?
> 
> In general, common/ directory contains code shared by browser and renderer.
This
> source_set contains code independent to other parts of code except base and
> skia, so it can be run in any process. 

I guess we can start splitting up printing/ a bit. Traditionally it's been just
a mixed bag.

https://codereview.chromium.org/2832633002/diff/360001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2832633002/diff/360001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3335:
{printing::mojom::kServiceName, IDR_PDF_COMPOSITOR_MANIFEST},
nit: The indentation on line 3331 looks off. It and this line should be indented
line line 3329.

https://codereview.chromium.org/2832633002/diff/360001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3336: #endif  //
BUILDFLAG(ENABLE_PRINTING)
nit: No need for comment, since this isn't nested like the #ifs above.

https://codereview.chromium.org/2832633002/diff/360001/printing/common/pdf_me...
File printing/common/pdf_metafile_utils.cc (right):

https://codereview.chromium.org/2832633002/diff/360001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.cc:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
No (c).

[Lei Zhang, 2017-05-04 02:13:14]

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/README.md (right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/README.md:1: The pdf_compositor service composites
multiple raw pictures from different
Can we document the discrepency between what we want pdf_compositor to do and
what this implementation does?

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/pdf_compositor_service.h (right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/pdf_compositor_service.h:15: #include
"base/single_thread_task_runner.h"
Not used?

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/public/cpp/pdf_compositor_service_factory.cc
(right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_service_factory.cc:8:
#include "content/public/common/content_client.h"
No needed.

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/public/interfaces/pdf_compositor.mojom (right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:13: //
|sk_handle| points to a buffer of a number of serialized page image data.
Does it make sense to mention the format? MSKP?

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:14: //       
     The content is obtained from skia::SerializePages().
skia::SerializePages() doesn't exist.

https://codereview.chromium.org/2832633002/diff/360001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/360001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:138: ConnectionFilterImpl(
explicit?

https://codereview.chromium.org/2832633002/diff/360001/content/public/browser...
File content/public/browser/utility_process_host.h (right):

https://codereview.chromium.org/2832633002/diff/360001/content/public/browser...
content/public/browser/utility_process_host.h:88: // Register mojo services that
the utility process can use.
Mention this needs to run on the UI thread.

https://codereview.chromium.org/2832633002/diff/360001/content/public/browser...
content/public/browser/utility_process_host.h:89: virtual void
RegisterMojoServices() = 0;
It's weird to have this method as is:

1) The impl can be static.
2) The rest of the class runs on the IO thread. This is a bit of an odd ball as
a class method.

Can it leave elsewhere? Is it really tied to UtilityProcessHost?

[Lei Zhang, 2017-05-04 02:13:28]

On 2017/05/04 02:13:14, Lei Zhang wrote:
> Can it leave elsewhere? Is it really tied to UtilityProcessHost?

*live*

[Wei Li, 2017-05-04 07:40:43]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 07:40:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 07:45:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 07:45:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-05-04 16:25:18]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 16:25:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-05-04 17:27:18]

https://codereview.chromium.org/2832633002/diff/420001/content/browser/utilit...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/2832633002/diff/420001/content/browser/utilit...
content/browser/utility_process_host_impl.h:63: void RegisterMojoServices()
override;
Agree with thestig@'s comments. This doesn't need to be tied to
UtilityProcessHostImpl.

RenderProcessHostImpl listens for the same memory-related interfaces using its
own ConnectionFilter, but that too is unnecessary. We should move the filter to
some common location so that *all* connections to the browser can bind it.

AFAICT though your patch should not depend on this stuff working for utility
processes -- the error messages that result during utility process launch are
innocuous and can be ignored -- so we should just treat this as a separate bug
and not address it here.

[commit-bot: I haz the power, 2017-05-04 17:53:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 17:53:28]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-05-04 18:18:29]

ptal, thanks a lot!

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD.gn
File printing/common/BUILD.gn (right):

https://codereview.chromium.org/2832633002/diff/220001/printing/common/BUILD....
printing/common/BUILD.gn:5: source_set("common") {
On 2017/05/04 01:31:09, Lei Zhang wrote:
> On 2017/04/29 04:35:18, Wei Li wrote:
> > On 2017/04/27 22:42:21, Lei Zhang wrote:
> > > Is printing/common meant to be code that can run in any process?
> > 
> > In general, common/ directory contains code shared by browser and renderer.
> This
> > source_set contains code independent to other parts of code except base and
> > skia, so it can be run in any process. 
> 
> I guess we can start splitting up printing/ a bit. Traditionally it's been
just
> a mixed bag.

Acknowledged.

https://codereview.chromium.org/2832633002/diff/360001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2832633002/diff/360001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3335:
{printing::mojom::kServiceName, IDR_PDF_COMPOSITOR_MANIFEST},
On 2017/05/04 01:31:09, Lei Zhang wrote:
> nit: The indentation on line 3331 looks off. It and this line should be
indented
> line line 3329.

I noticed this too. Our formatting script made this.

https://codereview.chromium.org/2832633002/diff/360001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3336: #endif  //
BUILDFLAG(ENABLE_PRINTING)
On 2017/05/04 01:31:10, Lei Zhang wrote:
> nit: No need for comment, since this isn't nested like the #ifs above.

Done.

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/README.md (right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/README.md:1: The pdf_compositor service composites
multiple raw pictures from different
On 2017/05/04 02:13:13, Lei Zhang wrote:
> Can we document the discrepency between what we want pdf_compositor to do and
> what this implementation does?

Done.

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/pdf_compositor_service.h (right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/pdf_compositor_service.h:15: #include
"base/single_thread_task_runner.h"
On 2017/05/04 02:13:13, Lei Zhang wrote:
> Not used?

Removed.

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/public/cpp/pdf_compositor_service_factory.cc
(right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_service_factory.cc:8:
#include "content/public/common/content_client.h"
On 2017/05/04 02:13:13, Lei Zhang wrote:
> No needed.

Done.

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
File components/printing/service/public/interfaces/pdf_compositor.mojom (right):

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:13: //
|sk_handle| points to a buffer of a number of serialized page image data.
On 2017/05/04 02:13:13, Lei Zhang wrote:
> Does it make sense to mention the format? MSKP?

Done.

https://codereview.chromium.org/2832633002/diff/360001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:14: //       
     The content is obtained from skia::SerializePages().
On 2017/05/04 02:13:13, Lei Zhang wrote:
> skia::SerializePages() doesn't exist.

Removed

https://codereview.chromium.org/2832633002/diff/360001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/360001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:138: ConnectionFilterImpl(
On 2017/05/04 02:13:13, Lei Zhang wrote:
> explicit?

Done.

https://codereview.chromium.org/2832633002/diff/360001/content/public/browser...
File content/public/browser/utility_process_host.h (right):

https://codereview.chromium.org/2832633002/diff/360001/content/public/browser...
content/public/browser/utility_process_host.h:88: // Register mojo services that
the utility process can use.
On 2017/05/04 02:13:13, Lei Zhang wrote:
> Mention this needs to run on the UI thread.

Done.

https://codereview.chromium.org/2832633002/diff/360001/content/public/browser...
content/public/browser/utility_process_host.h:89: virtual void
RegisterMojoServices() = 0;
On 2017/05/04 02:13:13, Lei Zhang wrote:
> It's weird to have this method as is:
> 
> 1) The impl can be static.
> 2) The rest of the class runs on the IO thread. This is a bit of an odd ball
as
> a class method.
> 
> Can it leave elsewhere? Is it really tied to UtilityProcessHost?

As Ken said, once we can refactor those memory interfaces to some common
location, we can remove this.

https://codereview.chromium.org/2832633002/diff/360001/printing/common/pdf_me...
File printing/common/pdf_metafile_utils.cc (right):

https://codereview.chromium.org/2832633002/diff/360001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.cc:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
On 2017/05/04 01:31:10, Lei Zhang wrote:
> No (c).

Done.

https://codereview.chromium.org/2832633002/diff/420001/content/browser/utilit...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/2832633002/diff/420001/content/browser/utilit...
content/browser/utility_process_host_impl.h:63: void RegisterMojoServices()
override;
On 2017/05/04 17:27:18, Ken Rockot wrote:
> Agree with thestig@'s comments. This doesn't need to be tied to
> UtilityProcessHostImpl.
> 
> RenderProcessHostImpl listens for the same memory-related interfaces using its
> own ConnectionFilter, but that too is unnecessary. We should move the filter
to
> some common location so that *all* connections to the browser can bind it.
> 
> AFAICT though your patch should not depend on this stuff working for utility
> processes -- the error messages that result during utility process launch are
> innocuous and can be ignored -- so we should just treat this as a separate bug
> and not address it here.

Acknowledged.

[Lei Zhang, 2017-05-04 19:31:28]

lgtm

[Wei Li, 2017-05-04 20:49:38]

weili@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org

[Wei Li, 2017-05-04 20:49:39]

nasko@ or dcheng@, can you have a security review on ipc changes? thanks

[Wei Li, 2017-05-04 23:34:06]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 23:34:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 02:10:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 02:10:10]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-05 18:28:10]

dcheng@chromium.org changed reviewers:
- nasko@chromium.org

[dcheng, 2017-05-05 18:28:11]

On 2017/05/04 20:49:39, Wei Li wrote:
> nasko@ or dcheng@, can you have a security review on ipc changes? thanks

In the future, please just pick one security reviewer. If you need an urgent
review, you can always ping on chat first.
nasko to cc, I'll review this

[Wei Li, 2017-05-06 19:27:51]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 19:27:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 19:54:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-06 19:54:57]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Wei Li, 2017-05-06 22:59:20]

Patchset #23 (id:500001) has been deleted

[Wei Li, 2017-05-06 23:01:21]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 23:01:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-07 00:27:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-07 00:27:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-05-07 06:09:26]

Patchset #22 (id:480001) has been deleted

[Wei Li, 2017-05-07 06:09:47]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-07 06:09:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-07 07:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-07 07:24:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-05-08 01:24:05]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 01:24:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 02:45:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 02:45:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wei Li, 2017-05-09 16:44:32]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 16:45:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 18:57:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 18:57:51]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-10 07:36:48]

https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3300: services->insert(
Nit: let's use emplace(printing::mojom::kServiceName, base::ASCIIToUTF16(...))

https://codereview.chromium.org/2832633002/diff/580001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:335: services->insert(
Ditto: let's use emplace() now

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:67:
MakePdfDocument(creator_.size() ? creator_ : "Chromium", &wstream);
Nit: .empty() instead of .size()

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/pdf_compositor_service.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/pdf_compositor_service.cc:22:
base::LazyInstance<std::string>::Leaky g_creator;
It feels a bit awkward to hold this as a LazyInstance. Won't subsequent calls to
PdfCompositorService::Create() change this and cause requests for future
PdfCompositors to be created with a different creator?

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/public/cpp/pdf_compositor_client.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:41: uint32_t
data_size,
Any particular reason for uint32_t instead of size_t?

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:46: if
(!compositor_.get())
Nit: no .get()

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:55:
callback_task_runner));
Optional: perhaps use base::Passed() with callback_task_runner, since this is
being passed by value?

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/public/interfaces/pdf_compositor.mojom (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:12: //
Composite content from one or more subframes, and convert into a PDF file.
This comment doesn't seem entirely accurate at the moment, it seems like this is
just about turning one skp into a pdf. So for now, maybe let's just keep the
comment about that, and update it when we add subframe support per the TODO?

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:16: =>
(handle<shared_buffer> pdf_handle);
Realistically, how large do we expect these buffers to be? Is it possible we'll
run into shmem limits by passing this over shared memory?

https://codereview.chromium.org/2832633002/diff/580001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:266:
memory_instrumentation::CoordinatorImpl::GetInstance())));
Not entirely clear to me why memory_instrumentation::CoordinatorImpl was
affected by this CL

https://codereview.chromium.org/2832633002/diff/580001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/2832633002/diff/580001/content/renderer/rende...
content/renderer/render_thread_impl.h:226:
scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner() override;
Nit: methods in .h and .cc files are supposed to be in the same order, so the
.cc file should be updated if we're also moving methods around

https://codereview.chromium.org/2832633002/diff/580001/printing/common/pdf_me...
File printing/common/pdf_metafile_utils.h (right):

https://codereview.chromium.org/2832633002/diff/580001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.h:18: PDF_SKIA_DOCUMENT_TYPE,
As this isn't used in many places, consider making this an enum class in a
followup (and then just removing the _DOCUMENT_TYPE suffix on the enumerator
names)

[Wei Li, 2017-05-10 20:19:00]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 20:20:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 21:13:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:13:19]

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_...)

[Wei Li, 2017-05-11 03:15:15]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 03:16:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 04:36:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:36:18]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-05-11 16:53:02]

thanks, ptal

https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3300: services->insert(
On 2017/05/10 07:36:47, dcheng wrote:
> Nit: let's use emplace(printing::mojom::kServiceName, base::ASCIIToUTF16(...))

Done.

https://codereview.chromium.org/2832633002/diff/580001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:335: services->insert(
On 2017/05/10 07:36:48, dcheng wrote:
> Ditto: let's use emplace() now

Done.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:67:
MakePdfDocument(creator_.size() ? creator_ : "Chromium", &wstream);
On 2017/05/10 07:36:48, dcheng wrote:
> Nit: .empty() instead of .size()

Done.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/pdf_compositor_service.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/pdf_compositor_service.cc:22:
base::LazyInstance<std::string>::Leaky g_creator;
On 2017/05/10 07:36:48, dcheng wrote:
> It feels a bit awkward to hold this as a LazyInstance. Won't subsequent calls
to
> PdfCompositorService::Create() change this and cause requests for future
> PdfCompositors to be created with a different creator?

I have not found a better solution. This string should be constant for a give
product since it stores user agent.
User agent is composed after browser starts. So passing in when creating the
service seems the right moment.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/public/cpp/pdf_compositor_client.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:41: uint32_t
data_size,
On 2017/05/10 07:36:48, dcheng wrote:
> Any particular reason for uint32_t instead of size_t?

Done.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:46: if
(!compositor_.get())
On 2017/05/10 07:36:48, dcheng wrote:
> Nit: no .get()

Done.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/cpp/pdf_compositor_client.cc:55:
callback_task_runner));
On 2017/05/10 07:36:48, dcheng wrote:
> Optional: perhaps use base::Passed() with callback_task_runner, since this is
> being passed by value?

Done.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/public/interfaces/pdf_compositor.mojom (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:12: //
Composite content from one or more subframes, and convert into a PDF file.
On 2017/05/10 07:36:48, dcheng wrote:
> This comment doesn't seem entirely accurate at the moment, it seems like this
is
> just about turning one skp into a pdf. So for now, maybe let's just keep the
> comment about that, and update it when we add subframe support per the TODO?

Updated.

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/public/interfaces/pdf_compositor.mojom:16: =>
(handle<shared_buffer> pdf_handle);
On 2017/05/10 07:36:48, dcheng wrote:
> Realistically, how large do we expect these buffers to be? Is it possible
we'll
> run into shmem limits by passing this over shared memory?

For web pages, skia document buffer is smaller than generated pdf file as it is
more compact.
For generated pdf file, the current code without this CL passes it using shared
memory from renderer to browser without problem. In general, for rich webpages
such as nytimes.com, theverge.com, the size is around a few megabytes.

There is one caveat when we load a large pdf file directly, skia redrawing might
generate large output. In this case, we should not call CompositePdf() [as we
don't need to].

https://codereview.chromium.org/2832633002/diff/580001/content/browser/utilit...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/content/browser/utilit...
content/browser/utility_process_host_impl.cc:266:
memory_instrumentation::CoordinatorImpl::GetInstance())));
On 2017/05/10 07:36:48, dcheng wrote:
> Not entirely clear to me why memory_instrumentation::CoordinatorImpl was
> affected by this CL

It is needed for some memory dump functions. Anyway, now it is added by service
manager so we don't need to worry.

https://codereview.chromium.org/2832633002/diff/580001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/2832633002/diff/580001/content/renderer/rende...
content/renderer/render_thread_impl.h:226:
scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner() override;
On 2017/05/10 07:36:48, dcheng wrote:
> Nit: methods in .h and .cc files are supposed to be in the same order, so the
> .cc file should be updated if we're also moving methods around

Done.

https://codereview.chromium.org/2832633002/diff/580001/printing/common/pdf_me...
File printing/common/pdf_metafile_utils.h (right):

https://codereview.chromium.org/2832633002/diff/580001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.h:18: PDF_SKIA_DOCUMENT_TYPE,
On 2017/05/10 07:36:48, dcheng wrote:
> As this isn't used in many places, consider making this an enum class in a
> followup (and then just removing the _DOCUMENT_TYPE suffix on the enumerator
> names)

Acknowledged.

[dcheng, 2017-05-12 08:14:20]

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
File components/printing/service/pdf_compositor_service.cc (right):

https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
components/printing/service/pdf_compositor_service.cc:22:
base::LazyInstance<std::string>::Leaky g_creator;
On 2017/05/11 16:53:02, Wei Li wrote:
> On 2017/05/10 07:36:48, dcheng wrote:
> > It feels a bit awkward to hold this as a LazyInstance. Won't subsequent
calls
> to
> > PdfCompositorService::Create() change this and cause requests for future
> > PdfCompositors to be created with a different creator?
> 
> I have not found a better solution. This string should be constant for a give
> product since it stores user agent.
> User agent is composed after browser starts. So passing in when creating the
> service seems the right moment.

How about just having a static function to configure this, and have
PdfCompositor just use the static directly? If it's stored as a member on each
PdfCompositorService/PdfCompositor, it kind of looks like it can be different
for different PdfCompositors -- but from this comment, it seems like it's
actually constant.

[Wei Li, 2017-05-12 15:28:47]

On 2017/05/12 08:14:20, dcheng wrote:
>
https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
> File components/printing/service/pdf_compositor_service.cc (right):
> 
>
https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
> components/printing/service/pdf_compositor_service.cc:22:
> base::LazyInstance<std::string>::Leaky g_creator;
> On 2017/05/11 16:53:02, Wei Li wrote:
> > On 2017/05/10 07:36:48, dcheng wrote:
> > > It feels a bit awkward to hold this as a LazyInstance. Won't subsequent
> calls
> > to
> > > PdfCompositorService::Create() change this and cause requests for future
> > > PdfCompositors to be created with a different creator?
> > 
> > I have not found a better solution. This string should be constant for a
give
> > product since it stores user agent.
> > User agent is composed after browser starts. So passing in when creating the
> > service seems the right moment.
> 
> How about just having a static function to configure this, and have
> PdfCompositor just use the static directly? If it's stored as a member on each
> PdfCompositorService/PdfCompositor, it kind of looks like it can be different
> for different PdfCompositors -- but from this comment, it seems like it's
> actually constant.

oh, I might misunderstand your previous comments.
PdfCompositorService::Create() is a static function and will be called only once
during creating the service. 
IIUC, It should only be called once since a service should handle all requests
when it is alive. You can think of it as a singleton.

[dcheng, 2017-05-12 15:54:08]

Actually I thought about this a bit more, and I don't think there's any time the
initial set of flags, origin, etc matter on the initial empty Document in a
provisional frame. So LGTM

[dcheng, 2017-05-12 15:54:29]

On 2017/05/12 15:54:08, dcheng wrote:
> Actually I thought about this a bit more, and I don't think there's any time
the
> initial set of flags, origin, etc matter on the initial empty Document in a
> provisional frame. So LGTM

Argh wrong review. Please ignore.

[dcheng, 2017-05-12 15:55:44]

On 2017/05/12 15:28:47, Wei Li wrote:
> On 2017/05/12 08:14:20, dcheng wrote:
> >
>
https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
> > File components/printing/service/pdf_compositor_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
> > components/printing/service/pdf_compositor_service.cc:22:
> > base::LazyInstance<std::string>::Leaky g_creator;
> > On 2017/05/11 16:53:02, Wei Li wrote:
> > > On 2017/05/10 07:36:48, dcheng wrote:
> > > > It feels a bit awkward to hold this as a LazyInstance. Won't subsequent
> > calls
> > > to
> > > > PdfCompositorService::Create() change this and cause requests for future
> > > > PdfCompositors to be created with a different creator?
> > > 
> > > I have not found a better solution. This string should be constant for a
> give
> > > product since it stores user agent.
> > > User agent is composed after browser starts. So passing in when creating
the
> > > service seems the right moment.
> > 
> > How about just having a static function to configure this, and have
> > PdfCompositor just use the static directly? If it's stored as a member on
each
> > PdfCompositorService/PdfCompositor, it kind of looks like it can be
different
> > for different PdfCompositors -- but from this comment, it seems like it's
> > actually constant.
> 
> oh, I might misunderstand your previous comments.
> PdfCompositorService::Create() is a static function and will be called only
once
> during creating the service. 
> IIUC, It should only be called once since a service should handle all requests
> when it is alive. You can think of it as a singleton.

Right, but what keeps the service alive, etc? Is it obvious to readers that
there's a single static instance? The way it's structured, I'd say that's not
entirely clear. Using a static field to hold the user agent would make that
clear that it's shared between all instances.

[Wei Li, 2017-05-16 00:37:25]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 00:38:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 03:46:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 03:46:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Wei Li, 2017-05-16 17:15:54]

On 2017/05/12 15:55:44, dcheng (in AEST) wrote:
> On 2017/05/12 15:28:47, Wei Li wrote:
> > On 2017/05/12 08:14:20, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
> > > File components/printing/service/pdf_compositor_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2832633002/diff/580001/components/printing/se...
> > > components/printing/service/pdf_compositor_service.cc:22:
> > > base::LazyInstance<std::string>::Leaky g_creator;
> > > On 2017/05/11 16:53:02, Wei Li wrote:
> > > > On 2017/05/10 07:36:48, dcheng wrote:
> > > > > It feels a bit awkward to hold this as a LazyInstance. Won't
subsequent
> > > calls
> > > > to
> > > > > PdfCompositorService::Create() change this and cause requests for
future
> > > > > PdfCompositors to be created with a different creator?
> > > > 
> > > > I have not found a better solution. This string should be constant for a
> > give
> > > > product since it stores user agent.
> > > > User agent is composed after browser starts. So passing in when creating
> the
> > > > service seems the right moment.
> > > 
> > > How about just having a static function to configure this, and have
> > > PdfCompositor just use the static directly? If it's stored as a member on
> each
> > > PdfCompositorService/PdfCompositor, it kind of looks like it can be
> different
> > > for different PdfCompositors -- but from this comment, it seems like it's
> > > actually constant.
> > 
> > oh, I might misunderstand your previous comments.
> > PdfCompositorService::Create() is a static function and will be called only
> once
> > during creating the service. 
> > IIUC, It should only be called once since a service should handle all
requests
> > when it is alive. You can think of it as a singleton.
> 
> Right, but what keeps the service alive, etc? Is it obvious to readers that
> there's a single static instance? The way it's structured, I'd say that's not
> entirely clear. Using a static field to hold the user agent would make that
> clear that it's shared between all instances.

Per our offline discussion, g_creator is already a global variable, there is no
apparent better solution.

Pls let me know if there is any other issue or concern. thanks

[dcheng, 2017-05-17 12:42:56]

ipc lgtm but...

For some background of why I'm not fully satisfied with the user agent name
handling:
- The user agent name is stored as a global
- It's bound by PdfCompositorService. Apparently this is only going to be called
once?
- PdfCompositorService uses the global user agent name...
- ...but PdfCompositorImpl takes the user agent name as an argument.

I would prefer to see a model where:
- We get rid of the global and PdfCompositorService has an instance variable to
hold the user agent, which it passes to new'd PdfCompositorImpls
- Otherwise, get rid of PdfCompositorImpl's user agent argument, and just always
use the global.

Either way would be more consistent than the current model, where some things
use the global and some things store it as an instance variable.

https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:44: if
(!shm->Map(memory_size) || shm->mapped_size() == 0) {
Nit: why does this need to check both? Shouldn't Map() returning false be
sufficient?

https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:67:
MakePdfDocument(creator_.empty() ? "Chromium" : creator_, &wstream);
I would suggest not checking this and just checking that creator_.empty() is
false.

(MakePdfDOcument also does a check for this which should probably also just be a
DCHECK)

The reason is this case should never happen in practice, so it's unfortunate to
emit code to alloc a string that will never be used.

[hal.canary, 2017-05-17 13:05:46]

On 2017/05/17 12:42:56, dcheng (in AEST) wrote:
> For some background of why I'm not fully satisfied with the user agent name
> handling...

I wrote the original version of this code.  I wasn't sure the best way to get
the user agent into the PDF, so any improvements to the code are welcome!

By the way, having this info inside the PDF is *really* helpful for debugging
issues in PDF generation, so thanks for keeping it in place!

[Wei Li, 2017-05-17 23:48:02]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 23:49:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 05:01:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 05:01:16]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Wei Li, 2017-05-18 20:26:35]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:28:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 21:24:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 21:24:52]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Wei Li, 2017-05-19 16:09:33]

On 2017/05/17 12:42:56, dcheng (in AEST) wrote:
> ipc lgtm but...
> 
> For some background of why I'm not fully satisfied with the user agent name
> handling:
> - The user agent name is stored as a global
> - It's bound by PdfCompositorService. Apparently this is only going to be
called
> once?
> - PdfCompositorService uses the global user agent name...
> - ...but PdfCompositorImpl takes the user agent name as an argument.
> 
> I would prefer to see a model where:
> - We get rid of the global and PdfCompositorService has an instance variable
to
> hold the user agent, which it passes to new'd PdfCompositorImpls
> - Otherwise, get rid of PdfCompositorImpl's user agent argument, and just
always
> use the global.
> 
> Either way would be more consistent than the current model, where some things
> use the global and some things store it as an instance variable.
> 
>
https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
> File components/printing/service/pdf_compositor_impl.cc (right):
> 
>
https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
> components/printing/service/pdf_compositor_impl.cc:44: if
> (!shm->Map(memory_size) || shm->mapped_size() == 0) {
> Nit: why does this need to check both? Shouldn't Map() returning false be
> sufficient?
> 
>
https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
> components/printing/service/pdf_compositor_impl.cc:67:
> MakePdfDocument(creator_.empty() ? "Chromium" : creator_, &wstream);
> I would suggest not checking this and just checking that creator_.empty() is
> false.
> 
> (MakePdfDOcument also does a check for this which should probably also just be
a
> DCHECK)
> 
> The reason is this case should never happen in practice, so it's unfortunate
to
> emit code to alloc a string that will never be used.

Thank you for the detailed explanation. Now I think I got your point. I modified
to use class instance to store it.

[Wei Li, 2017-05-19 16:10:01]

https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:44: if
(!shm->Map(memory_size) || shm->mapped_size() == 0) {
On 2017/05/17 12:42:56, dcheng (in AEST) wrote:
> Nit: why does this need to check both? Shouldn't Map() returning false be
> sufficient?

Done.

https://codereview.chromium.org/2832633002/diff/720001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:67:
MakePdfDocument(creator_.empty() ? "Chromium" : creator_, &wstream);
On 2017/05/17 12:42:56, dcheng (in AEST) wrote:
> I would suggest not checking this and just checking that creator_.empty() is
> false.
> 
> (MakePdfDOcument also does a check for this which should probably also just be
a
> DCHECK)
> 
> The reason is this case should never happen in practice, so it's unfortunate
to
> emit code to alloc a string that will never be used.

I think your suggestion is reasonable. Keeping one constant string in exchange
of the flexibility of handling empty string should also be fine.

To avoid the checks, I moved the check to service constructor.

[dcheng, 2017-05-22 09:40:48]

Sorry for the review delays. LGTM and thanks for making the changes.

https://codereview.chromium.org/2832633002/diff/780001/components/printing/se...
File components/printing/service/pdf_compositor_impl.cc (right):

https://codereview.chromium.org/2832633002/diff/780001/components/printing/se...
components/printing/service/pdf_compositor_impl.cc:42:
std::unique_ptr<base::SharedMemory> shm =
FWIW, it's OK to use 'auto' as the typename in this case

[Wei Li, 2017-05-22 17:19:42]

The CQ bit was checked by weili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 17:20:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 18:33:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 18:33:18]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-05-22 18:56:33]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2017-05-22 18:56:34]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, thestig@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2832633002/#ps800001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-22 18:57:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 19:05:24]

CQ is committing da patch.

Bot data: {"patchset_id": 800001, "attempt_start_ts": 1495479393891770,
"parent_rev": "222e5e899928269ee42e1215b21b33b656522027", "commit_rev":
"fabbf757ba1f0fe03a5671eb0bb4936efbb4a890"}

[commit-bot: I haz the power, 2017-05-22 19:05:39]

Description was changed from

==========
Add PDF compositor service

Currently, the service converts one or multiple pages encapsulated in a
SkMultiPictureDocument to a PDF file. It runs in a sandboxed utility process.

This allows Chromium to move PDF generation code into a separate
process, and eventually can support compositing content generated from
multiple processes.

BUG=455764
==========

to

==========
Add PDF compositor service

Currently, the service converts one or multiple pages encapsulated in a
SkMultiPictureDocument to a PDF file. It runs in a sandboxed utility process.

This allows Chromium to move PDF generation code into a separate
process, and eventually can support compositing content generated from
multiple processes.

BUG=455764

Review-Url: https://codereview.chromium.org/2832633002
Cr-Commit-Position: refs/heads/master@{#473644}
Committed:
https://chromium.googlesource.com/chromium/src/+/fabbf757ba1f0fe03a5671eb0bb4...
==========

[commit-bot: I haz the power, 2017-05-22 19:05:42]

Committed patchset #36 (id:800001) as
https://chromium.googlesource.com/chromium/src/+/fabbf757ba1f0fe03a5671eb0bb4...