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

Issue 2832633002: Add PDF compositor service (Closed)

Created:
2 years, 2 months ago by Wei Li
Modified:
2 years, 1 month ago
CC:
Aaron Boodman, abarth-chromium, blundell+watchlist_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, droger+watchlist_chromium.org, jam, qsr+mojo_chromium.org, sdefresne+watchlist_chromium.org, site-isolation-reviews_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, nasko
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : style fixes #

Patch Set 3 : rebase #

Patch Set 4 : fix header #

Patch Set 5 : remove headers #

Total comments: 26

Patch Set 6 : address comments #

Patch Set 7 : rebase #

Patch Set 8 : modify deps and rebase #

Patch Set 9 : change a check #

Patch Set 10 : rebase again #

Total comments: 14

Patch Set 11 : address thestig's comments #

Patch Set 12 : rebase #

Total comments: 6

Patch Set 13 : address jam's comments #

Patch Set 14 : rebase #

Patch Set 15 : Add back renderThread's impl #

Patch Set 16 : rebase #

Total comments: 22

Patch Set 17 : rebase again #

Patch Set 18 : address Lei's comments and a bit more cleanup #

Patch Set 19 : remove a header #

Total comments: 2

Patch Set 20 : rebase #

Patch Set 21 : more rebase #

Patch Set 22 : re-rebase #

Patch Set 23 : rebase #

Patch Set 24 : rebase #

Patch Set 25 : rebase more #

Total comments: 25

Patch Set 26 : address dcheng's comments #

Patch Set 27 : rebase #

Patch Set 28 : remove service reg since it was added by service manager #

Patch Set 29 : fix nits #

Patch Set 30 : rebase #

Patch Set 31 : rebase #

Patch Set 32 : Update to use OnceCallback #

Total comments: 4

Patch Set 33 : address more comments #

Patch Set 34 : rebase #

Patch Set 35 : rebase #

Total comments: 1

Patch Set 36 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+674 lines, -49 lines) Patch
M chrome/app/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/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 35 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/browser_resources.grd 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 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_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 33 34 35 3 chunks +14 lines, -3 lines 0 comments Download
M chrome/utility/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/utility/DEPS View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 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 2 chunks +16 lines, -0 lines 0 comments Download
A components/printing/service/BUILD.gn View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
A components/printing/service/DEPS View 1 2 3 4 5 1 chunk +10 lines, -0 lines 0 comments Download
A components/printing/service/README.md View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -0 lines 0 comments Download
A components/printing/service/pdf_compositor_impl.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 +38 lines, -0 lines 0 comments Download
A components/printing/service/pdf_compositor_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 1 chunk +86 lines, -0 lines 0 comments Download
A components/printing/service/pdf_compositor_manifest.json View 1 chunk +15 lines, -0 lines 0 comments Download
A components/printing/service/pdf_compositor_service.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 1 chunk +54 lines, -0 lines 0 comments Download
A components/printing/service/pdf_compositor_service.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 +73 lines, -0 lines 0 comments Download
A components/printing/service/public/cpp/BUILD.gn View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
A components/printing/service/public/cpp/pdf_compositor_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 +38 lines, -0 lines 0 comments Download
A components/printing/service/public/cpp/pdf_compositor_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 +58 lines, -0 lines 0 comments Download
A components/printing/service/public/cpp/pdf_compositor_service_factory.h View 1 2 3 4 5 1 chunk +20 lines, -0 lines 0 comments Download
A components/printing/service/public/cpp/pdf_compositor_service_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +23 lines, -0 lines 0 comments Download
A components/printing/service/public/interfaces/BUILD.gn View 1 chunk +11 lines, -0 lines 0 comments Download
A components/printing/service/public/interfaces/OWNERS View 1 chunk +2 lines, -0 lines 0 comments Download
A components/printing/service/public/interfaces/pdf_compositor.mojom 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 1 chunk +16 lines, -0 lines 0 comments Download
M content/child/child_thread_impl.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 2 chunks +1 line, -1 line 0 comments Download
M content/public/app/mojo/content_browser_manifest.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 33 34 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/child/child_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +4 lines, -1 line 0 comments Download
M content/public/renderer/render_thread.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 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/render_thread_impl.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 34 35 2 chunks +3 lines, -1 line 0 comments Download
M content/renderer/render_thread_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 35 2 chunks +5 lines, -5 lines 0 comments Download
M printing/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 1 chunk +1 line, -0 lines 0 comments Download
A printing/common/BUILD.gn View 1 2 3 4 5 1 chunk +15 lines, -0 lines 0 comments Download
A printing/common/pdf_metafile_utils.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +28 lines, -0 lines 0 comments Download
A printing/common/pdf_metafile_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +46 lines, -0 lines 0 comments Download
M printing/pdf_metafile_skia.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -6 lines 0 comments Download
M printing/pdf_metafile_skia.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +1 line, -31 lines 0 comments Download

Messages

