Add error handling and unit test for pdf compositor service

Add error handling and unit test for pdf compositor service

Add a return status code to avoid aborting the service upon any error.
Mojo verifies the validity of returned handle, will exit it is not
valid.

Also add unit test for this service.

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

[Wei Li, 2017-06-01 22:19:36]

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

[commit-bot: I haz the power, 2017-06-01 22:20:14]

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-06-01 22:30:15]

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

[commit-bot: I haz the power, 2017-06-01 22:30:16]

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

[Wei Li, 2017-06-01 22:58:28]

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

[commit-bot: I haz the power, 2017-06-01 22:59:17]

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-06-02 00:55:22]

Description was changed from

==========
Handle error better and add unittest for pdf compositor service

Add a return status code to avoid aborting the service upon any error.
Mojo verifies the validity of returned handle, will exit it is not
valid.

Also add unit test for this service.

BUG=chromium:455764
==========

to

==========
Add error handling and unit test for pdf compositor service

Add a return status code to avoid aborting the service upon any error.
Mojo verifies the validity of returned handle, will exit it is not
valid.

Also add unit test for this service.

BUG=chromium:455764
==========

[commit-bot: I haz the power, 2017-06-02 01:00:19]

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

[commit-bot: I haz the power, 2017-06-02 01:00:20]

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

[Wei Li, 2017-06-02 18:15:14]

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

[commit-bot: I haz the power, 2017-06-02 18:15:36]

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-06-02 19:55:39]

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

[commit-bot: I haz the power, 2017-06-02 19:55:40]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-06-02 20:58:59]

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

[Wei Li, 2017-06-02 20:58:59]

Lei, pls review
Daniel, Pls do security review as I changed the .mojom file;
Ken, Pls review that my DEPS change on service_manager/public/c is ok

thanks

[Lei Zhang, 2017-06-02 21:54:09]

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
File components/printing/service/pdf_compositor_service_unittest.cc (right):

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:47:
run_loop_.reset(new base::RunLoop());
run_loop_ = base::MakeUnique<base::RunLoop>();

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:55:
base::SharedMemoryHandle LoadFileInSharedMemory(size_t* len) {
base::SharedMemoryHandle has a GetSize() method. Do we need |len| as an out
param?

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:61:
base::ReadFileToString(test_file, &content);
Do we want to return early if this fails? Same for Create() and Map() below.

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:99:
ASSERT_NE(10u, len);
Can we check for the expected size of |len| instead?

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
File components/printing/service/public/interfaces/pdf_compositor.mojom (right):

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/public/interfaces/pdf_compositor.mojom:16:
COMPOSTING_FAILURE,
type: COMPOSITING_FAILURE
style: kCompositingFailure

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
File components/printing/service/service_main.cc (right):

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/service_main.cc:18: void PrepareToStart() override;
Need a dtor that overrides.

[dcheng, 2017-06-05 18:40:56]

mojom changes lgtm

[Wei Li, 2017-07-07 19:57:51]

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

[commit-bot: I haz the power, 2017-07-07 19:58:04]

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-07 21:17:53]

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

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

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-07-07 21:23:57]

Sorry for the delay: planned to finish it while ooo, but didn't.

ptal, thanks.

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
File components/printing/service/pdf_compositor_service_unittest.cc (right):

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:47:
run_loop_.reset(new base::RunLoop());
On 2017/06/02 21:54:09, Lei Zhang wrote:
> run_loop_ = base::MakeUnique<base::RunLoop>();

Done.

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:55:
base::SharedMemoryHandle LoadFileInSharedMemory(size_t* len) {
On 2017/06/02 21:54:09, Lei Zhang wrote:
> base::SharedMemoryHandle has a GetSize() method. Do we need |len| as an out
> param?

Added a check to make sure these two are same so we can use either one.

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:61:
base::ReadFileToString(test_file, &content);
On 2017/06/02 21:54:09, Lei Zhang wrote:
> Do we want to return early if this fails? Same for Create() and Map() below.

Checks added.

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/pdf_compositor_service_unittest.cc:99:
ASSERT_NE(10u, len);
On 2017/06/02 21:54:09, Lei Zhang wrote:
> Can we check for the expected size of |len| instead?

The len is still file content size. But we truncated the shared memory to size
10 (smaller than needed) to simulate an erroneous buffer.

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
File components/printing/service/public/interfaces/pdf_compositor.mojom (right):

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/public/interfaces/pdf_compositor.mojom:16:
COMPOSTING_FAILURE,
On 2017/06/02 21:54:09, Lei Zhang wrote:
> type: COMPOSITING_FAILURE
> style: kCompositingFailure

Both styles are ok per style guide
(https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...)

But mojom files mostly use the former
(https://cs.chromium.org/chromium/src/ipc/ipc.mojom?l=14)

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
File components/printing/service/service_main.cc (right):

https://codereview.chromium.org/2919823004/diff/40001/components/printing/ser...
components/printing/service/service_main.cc:18: void PrepareToStart() override;
On 2017/06/02 21:54:09, Lei Zhang wrote:
> Need a dtor that overrides.

Done.

[Wei Li, 2017-07-17 19:51:16]

Patchset #5 (id:80001) has been deleted

[Wei Li, 2017-07-24 23:28:29]

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

[commit-bot: I haz the power, 2017-07-24 23:28:41]

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 01:56:38]

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

[commit-bot: I haz the power, 2017-07-25 01:56:39]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-07-25 04:56:29]

