Use pdf compositor service for printing when OOPIF is enabled

Use pdf compositor service for printing when OOPIF is enabled

When OOPIF is enabled (by site-per-process flag or
top-document-isolation feature), use the pdf compositor service for
converting PaintRecord to PDF on renderers.

In the future, this will make compositing PDF from multiple renderers
possible.

BUG=455764

[Wei Li, 2017-07-21 23:31:30]

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

[commit-bot: I haz the power, 2017-07-21 23:33: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-07-21 23:39:10]

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

[commit-bot: I haz the power, 2017-07-21 23:39:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Wei Li, 2017-07-25 00:18:43]

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

[commit-bot: I haz the power, 2017-07-25 00:18:56]

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-07-25 00:30:29]

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

[commit-bot: I haz the power, 2017-07-25 00:30:30]

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

[Wei Li, 2017-07-25 07:11:37]

Patchset #7 (id:120001) has been deleted

[Wei Li, 2017-07-25 07:12:18]

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

[commit-bot: I haz the power, 2017-07-25 07:12: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-07-25 07:17:20]

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

[commit-bot: I haz the power, 2017-07-25 07:17:21]

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

[Wei Li, 2017-07-25 17:53:12]

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

[commit-bot: I haz the power, 2017-07-25 17:53:22]

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-07-25 18:07:17]

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

[commit-bot: I haz the power, 2017-07-25 18:07:19]

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-08-03 00:46:41]

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

[commit-bot: I haz the power, 2017-08-03 00:46: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-08-03 00:58:48]

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

[commit-bot: I haz the power, 2017-08-03 00:58:49]

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-08-03 07:16:24]

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

[commit-bot: I haz the power, 2017-08-03 07:16:33]

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-08-03 08:01:43]

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

[commit-bot: I haz the power, 2017-08-03 08:01:44]

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

[Wei Li, 2017-08-03 18:20:25]

Description was changed from

==========
[Experimental] Use pdf compositor service for printing

BUG=
==========

to

==========
When OOPIF is enabled (by site-per-process flag or top-document-isolation
feature), use the pdf compositor service for converting PaintRecord to PDF on
renderers.
In the future, this will make compositing PDF from multiple renderers possible. 
 

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

[Wei Li, 2017-08-03 18:30:51]

Description was changed from

==========
When OOPIF is enabled (by site-per-process flag or top-document-isolation
feature), use the pdf compositor service for converting PaintRecord to PDF on
renderers.
In the future, this will make compositing PDF from multiple renderers possible. 
 

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

to

==========
When OOPIF is enabled (by site-per-process flag or
top-document-isolation feature), use the pdf compositor service for
converting PaintRecord to PDF on renderers.
In the future, this will make compositing PDF from multiple
renderers possible.

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

[Wei Li, 2017-08-05 01:48:36]

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

[commit-bot: I haz the power, 2017-08-05 01:48:47]

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-08-05 03:03:39]

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

[commit-bot: I haz the power, 2017-08-05 03:03:40]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-08-05 03:19:14]

weili@chromium.org changed reviewers:
+ dcheng@chromium.org, jzfeng@chromium.org, thestig@chromium.org

[Wei Li, 2017-08-05 03:19:15]

Lei, pls review in general;
Daniel, pls review the print_messages and content browser manifest change;
Jianzhou, pls review changes to headless chrome;

thanks

[jzfeng, 2017-08-07 05:08:56]

lgtm

[Wei Li, 2017-08-14 19:33:42]

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

[commit-bot: I haz the power, 2017-08-14 19:33:56]

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-08-14 21:05:55]

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

[commit-bot: I haz the power, 2017-08-14 21:05:56]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-08-14 21:18:00]

gentle ping?

[Lei Zhang, 2017-08-16 21:01:19]

Description was changed from

==========
When OOPIF is enabled (by site-per-process flag or
top-document-isolation feature), use the pdf compositor service for
converting PaintRecord to PDF on renderers.
In the future, this will make compositing PDF from multiple
renderers possible.

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

