Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2920013002: Use pdf compositor service for printing when OOPIF is enabled

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by Wei Li
Modified:
2 weeks, 6 days ago
Reviewers:
Lei Zhang, jzfeng, dcheng
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : use a flag to keep oopif on/off #

Patch Set 4 : rebase #

Patch Set 5 : fix dep #

Patch Set 6 : move flag to client #

Patch Set 7 : fix for mac #

Patch Set 8 : fix dep #

Patch Set 9 : fix dep #

Patch Set 10 : fix gn check error #

Patch Set 11 : rebase #

Patch Set 12 : make extensions/app/headless work #

Patch Set 13 : rebase #

Patch Set 14 : fix cast_android bot #

Patch Set 15 : fix headless #

Patch Set 16 : rebase #

Patch Set 17 : rebase #

Total comments: 12

Patch Set 18 : rebase after splitting out two CLs #

Patch Set 19 : rebase after splitting out one more CL #

Patch Set 20 : address Lei's comments #

Patch Set 21 : rebase #

Total comments: 23

Patch Set 22 : address Lei's comments #

Patch Set 23 : rebase and resolve conflict #

Total comments: 6

Patch Set 24 : address more comments #

Patch Set 25 : rebase #

Patch Set 26 : rebase again #

Total comments: 2

Patch Set 27 : rebase #

Patch Set 28 : remove header #

Patch Set 29 : rebase #

Patch Set 30 : fix due to rebase #

Total comments: 19

Patch Set 31 : rebase #

Patch Set 32 : address Lei's comments #

Total comments: 5

Patch Set 33 : rebase #

Patch Set 34 : address more comments #