gentle ping

[Ken Rockot(use gerrit already), 2017-07-25 05:02:06]

Sorry for the delay, I haven't been looking at Rietveld for a while now. LGTM

https://codereview.chromium.org/2919823004/diff/100001/components/printing/se...
File components/printing/service/DEPS (right):

https://codereview.chromium.org/2919823004/diff/100001/components/printing/se...
components/printing/service/DEPS:8: "+services/service_manager/public/c",    #
For tests only.
rather than having this comment, you could move this entry out of include_rules
and into:

specific_include_rules: {
  "service_main.cc": [
    "+services/service_manager/public/c",
  ]
}

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

https://codereview.chromium.org/2919823004/diff/100001/components/printing/se...
components/printing/service/service_main.cc:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
nit: maybe test_service_main.cc would make more sense?

[Wei Li, 2017-07-25 21:44:39]

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

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

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-26 00:00:30]

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

[commit-bot: I haz the power, 2017-07-26 00:00:32]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-07-26 05:08:50]

thanks!

https://codereview.chromium.org/2919823004/diff/100001/components/printing/se...
File components/printing/service/DEPS (right):

https://codereview.chromium.org/2919823004/diff/100001/components/printing/se...
components/printing/service/DEPS:8: "+services/service_manager/public/c",    #
For tests only.
On 2017/07/25 05:02:06, Ken Rockot(use gerrit already) wrote:
> rather than having this comment, you could move this entry out of
include_rules
> and into:
> 
> specific_include_rules: {
>   "service_main.cc": [
>     "+services/service_manager/public/c",
>   ]
> }

Done.

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

https://codereview.chromium.org/2919823004/diff/100001/components/printing/se...
components/printing/service/service_main.cc:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/07/25 05:02:06, Ken Rockot(use gerrit already) wrote:
> nit: maybe test_service_main.cc would make more sense?

More meaningful, done.

[Lei Zhang, 2017-07-27 01:57:14]

https://codereview.chromium.org/2919823004/diff/140001/components/printing/se...
File components/printing/service/test_service_main.cc (right):

https://codereview.chromium.org/2919823004/diff/140001/components/printing/se...
components/printing/service/test_service_main.cc:24:
base::DiscardableMemoryAllocator::SetInstance(&mem_allocator);
Isn't this passing a pointer to an object that is about to get destroyed?

[Wei Li, 2017-07-27 19:03:49]

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

[commit-bot: I haz the power, 2017-07-27 19:04: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-07-27 19:04:02]

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

[commit-bot: I haz the power, 2017-07-27 19:04:04]

Dry run: CQ cannot get SignCLA result. Please try later.

[Wei Li, 2017-07-28 06:14:47]

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

[commit-bot: I haz the power, 2017-07-28 06:15: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-07-28 08:11:36]

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

[commit-bot: I haz the power, 2017-07-28 08:11:38]

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-07-28 23:43:03]

thanks!

https://codereview.chromium.org/2919823004/diff/140001/components/printing/se...
File components/printing/service/test_service_main.cc (right):

https://codereview.chromium.org/2919823004/diff/140001/components/printing/se...
components/printing/service/test_service_main.cc:24:
base::DiscardableMemoryAllocator::SetInstance(&mem_allocator);
On 2017/07/27 01:57:14, Lei Zhang wrote:
> Isn't this passing a pointer to an object that is about to get destroyed?

Done.

[Lei Zhang, 2017-07-29 00:26:53]

lgtm

[Wei Li, 2017-07-29 22:35:25]

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

[commit-bot: I haz the power, 2017-07-29 22:35:36]

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-29 23:55:44]

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

[commit-bot: I haz the power, 2017-07-29 23:55:45]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2017-07-30 01:44:12]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2017-07-30 01:44:13]

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

[commit-bot: I haz the power, 2017-07-30 01:44:24]

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-30 01:48:51]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1501379052553960,
"parent_rev": "f188c9f117764b05a42bd81a3305a433cb616b62", "commit_rev":
"04c36f14562dc46403c199ff9d9478101d9bc04a"}

[commit-bot: I haz the power, 2017-07-30 01:49:05]

Description was changed from

==========
Add error handling and unit test for pdf compositor service

Add a return status code to avoid aborting the service upon any error.
Mojo verifies the validity of returned handle, will exit it is not
valid.

Also add unit test for this service.

BUG=chromium:455764
==========

to

==========
Add error handling and unit test for pdf compositor service

Add a return status code to avoid aborting the service upon any error.
Mojo verifies the validity of returned handle, will exit it is not
valid.

Also add unit test for this service.

BUG=chromium:455764

Review-Url: https://codereview.chromium.org/2919823004
Cr-Commit-Position: refs/heads/master@{#490679}
Committed:
https://chromium.googlesource.com/chromium/src/+/04c36f14562dc46403c199ff9d94...
==========

[commit-bot: I haz the power, 2017-07-30 01:49:07]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/04c36f14562dc46403c199ff9d94...