to

==========
Use pdf compositor service for printing when OOPIF is enabled

When OOPIF is enabled (by site-per-process flag or
top-document-isolation feature), use the pdf compositor service for
converting PaintRecord to PDF on renderers.
In the future, this will make compositing PDF from multiple
renderers possible.

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

[Lei Zhang, 2017-08-16 21:01:55]

Description was changed from

==========
Use pdf compositor service for printing when OOPIF is enabled

When OOPIF is enabled (by site-per-process flag or
top-document-isolation feature), use the pdf compositor service for
converting PaintRecord to PDF on renderers.
In the future, this will make compositing PDF from multiple
renderers possible.

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

to

==========
Use pdf compositor service for printing when OOPIF is enabled

When OOPIF is enabled (by site-per-process flag or
top-document-isolation feature), use the pdf compositor service for
converting PaintRecord to PDF on renderers.

In the future, this will make compositing PDF from multiple renderers
possible.

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

[Lei Zhang, 2017-08-16 21:17:01]

Can we split off some of the changes into their own CLs?

https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printin...
File chrome/browser/printing/printing_init.h (right):

https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printin...
chrome/browser/printing/printing_init.h:15: void
InitializePrinting(content::WebContents* web_contents);
Can this be its own CL?

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
File components/printing/browser/DEPS (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
components/printing/browser/DEPS:2: "+base",
Probably not needed, since base/ is almost universally available via a rule in
//DEPS.

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
components/printing/browser/print_manager_utils.cc:18:
base::LazyInstance<bool>::Leaky g_oopif_enabled;
Can this just be a bool?

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
File components/printing/browser/print_manager_utils.h (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
components/printing/browser/print_manager_utils.h:21: void
CreateCompositeClientIfNeeded(content::WebContents* web_contents);
Add a comment to mention it's creating the client for |web_contents| ?

https://codereview.chromium.org/2920013002/diff/330001/components/printing/re...
File components/printing/renderer/print_render_frame_helper.cc (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/re...
components/printing/renderer/print_render_frame_helper.cc:1400:
draft_metafile.reset(new PdfMetafileSkia(print_params.printed_doc_type));
Can't use MakeUnique anymore?

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

https://codereview.chromium.org/2920013002/diff/330001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.h:17: enum class SkiaDocumentType {
Can we split this into a separate CL?

[Wei Li, 2017-08-25 23:39:38]

I split out a few CLs. Now ptal, thanks!

https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printin...
File chrome/browser/printing/printing_init.h (right):

https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printin...
chrome/browser/printing/printing_init.h:15: void
InitializePrinting(content::WebContents* web_contents);
On 2017/08/16 21:17:00, Lei Zhang wrote:
> Can this be its own CL?

Done.

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
File components/printing/browser/DEPS (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
components/printing/browser/DEPS:2: "+base",
On 2017/08/16 21:17:00, Lei Zhang wrote:
> Probably not needed, since base/ is almost universally available via a rule in
> //DEPS.

Done.

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
components/printing/browser/print_manager_utils.cc:18:
base::LazyInstance<bool>::Leaky g_oopif_enabled;
On 2017/08/16 21:17:00, Lei Zhang wrote:
> Can this just be a bool?

Yes, done.

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
File components/printing/browser/print_manager_utils.h (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/br...
components/printing/browser/print_manager_utils.h:21: void
CreateCompositeClientIfNeeded(content::WebContents* web_contents);
On 2017/08/16 21:17:01, Lei Zhang wrote:
> Add a comment to mention it's creating the client for |web_contents| ?

Done.

https://codereview.chromium.org/2920013002/diff/330001/components/printing/re...
File components/printing/renderer/print_render_frame_helper.cc (right):

https://codereview.chromium.org/2920013002/diff/330001/components/printing/re...
components/printing/renderer/print_render_frame_helper.cc:1400:
draft_metafile.reset(new PdfMetafileSkia(print_params.printed_doc_type));
On 2017/08/16 21:17:01, Lei Zhang wrote:
> Can't use MakeUnique anymore?

Done in another CL.

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

https://codereview.chromium.org/2920013002/diff/330001/printing/common/pdf_me...
printing/common/pdf_metafile_utils.h:17: enum class SkiaDocumentType {
On 2017/08/16 21:17:01, Lei Zhang wrote:
> Can we split this into a separate CL?

Done.

[Lei Zhang, 2017-08-28 22:46:40]

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool
enabled) {
Since this is setting a global variable, aren't the calls in
CreateCompositeClientIfNeeded() and HeadlessWebContentsImpl ctor going to
conflict?

[Lei Zhang, 2017-08-28 23:01:13]

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:138: DCHECK(client);
Can this be reached?

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:143:
client->DoComposite(
How does |client| know what this is compositing vs the call on line 176?

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:146:
base::Unretained(this), params.page_number,
Is it safe to use Unretained() here?

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.h (right):

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.h:46: void
OnCompositePdfPageDone(int page_number,
I think these can be private.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.cc:66: new
base::SharedMemory(memory_handle, true /* read_only */));
MakeUnique

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.cc:82: return
scoped_refptr<base::RefCountedBytes>(new base::RefCountedBytes(
MakeRefCounted

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.h:31:
scoped_refptr<base::SequencedTaskRunner> callback_task_runner);
Is this always going to be UI thread?

[Wei Li, 2017-08-29 20:17:55]

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

[commit-bot: I haz the power, 2017-08-29 20:18:10]

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-08-29 21:30:02]

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

[commit-bot: I haz the power, 2017-08-29 21:30:04]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-08-30 00:24:06]

thanks for the review!

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:138: DCHECK(client);
On 2017/08/28 23:01:12, Lei Zhang wrote:
> Can this be reached?

This essentially becomes NOTREACHED(). Now made it simpler by removing the if.

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:143:
client->DoComposite(
On 2017/08/28 23:01:12, Lei Zhang wrote:
> How does |client| know what this is compositing vs the call on line 176?

Compositing from multiple source is not yet added. Now the call is a simple
conversion call for either case.

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:146:
base::Unretained(this), params.page_number,
On 2017/08/28 23:01:12, Lei Zhang wrote:
> Is it safe to use Unretained() here?

PrintPreviewMessageHandler and client should have (almost) identical life cycle
since they are both web content user data. Since the dtor order may vary, change
this to weak ptr is safer.

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.h (right):

https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.h:46: void
OnCompositePdfPageDone(int page_number,
On 2017/08/28 23:01:12, Lei Zhang wrote:
> I think these can be private.

Done.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.cc:66: new
base::SharedMemory(memory_handle, true /* read_only */));
On 2017/08/28 23:01:12, Lei Zhang wrote:
> MakeUnique

Done.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.cc:82: return
scoped_refptr<base::RefCountedBytes>(new base::RefCountedBytes(
On 2017/08/28 23:01:12, Lei Zhang wrote:
> MakeRefCounted

RefCountedBytes doesn't have similar function defined.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.h:31:
scoped_refptr<base::SequencedTaskRunner> callback_task_runner);
On 2017/08/28 23:01:13, Lei Zhang wrote:
> Is this always going to be UI thread?

Yes, so far all the callers are from UI thread. But this function is not tied to
UI thread.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool
enabled) {
On 2017/08/28 22:46:39, Lei Zhang wrote:
> Since this is setting a global variable, aren't the calls in
> CreateCompositeClientIfNeeded() and HeadlessWebContentsImpl ctor going to
> conflict?

They belong to different embedders, so will not co-exist.

[Lei Zhang, 2017-08-30 02:00:36]

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.cc:82: return
scoped_refptr<base::RefCountedBytes>(new base::RefCountedBytes(
On 2017/08/30 00:24:02, Wei Li wrote:
> On 2017/08/28 23:01:12, Lei Zhang wrote:
> > MakeRefCounted
> 
> RefCountedBytes doesn't have similar function defined.

return base::MakeRefCounted<base::RefCountedBytes>(
    reinterpret_cast<const unsigned char*>(shm->memory()),
    shm->mapped_size());

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.h:31:
scoped_refptr<base::SequencedTaskRunner> callback_task_runner);
On 2017/08/30 00:24:03, Wei Li wrote:
> On 2017/08/28 23:01:13, Lei Zhang wrote:
> > Is this always going to be UI thread?
> 
> Yes, so far all the callers are from UI thread. But this function is not tied
to
> UI thread.

Is it reasonable to make DoComposite() just reply on the task runner it was
called on?

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool
enabled) {
On 2017/08/30 00:24:03, Wei Li wrote:
> On 2017/08/28 22:46:39, Lei Zhang wrote:
> > Since this is setting a global variable, aren't the calls in
> > CreateCompositeClientIfNeeded() and HeadlessWebContentsImpl ctor going to
> > conflict?
> 
> They belong to different embedders, so will not co-exist.

Jianzhou, we are not trying to make headless mode co-exist in the browser
process anymore, right?

https://codereview.chromium.org/2920013002/diff/450001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/2920013002/diff/450001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_base.cc:181: if (auto* client =
Can this also DCHECK(client), like print_preview_message_handler.cc?

https://codereview.chromium.org/2920013002/diff/450001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/450001/components/printing/br...
components/printing/browser/print_manager_utils.cc:18: static bool
g_oopif_enabled;
Can we initialize this to false, and only enable it where needed?

https://codereview.chromium.org/2920013002/diff/450001/components/printing/br...
components/printing/browser/print_manager_utils.cc:34: SetOopifEnabled(true);
Why do we have to set this once per WebContents?

[Lei Zhang, 2017-08-30 04:50:14]

Also, when I turned on --site-per-process to try it out on chrome://version,
SkDiscardableMemory::Create() runs out of memory. Is that expected?

[Wei Li, 2017-08-30 20:06:52]

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

[commit-bot: I haz the power, 2017-08-30 20:06: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-08-30 22:09:59]

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

[commit-bot: I haz the power, 2017-08-30 22:10:00]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-08-31 19:25:40]

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

[commit-bot: I haz the power, 2017-08-31 19:25:53]

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-08-31 20:49:24]

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

[commit-bot: I haz the power, 2017-08-31 20:49:26]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-08-31 20:59:49]

On 2017/08/30 04:50:14, Lei Zhang wrote:
> Also, when I turned on --site-per-process to try it out on chrome://version,
> SkDiscardableMemory::Create() runs out of memory. Is that expected?

This was a regression by other's CL. I just fixed it (crrev/c/643788).

[Wei Li, 2017-08-31 21:00:25]

Thanks, ptal

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.cc:82: return
scoped_refptr<base::RefCountedBytes>(new base::RefCountedBytes(
On 2017/08/30 02:00:35, Lei Zhang wrote:
> On 2017/08/30 00:24:02, Wei Li wrote:
> > On 2017/08/28 23:01:12, Lei Zhang wrote:
> > > MakeRefCounted
> > 
> > RefCountedBytes doesn't have similar function defined.
> 
> return base::MakeRefCounted<base::RefCountedBytes>(
>     reinterpret_cast<const unsigned char*>(shm->memory()),
>     shm->mapped_size());

Thanks, done.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_composite_client.h:31:
scoped_refptr<base::SequencedTaskRunner> callback_task_runner);
On 2017/08/30 02:00:35, Lei Zhang wrote:
> On 2017/08/30 00:24:03, Wei Li wrote:
> > On 2017/08/28 23:01:13, Lei Zhang wrote:
> > > Is this always going to be UI thread?
> > 
> > Yes, so far all the callers are from UI thread. But this function is not
tied
> to
> > UI thread.
> 
> Is it reasonable to make DoComposite() just reply on the task runner it was
> called on?

Sounds good, done.

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool
enabled) {
On 2017/08/30 02:00:35, Lei Zhang wrote:
> On 2017/08/30 00:24:03, Wei Li wrote:
> > On 2017/08/28 22:46:39, Lei Zhang wrote:
> > > Since this is setting a global variable, aren't the calls in
> > > CreateCompositeClientIfNeeded() and HeadlessWebContentsImpl ctor going to
> > > conflict?
> > 
> > They belong to different embedders, so will not co-exist.
> 
> Jianzhou, we are not trying to make headless mode co-exist in the browser
> process anymore, right?

I am pretty sure about this as headless chrome doesn't use Chrome browser code.
Jianzhou, can you confirm this?

https://codereview.chromium.org/2920013002/diff/450001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/2920013002/diff/450001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_base.cc:181: if (auto* client =
On 2017/08/30 02:00:35, Lei Zhang wrote:
> Can this also DCHECK(client), like print_preview_message_handler.cc?

Done.

https://codereview.chromium.org/2920013002/diff/450001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/450001/components/printing/br...
components/printing/browser/print_manager_utils.cc:18: static bool
g_oopif_enabled;
On 2017/08/30 02:00:35, Lei Zhang wrote:
> Can we initialize this to false, and only enable it where needed?

Done.

https://codereview.chromium.org/2920013002/diff/450001/components/printing/br...
components/printing/browser/print_manager_utils.cc:34: SetOopifEnabled(true);
On 2017/08/30 02:00:35, Lei Zhang wrote:
> Why do we have to set this once per WebContents?

We don't need to, but it is easier this way. We may be able to setup it during
browser init process, but since it has to be conditional on printing feature,
the change might look messier. Alternatively, we can use a variable to check
whether we already set it to make sure we set it once, but it is essentially
same with roughly same cost.

[Lei Zhang, 2017-08-31 22:35:04]

Still doesn't work for me at ToT with the latest patch set patched in. The print
preview never displays. On the console, I see:

[...:ERROR:service_manager.cc(156)] Connection InterfaceProviderSpec prevented
service: content_browser from binding interface: printing::mojom::PdfCompositor
exposed by: pdf_compositor

https://codereview.chromium.org/2920013002/diff/510001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/510001/components/printing/br...
components/printing/browser/print_composite_client.h:11: #include
"base/sequenced_task_runner.h"
No longer needed in the header -> move to .cc file.

[Wei Li, 2017-09-01 04:32:08]

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

[commit-bot: I haz the power, 2017-09-01 04:32:19]

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-09-01 05:09:49]

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

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

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

[jzfeng, 2017-09-01 05:18:04]

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2920013002/diff/410001/components/printing/br...
components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool
enabled) {
On 2017/08/31 at 21:00:25, Wei Li wrote:
> On 2017/08/30 02:00:35, Lei Zhang wrote:
> > On 2017/08/30 00:24:03, Wei Li wrote:
> > > On 2017/08/28 22:46:39, Lei Zhang wrote:
> > > > Since this is setting a global variable, aren't the calls in
> > > > CreateCompositeClientIfNeeded() and HeadlessWebContentsImpl ctor going
to
> > > > conflict?
> > > 
> > > They belong to different embedders, so will not co-exist.
> > 
> > Jianzhou, we are not trying to make headless mode co-exist in the browser
> > process anymore, right?
> 
> I am pretty sure about this as headless chrome doesn't use Chrome browser
code. Jianzhou, can you confirm this?

Yes, headless chrome is another embedder. It only reuse code in content layer
and components.

[Wei Li, 2017-09-01 19:52:15]

https://codereview.chromium.org/2920013002/diff/510001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/510001/components/printing/br...
components/printing/browser/print_composite_client.h:11: #include
"base/sequenced_task_runner.h"
On 2017/08/31 22:35:03, Lei Zhang wrote:
> No longer needed in the header -> move to .cc file.

Removed it.

[Wei Li, 2017-09-01 19:54:52]

On 2017/08/31 22:35:04, Lei Zhang wrote:
> Still doesn't work for me at ToT with the latest patch set patched in. The
print
> preview never displays. On the console, I see:
> 
> [...:ERROR:service_manager.cc(156)] Connection InterfaceProviderSpec prevented
> service: content_browser from binding interface:
printing::mojom::PdfCompositor
> exposed by: pdf_compositor
> 

Can you check whether the patch was applied successfully? Looks like the file
chrome_content_browser_manifest_overlay.json was not correctly patched.
My tests run ok. thanks for checking!

[Wei Li, 2017-09-05 22:10:44]

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

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

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-09-06 00:47:03]

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

[commit-bot: I haz the power, 2017-09-06 00:47:05]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-09-07 18:51:01]

On 2017/09/01 19:54:52, Wei Li wrote:
> On 2017/08/31 22:35:04, Lei Zhang wrote:
> > Still doesn't work for me at ToT with the latest patch set patched in. The
> print
> > preview never displays. On the console, I see:
> > 
> > [...:ERROR:service_manager.cc(156)] Connection InterfaceProviderSpec
prevented
> > service: content_browser from binding interface:
> printing::mojom::PdfCompositor
> > exposed by: pdf_compositor
> > 
> 
> Can you check whether the patch was applied successfully? Looks like the file
> chrome_content_browser_manifest_overlay.json was not correctly patched.
> My tests run ok. thanks for checking!

I applied patch set 30 cleanly on top of r499694 locally and it still does not
work for me.

[Lei Zhang, 2017-09-08 07:50:53]

I'll finish reviewing chrome/browser/printing/print_view_manager_base.cc later.

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:188: NOTREACHED() <<
"Compositing pdf failed";
You shouldn't use NOTREACHED() here, because the compositor can fail. Switch to
some form of logging instead?

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.h (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.h:75: void
NotifyUIPreviewPageReady(
Can the declaration order match the implementation order?

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:1146: "//components/printing/browser:browser",
I think you can omit the colon and everything after that, if it's the same as
the part before the colon.

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
File components/printing/browser/DEPS (right):

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/DEPS:5: "+printing",
Already in components/printing/DEPS, so not needed?

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.cc:1: // Copyright (c) 2017
The Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.cc:38: if (!data_size)
Don't we have to run |callback| here and indicate failure?

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.h:19: public
content::WebContentsObserver,
Is this class actually a WebContentsObserver?

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.h:25: void
CreateConnectorRequest();
private

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.h:39:
base::WeakPtrFactory<PrintCompositeClient> weak_factory_;
Not used?

[Wei Li, 2017-09-08 20:46:49]

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

[commit-bot: I haz the power, 2017-09-08 20:46:59]

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-09-08 22:42:29]

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

[commit-bot: I haz the power, 2017-09-08 22:42:30]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-09-08 22:57:48]

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:188: NOTREACHED() <<
"Compositing pdf failed";
On 2017/09/08 07:50:53, Lei Zhang wrote:
> You shouldn't use NOTREACHED() here, because the compositor can fail. Switch
to
> some form of logging instead?

Compositor only fails on unable to map memory or invalid buffer format which
should not happen in our current use, so should be safe to use NOTREACHED. For
the next steps (adding the logic into compositor), I will have to think ways to
handle errors as compositor may fail for some legit reasons.

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.h (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.h:75: void
NotifyUIPreviewPageReady(
On 2017/09/08 07:50:53, Lei Zhang wrote:
> Can the declaration order match the implementation order?

Changed on impl side. Done.

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:1146: "//components/printing/browser:browser",
On 2017/09/08 07:50:53, Lei Zhang wrote:
> I think you can omit the colon and everything after that, if it's the same as
> the part before the colon.

Done.

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
File components/printing/browser/DEPS (right):

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/DEPS:5: "+printing",
On 2017/09/08 07:50:53, Lei Zhang wrote:
> Already in components/printing/DEPS, so not needed?

Done.

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.cc:1: // Copyright (c) 2017
The Chromium Authors. All rights reserved.
On 2017/09/08 07:50:53, Lei Zhang wrote:
> No (c)

Done.

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.cc:38: if (!data_size)
On 2017/09/08 07:50:53, Lei Zhang wrote:
> Don't we have to run |callback| here and indicate failure?

Changed to DCHECK

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
File components/printing/browser/print_composite_client.h (right):

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.h:19: public
content::WebContentsObserver,
On 2017/09/08 07:50:53, Lei Zhang wrote:
> Is this class actually a WebContentsObserver?

Done.

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.h:25: void
CreateConnectorRequest();
On 2017/09/08 07:50:53, Lei Zhang wrote:
> private

Done.

https://codereview.chromium.org/2920013002/diff/590001/components/printing/br...
components/printing/browser/print_composite_client.h:39:
base::WeakPtrFactory<PrintCompositeClient> weak_factory_;
On 2017/09/08 07:50:53, Lei Zhang wrote:
> Not used?

Done.

[Lei Zhang, 2017-09-09 00:45:00]

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.cc:188: NOTREACHED() <<
"Compositing pdf failed";
On 2017/09/08 22:57:47, Wei Li wrote:
> On 2017/09/08 07:50:53, Lei Zhang wrote:
> > You shouldn't use NOTREACHED() here, because the compositor can fail. Switch
> to
> > some form of logging instead?
> 
> Compositor only fails on unable to map memory or invalid buffer format which
> should not happen in our current use, so should be safe to use NOTREACHED. For
> the next steps (adding the logic into compositor), I will have to think ways
to
> handle errors as compositor may fail for some legit reasons. 

But on the compositor side, a Map() failure isn't treated as NOTREACHED.
Shouldn't the two sides be consistent?

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/printin...
File chrome/browser/printing/print_preview_message_handler.h (right):

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/printin...
chrome/browser/printing/print_preview_message_handler.h:94:
base::WeakPtrFactory<PrintPreviewMessageHandler> weak_ptr_factory_;
#include "base/memory/weak_ptr.h"

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_base.cc:205: if (!IsOopifEnabled())
With the original code, on Linux, where |metafile_must_be_valid| is not always
true, the following can occurs:

1. For the first page |metafile_must_be_valid| is true.
2. Pass the checks above.
3. |metafile| gets created and initialized.
4. PrintedDocument::SetPage() and ShouldQuitFromInnerMessageLoop() get called.

5. For the second page and beyond, |metafile_must_be_valid| is false.
6. Still has to pass the checks above.
7. |metafile| gets created but remains uninitialized.
8. PrintedDocument::SetPage() and ShouldQuitFromInnerMessageLoop() get called.

With this CL and OOPIF:

1. For the first page |metafile_must_be_valid| is true.
2. In the checks above, DoComposite() gets called.
3. On success, the callback runs UpdateForPrintedPage().
4. |metafile| gets created and initialized.
5. PrintedDocument::SetPage() and ShouldQuitFromInnerMessageLoop() get called.

6. For the second page and beyond, |metafile_must_be_valid| is false.
7. Still has to pass the checks above.

And then this method does nothing. Isn't this going to leave
PrintViewManagerBase in a bad state?

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_base.h (right):

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_base.h:183:
base::WeakPtrFactory<PrintViewManagerBase> weak_ptr_factory_;
#include "base/memory/weak_ptr.h"

e.g. this only works because of this #include chains:
components/prefs/pref_member.h -> base/bind.h -> base/bind_internal.h ->
base/memory/weak_ptr.h

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2920013002/diff/630001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:1143: "//printing:printing",
Same here. Just //printing.

https://codereview.chromium.org/2920013002/diff/630001/components/printing/br...
File components/printing/browser/print_composite_client.cc (right):

https://codereview.chromium.org/2920013002/diff/630001/components/printing/br...
components/printing/browser/print_composite_client.cc:37: DCHECK(data_size);
With the DCHECK, I'm worried the call from
PrintViewManagerBase::OnDidPrintPage() can potentially cause this to fail, since
it doesn't check for empty data. The print_preview_message_handler.cc callers do
check.

[Wei Li, 2017-09-27 00:56:27]

Patchset #34 (id:670001) has been deleted