Patch Set 35 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+393 lines, -60 lines) Patch
M chrome/browser/chrome_content_browser_manifest_overlay.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/printing/print_preview_message_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 2 chunks +24 lines, -0 lines 0 comments Download
M chrome/browser/printing/print_preview_message_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 6 chunks +96 lines, -26 lines 0 comments Download
M chrome/browser/printing/print_view_manager_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +18 lines, -1 line 0 comments Download
M chrome/browser/printing/print_view_manager_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 8 chunks +70 lines, -30 lines 0 comments Download
M chrome/browser/printing/printing_init.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +4 lines, -1 line 0 comments Download
M chrome/utility/chrome_content_utility_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -1 line 0 comments Download
M components/printing/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -0 lines 0 comments Download
M components/printing/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +3 lines, -0 lines 0 comments Download
A components/printing/browser/print_composite_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +45 lines, -0 lines 0 comments Download
A components/printing/browser/print_composite_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +83 lines, -0 lines 0 comments Download
M components/printing/browser/print_manager_utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +11 lines, -0 lines 0 comments Download
M components/printing/browser/print_manager_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +29 lines, -1 line 0 comments Download
M headless/lib/browser/headless_web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +1 line, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 83 (63 generated)
Wei Li
Lei, pls review in general; Daniel, pls review the print_messages and content browser manifest change; ...
2 months, 2 weeks ago (2017-08-05 03:19:15 UTC) #33
jzfeng
lgtm
2 months, 1 week ago (2017-08-07 05:08:56 UTC) #34
Wei Li
gentle ping?
2 months ago (2017-08-14 21:18:00 UTC) #39
Lei Zhang
Can we split off some of the changes into their own CLs? https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printing/printing_init.h File chrome/browser/printing/printing_init.h ...
2 months ago (2017-08-16 21:17:01 UTC) #42
Wei Li
I split out a few CLs. Now ptal, thanks! https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printing/printing_init.h File chrome/browser/printing/printing_init.h (right): https://codereview.chromium.org/2920013002/diff/330001/chrome/browser/printing/printing_init.h#newcode15 chrome/browser/printing/printing_init.h:15: ...
1 month, 3 weeks ago (2017-08-25 23:39:38 UTC) #43
Lei Zhang
https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_manager_utils.cc File components/printing/browser/print_manager_utils.cc (right): https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_manager_utils.cc#newcode20 components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool enabled) { Since this is setting a ...
1 month, 3 weeks ago (2017-08-28 22:46:40 UTC) #44
Lei Zhang
https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printing/print_preview_message_handler.cc File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printing/print_preview_message_handler.cc#newcode138 chrome/browser/printing/print_preview_message_handler.cc:138: DCHECK(client); Can this be reached? https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printing/print_preview_message_handler.cc#newcode143 chrome/browser/printing/print_preview_message_handler.cc:143: client->DoComposite( How ...
1 month, 3 weeks ago (2017-08-28 23:01:13 UTC) #45
Wei Li
thanks for the review! https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printing/print_preview_message_handler.cc File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/2920013002/diff/410001/chrome/browser/printing/print_preview_message_handler.cc#newcode138 chrome/browser/printing/print_preview_message_handler.cc:138: DCHECK(client); On 2017/08/28 23:01:12, Lei ...
1 month, 2 weeks ago (2017-08-30 00:24:06 UTC) #50
Lei Zhang
https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_composite_client.cc File components/printing/browser/print_composite_client.cc (right): https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_composite_client.cc#newcode82 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: ...
1 month, 2 weeks ago (2017-08-30 02:00:36 UTC) #51
Lei Zhang
Also, when I turned on --site-per-process to try it out on chrome://version, SkDiscardableMemory::Create() runs out ...
1 month, 2 weeks ago (2017-08-30 04:50:14 UTC) #52
Wei Li
On 2017/08/30 04:50:14, Lei Zhang wrote: > Also, when I turned on --site-per-process to try ...
1 month, 2 weeks ago (2017-08-31 20:59:49 UTC) #61
Wei Li
Thanks, ptal https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_composite_client.cc File components/printing/browser/print_composite_client.cc (right): https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_composite_client.cc#newcode82 components/printing/browser/print_composite_client.cc:82: return scoped_refptr<base::RefCountedBytes>(new base::RefCountedBytes( On 2017/08/30 02:00:35, Lei ...
1 month, 2 weeks ago (2017-08-31 21:00:25 UTC) #62
Lei Zhang
Still doesn't work for me at ToT with the latest patch set patched in. The ...
1 month, 2 weeks ago (2017-08-31 22:35:04 UTC) #63
jzfeng
https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_manager_utils.cc File components/printing/browser/print_manager_utils.cc (right): https://codereview.chromium.org/2920013002/diff/410001/components/printing/browser/print_manager_utils.cc#newcode20 components/printing/browser/print_manager_utils.cc:20: void SetOopifEnabled(bool enabled) { On 2017/08/31 at 21:00:25, Wei ...
1 month, 2 weeks ago (2017-09-01 05:18:04 UTC) #68
Wei Li
https://codereview.chromium.org/2920013002/diff/510001/components/printing/browser/print_composite_client.h File components/printing/browser/print_composite_client.h (right): https://codereview.chromium.org/2920013002/diff/510001/components/printing/browser/print_composite_client.h#newcode11 components/printing/browser/print_composite_client.h:11: #include "base/sequenced_task_runner.h" On 2017/08/31 22:35:03, Lei Zhang wrote: > ...
1 month, 2 weeks ago (2017-09-01 19:52:15 UTC) #69
Wei Li
On 2017/08/31 22:35:04, Lei Zhang wrote: > Still doesn't work for me at ToT with ...
1 month, 2 weeks ago (2017-09-01 19:54:52 UTC) #70
Lei Zhang
On 2017/09/01 19:54:52, Wei Li wrote: > On 2017/08/31 22:35:04, Lei Zhang wrote: > > ...
1 month, 1 week ago (2017-09-07 18:51:01 UTC) #75
Lei Zhang
I'll finish reviewing chrome/browser/printing/print_view_manager_base.cc later. https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printing/print_preview_message_handler.cc File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printing/print_preview_message_handler.cc#newcode188 chrome/browser/printing/print_preview_message_handler.cc:188: NOTREACHED() << "Compositing pdf ...
1 month, 1 week ago (2017-09-08 07:50:53 UTC) #76
Wei Li
https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printing/print_preview_message_handler.cc File chrome/browser/printing/print_preview_message_handler.cc (right): https://codereview.chromium.org/2920013002/diff/590001/chrome/browser/printing/print_preview_message_handler.cc#newcode188 chrome/browser/printing/print_preview_message_handler.cc:188: NOTREACHED() << "Compositing pdf failed"; On 2017/09/08 07:50:53, Lei ...
1 month, 1 week ago (2017-09-08 22:57:48 UTC) #81
Lei Zhang
1 month, 1 week ago (2017-09-09 00:45:00 UTC) #82
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa