pdf: Move PepperPDFHost into the pdf component.

Committed: https://crrev.com/a1733ee3921edbf62acd0c8128b4160852997761
Cr-Commit-Position: refs/heads/master@{#292416}

[sadrul, 2014-08-26 19:39:30]

sadrul@chromium.org changed reviewers:
+ raymes@chromium.org, thestig@chromium.org

[sadrul, 2014-08-26 19:39:31]

Hi! This is the follow-up patch from https://codereview.chromium.org/477263003/
to move PepperPDFHost into the pdf component.

[Lei Zhang, 2014-08-26 21:52:48]

Looking...

Can you add an OWNERS file, and also a README file to describe what this
component does? See components/*/README for examples.

[Lei Zhang, 2014-08-26 21:58:26]

https://codereview.chromium.org/505243003/diff/1/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/505243003/diff/1/chrome/renderer/DEPS#newcode15
chrome/renderer/DEPS:15: "+components/pdf/renderer",
Your CL diff is funny. Upstream chrome/renderer/DEPS doesn't have this.

https://codereview.chromium.org/505243003/diff/1/components/pdf.gypi
File components/pdf.gypi (right):

https://codereview.chromium.org/505243003/diff/1/components/pdf.gypi#newcode1
components/pdf.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
Again, this file doesn't exist on trunk.

https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
components/pdf/renderer/ppb_pdf_impl.cc:326:
PPB_PDF_Impl::InvokePrintingForInstance(instance);
Why do we bother making InvokePrintingForInstance() return a bool when we don't
actually care about the return results?

[Lei Zhang, 2014-08-26 22:00:49]

Just to confirm, this is intended to be the first CL in a series, right? In
which case, it needs to be diffable to trunk.

[sadrul, 2014-08-26 22:18:31]

On 2014/08/26 22:00:49, Lei Zhang wrote:
> Just to confirm, this is intended to be the first CL in a series, right? In
> which case, it needs to be diffable to trunk.

This is actually the second patch. The first one is:
https://codereview.chromium.org/477263003/

[raymes, 2014-08-27 00:50:01]

On 2014/08/26 21:58:26, Lei Zhang wrote:
> https://codereview.chromium.org/505243003/diff/1/chrome/renderer/DEPS
> File chrome/renderer/DEPS (right):
> 
>
https://codereview.chromium.org/505243003/diff/1/chrome/renderer/DEPS#newcode15
> chrome/renderer/DEPS:15: "+components/pdf/renderer",
> Your CL diff is funny. Upstream chrome/renderer/DEPS doesn't have this.
> 
> https://codereview.chromium.org/505243003/diff/1/components/pdf.gypi
> File components/pdf.gypi (right):
> 
> https://codereview.chromium.org/505243003/diff/1/components/pdf.gypi#newcode1
> components/pdf.gypi:1: # Copyright 2014 The Chromium Authors. All rights
> reserved.
> Again, this file doesn't exist on trunk.
> 
>
https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
> File components/pdf/renderer/ppb_pdf_impl.cc (right):
> 
>
https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
> components/pdf/renderer/ppb_pdf_impl.cc:326:
> PPB_PDF_Impl::InvokePrintingForInstance(instance);
> Why do we bother making InvokePrintingForInstance() return a bool when we
don't
> actually care about the return results?

I have the same question, but otherwise looks good

[sadrul, 2014-08-27 01:05:15]

https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
components/pdf/renderer/ppb_pdf_impl.cc:326:
PPB_PDF_Impl::InvokePrintingForInstance(instance);
On 2014/08/26 21:58:26, Lei Zhang wrote:
> Why do we bother making InvokePrintingForInstance() return a bool when we
don't
> actually care about the return results?

The return value is used in PepperPDFHost::OnHostMsgPrint() to determine whether
to return PP_OK or PP_ERROR_FAILED

[raymes, 2014-08-27 01:15:26]

Ah right, sorry I missed that. lgtm

On Wed, Aug 27, 2014 at 11:05 AM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
> File components/pdf/renderer/ppb_pdf_impl.cc (right):
>
>
https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
> components/pdf/renderer/ppb_pdf_impl.cc:326:
> PPB_PDF_Impl::InvokePrintingForInstance(instance);
> On 2014/08/26 21:58:26, Lei Zhang wrote:
>>
>> Why do we bother making InvokePrintingForInstance() return a bool when
>
> we don't
>>
>> actually care about the return results?
>
>
> The return value is used in PepperPDFHost::OnHostMsgPrint() to determine
> whether to return PP_OK or PP_ERROR_FAILED
>
> https://codereview.chromium.org/505243003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2014-08-27 01:16:21]

lgtm

[raymes, 2014-08-27 01:16:31]

Also, you may want to manually test that invoking printing from PDF
works correctly (in and out of process).

On Wed, Aug 27, 2014 at 11:14 AM, Raymes Khoury <raymes@chromium.org> wrote:
> Ah right, sorry I missed that. lgtm
>
> On Wed, Aug 27, 2014 at 11:05 AM,  <sadrul@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
>> File components/pdf/renderer/ppb_pdf_impl.cc (right):
>>
>>
https://codereview.chromium.org/505243003/diff/1/components/pdf/renderer/ppb_...
>> components/pdf/renderer/ppb_pdf_impl.cc:326:
>> PPB_PDF_Impl::InvokePrintingForInstance(instance);
>> On 2014/08/26 21:58:26, Lei Zhang wrote:
>>>
>>> Why do we bother making InvokePrintingForInstance() return a bool when
>>
>> we don't
>>>
>>> actually care about the return results?
>>
>>
>> The return value is used in PepperPDFHost::OnHostMsgPrint() to determine
>> whether to return PP_OK or PP_ERROR_FAILED
>>
>> https://codereview.chromium.org/505243003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-08-28 15:47:02]

The first patch has landed. This is now ready to land too.

On 2014/08/27 01:16:31, raymes wrote:
> Also, you may want to manually test that invoking printing from PDF
> works correctly (in and out of process).

I tried it out both with and without --out-of-process-pdf, seems to work OK
(caveat: with patch applied from crbug.com/408593, since printing seems broken
currently on ToT).

[sadrul, 2014-08-28 16:23:19]

sadrul@chromium.org changed reviewers:
+ senorblanco@chromium.org, sky@chromium.org

[sadrul, 2014-08-28 16:23:19]

+sky@ for added DEPS on ui/gfx/ from components/pdf/renderer

+senorblanco@ for added DEPS on skia/ext from components/pdf/renderer

[sky, 2014-08-28 16:31:00]

LGTM

[Stephen White, 2014-08-28 16:34:59]

LGTM (although I don't think my approval has any magical powers in this case).

[Lei Zhang, 2014-08-28 16:51:51]

lgtm

[sadrul, 2014-08-28 18:20:27]

Committed patchset #2 (id:20001) to pending queue manually as f255c6d (presubmit
successful).

[commit-bot: I haz the power, 2014-09-10 03:00:40]

Patchset 2 (id:??) landed as
https://crrev.com/a1733ee3921edbf62acd0c8128b4160852997761
Cr-Commit-Position: refs/heads/master@{#292416}