Total messages: 137 (100 generated)
Wei Li
Per our discussion, moved this service to components/printing/service. Pls review, thanks!
2 years, 2 months ago (2017-04-21 17:29:11 UTC) #12
Lei Zhang
Did we decide it was ok to put more printing bits in content/ ? https://codereview.chromium.org/2832633002/diff/100001/chrome/browser/BUILD.gn ...
2 years, 2 months ago (2017-04-22 00:12:26 UTC) #13
jam
https://codereview.chromium.org/2832633002/diff/100001/content/utility/utility_service_factory.cc File content/utility/utility_service_factory.cc (right): https://codereview.chromium.org/2832633002/diff/100001/content/utility/utility_service_factory.cc#newcode31 content/utility/utility_service_factory.cc:31: // nogncheck because dependency on //printing is conditional upon ...
2 years, 2 months ago (2017-04-24 15:18:05 UTC) #14
hal.canary
Skia-specific code in pdf_compositor_impl.cc looks good to me. https://codereview.chromium.org/2832633002/diff/100001/components/printing/service/pdf_compositor_impl.cc File components/printing/service/pdf_compositor_impl.cc (right): https://codereview.chromium.org/2832633002/diff/100001/components/printing/service/pdf_compositor_impl.cc#newcode21 components/printing/service/pdf_compositor_impl.cc:21: #include ...
2 years, 2 months ago (2017-04-24 19:13:17 UTC) #16
Wei Li
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.gn#newcode1529 chrome/browser/BUILD.gn:1529: "//components/printing/service/public/interfaces", On 2017/04/22 00:12:26, ...
2 years, 1 month ago (2017-04-27 05:34:17 UTC) #28
Wei Li
On 2017/04/22 00:12:26, Lei Zhang wrote: > Did we decide it was ok to put ...
2 years, 1 month ago (2017-04-27 05:36:37 UTC) #29
Lei Zhang
On 2017/04/27 05:36:37, Wei Li wrote: > On 2017/04/22 00:12:26, Lei Zhang wrote: > > ...
2 years, 1 month ago (2017-04-27 22:20:12 UTC) #30
Lei Zhang
https://codereview.chromium.org/2832633002/diff/100001/content/browser/utility_process_host_impl.cc File content/browser/utility_process_host_impl.cc (right): https://codereview.chromium.org/2832633002/diff/100001/content/browser/utility_process_host_impl.cc#newcode266 content/browser/utility_process_host_impl.cc:266: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { On 2017/04/27 05:34:16, Wei Li wrote: ...
2 years, 1 month ago (2017-04-27 22:42:22 UTC) #31
jam
https://codereview.chromium.org/2832633002/diff/260001/content/public/app/BUILD.gn File content/public/app/BUILD.gn (right): https://codereview.chromium.org/2832633002/diff/260001/content/public/app/BUILD.gn#newcode184 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/utility_process_host.h File content/public/browser/utility_process_host.h ...
2 years, 1 month ago (2017-04-28 15:49:10 UTC) #36
Wei Li
On 2017/04/27 22:20:12, Lei Zhang wrote: > On 2017/04/27 05:36:37, Wei Li wrote: > > ...
2 years, 1 month ago (2017-04-29 04:29:21 UTC) #46
Wei Li
ptal, thanks https://codereview.chromium.org/2832633002/diff/100001/content/browser/utility_process_host_impl.cc File content/browser/utility_process_host_impl.cc (right): https://codereview.chromium.org/2832633002/diff/100001/content/browser/utility_process_host_impl.cc#newcode266 content/browser/utility_process_host_impl.cc:266: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { On 2017/04/27 22:42:21, Lei ...
2 years, 1 month ago (2017-04-29 04:35:18 UTC) #47
jam
lgtm Ken: ptal at utility_process_host_impl.cc
2 years, 1 month ago (2017-05-01 15:42:14 UTC) #48
Ken Rockot(use gerrit already)
lgtm
2 years, 1 month ago (2017-05-01 15:54:45 UTC) #49
Wei Li
Lei, ptal, thanks
2 years, 1 month ago (2017-05-04 00:09:24 UTC) #50
Lei Zhang
I haven't finished looking yet, but this doesn't build at ToT anymore. Looks like ben@ ...
2 years, 1 month ago (2017-05-04 01:31:10 UTC) #51
Lei Zhang
https://codereview.chromium.org/2832633002/diff/360001/components/printing/service/README.md File components/printing/service/README.md (right): https://codereview.chromium.org/2832633002/diff/360001/components/printing/service/README.md#newcode1 components/printing/service/README.md:1: The pdf_compositor service composites multiple raw pictures from different ...
2 years, 1 month ago (2017-05-04 02:13:14 UTC) #52
Lei Zhang
On 2017/05/04 02:13:14, Lei Zhang wrote: > Can it leave elsewhere? Is it really tied ...
2 years, 1 month ago (2017-05-04 02:13:28 UTC) #53
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2832633002/diff/420001/content/browser/utility_process_host_impl.h File content/browser/utility_process_host_impl.h (right): https://codereview.chromium.org/2832633002/diff/420001/content/browser/utility_process_host_impl.h#newcode63 content/browser/utility_process_host_impl.h:63: void RegisterMojoServices() override; Agree with thestig@'s comments. This doesn't ...
2 years, 1 month ago (2017-05-04 17:27:18 UTC) #60
Wei Li
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.gn#newcode5 printing/common/BUILD.gn:5: source_set("common") { On 2017/05/04 01:31:09, ...
2 years, 1 month ago (2017-05-04 18:18:29 UTC) #63
Lei Zhang
lgtm
2 years, 1 month ago (2017-05-04 19:31:28 UTC) #64
Wei Li
nasko@ or dcheng@, can you have a security review on ipc changes? thanks
2 years, 1 month ago (2017-05-04 20:49:39 UTC) #66
dcheng
On 2017/05/04 20:49:39, Wei Li wrote: > nasko@ or dcheng@, can you have a security ...
2 years, 1 month ago (2017-05-05 18:28:11 UTC) #72
dcheng
https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_content_browser_client.cc#newcode3300 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_content_utility_client.cc File chrome/utility/chrome_content_utility_client.cc ...
2 years, 1 month ago (2017-05-10 07:36:48 UTC) #95
Wei Li
thanks, ptal https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2832633002/diff/580001/chrome/browser/chrome_content_browser_client.cc#newcode3300 chrome/browser/chrome_content_browser_client.cc:3300: services->insert( On 2017/05/10 07:36:47, dcheng wrote: > ...
2 years, 1 month ago (2017-05-11 16:53:02 UTC) #104
dcheng
https://codereview.chromium.org/2832633002/diff/580001/components/printing/service/pdf_compositor_service.cc File components/printing/service/pdf_compositor_service.cc (right): https://codereview.chromium.org/2832633002/diff/580001/components/printing/service/pdf_compositor_service.cc#newcode22 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: > ...
2 years, 1 month ago (2017-05-12 08:14:20 UTC) #105
Wei Li
On 2017/05/12 08:14:20, dcheng wrote: > https://codereview.chromium.org/2832633002/diff/580001/components/printing/service/pdf_compositor_service.cc > File components/printing/service/pdf_compositor_service.cc (right): > > https://codereview.chromium.org/2832633002/diff/580001/components/printing/service/pdf_compositor_service.cc#newcode22 > ...
2 years, 1 month ago (2017-05-12 15:28:47 UTC) #106
dcheng
Actually I thought about this a bit more, and I don't think there's any time ...
2 years, 1 month ago (2017-05-12 15:54:08 UTC) #107
dcheng
On 2017/05/12 15:54:08, dcheng wrote: > Actually I thought about this a bit more, and ...
2 years, 1 month ago (2017-05-12 15:54:29 UTC) #108
dcheng
On 2017/05/12 15:28:47, Wei Li wrote: > On 2017/05/12 08:14:20, dcheng wrote: > > > ...
2 years, 1 month ago (2017-05-12 15:55:44 UTC) #109
Wei Li
On 2017/05/12 15:55:44, dcheng (in AEST) wrote: > On 2017/05/12 15:28:47, Wei Li wrote: > ...
2 years, 1 month ago (2017-05-16 17:15:54 UTC) #114
dcheng
ipc lgtm but... For some background of why I'm not fully satisfied with the user ...
2 years, 1 month ago (2017-05-17 12:42:56 UTC) #115
hal.canary
On 2017/05/17 12:42:56, dcheng (in AEST) wrote: > For some background of why I'm not ...
2 years, 1 month ago (2017-05-17 13:05:46 UTC) #116
Wei Li
On 2017/05/17 12:42:56, dcheng (in AEST) wrote: > ipc lgtm but... > > For some ...
2 years, 1 month ago (2017-05-19 16:09:33 UTC) #125
Wei Li
https://codereview.chromium.org/2832633002/diff/720001/components/printing/service/pdf_compositor_impl.cc File components/printing/service/pdf_compositor_impl.cc (right): https://codereview.chromium.org/2832633002/diff/720001/components/printing/service/pdf_compositor_impl.cc#newcode44 components/printing/service/pdf_compositor_impl.cc:44: if (!shm->Map(memory_size) || shm->mapped_size() == 0) { On 2017/05/17 ...
2 years, 1 month ago (2017-05-19 16:10:01 UTC) #126
dcheng
Sorry for the review delays. LGTM and thanks for making the changes. https://codereview.chromium.org/2832633002/diff/780001/components/printing/service/pdf_compositor_impl.cc File components/printing/service/pdf_compositor_impl.cc ...
2 years, 1 month ago (2017-05-22 09:40:48 UTC) #127
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832633002/800001
2 years, 1 month ago (2017-05-22 18:57:33 UTC) #134
commit-bot: I haz the power
2 years, 1 month ago (2017-05-22 19:05:42 UTC) #137
Message was sent while issue was closed.
Committed patchset #36 (id:800001) as
https://chromium.googlesource.com/chromium/src/+/fabbf757ba1f0fe03a5671eb0bb4...

Powered by Google App Engine
This is Rietveld 408576698