Implement the host side of the PPB_PDF proxy.

Implement the host side of the PPB_PDF proxy.

TBR=brettw@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192669

[yzshen1, 2013-04-03 18:32:39]

Only a few nits.

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_pdf_host.cc (right):

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.cc:158:
static_cast<PP_ResourceString>(string_id);
If we pass |string_id| as int to be 'safe', then we should check the value
before casting it back to an enum value.

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.cc:368: int32_t format,
Why not making it PP_ImageDataFormat directly?

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_pdf_host.h (right):

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.h:56: std::string action);
const &?

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1766: // message as |fd|. This is due to the
unfortunate way that ImageHandles are
Does it make sense to have a TODO to make SerializedHandle also works with
ImageHandle?

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:525: // fails to return a handle to the
plugin?
The resource is passed to the plugin side as HostResource |result|. As long as
we create a plugin-side resource object using this HostResource, it is managed.
It seems even if the code fails to return a handle, it is okay. But if
|image_data_desc| has wrong size, it will leak.

[raymes, 2013-04-03 23:18:20]

+jschuh for message handlers.

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_pdf_host.cc (right):

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.cc:158:
static_cast<PP_ResourceString>(string_id);
I just changed it to send the enum value.

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.cc:368: int32_t format,
On 2013/04/03 18:32:39, yzshen1 wrote:
> Why not making it PP_ImageDataFormat directly?

Done.

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_pdf_host.h (right):

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.h:56: std::string action);
On 2013/04/03 18:32:39, yzshen1 wrote:
> const &?

Done.

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1766: // message as |fd|. This is due to the
unfortunate way that ImageHandles are
Done. I put it in serialized_structs.h

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/13004012/diff/39001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:525: // fails to return a handle to the
plugin?
ok I removed the todo

[yzshen1, 2013-04-04 01:30:07]

LGTM with one comment.

Thanks!

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
File chrome/renderer/pepper/pepper_pdf_host.cc (right):

https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
chrome/renderer/pepper/pepper_pdf_host.cc:158:
static_cast<PP_ResourceString>(string_id);
There was a discussion about passing enum values over IPC is not safe, though.
Because the IPC layer doesn't verify that the integer received is a valid enum
value before casting it.

Justin: could you please give suggestion about the best practice?

On 2013/04/03 23:18:21, raymes wrote:
> I just changed it to send the enum value.

[jschuh, 2013-04-04 17:05:43]

On 2013/04/04 01:30:07, yzshen1 wrote:
> LGTM with one comment.
> 
> Thanks!
> 
>
https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
> File chrome/renderer/pepper/pepper_pdf_host.cc (right):
> 
>
https://codereview.chromium.org/13004012/diff/39001/chrome/renderer/pepper/pe...
> chrome/renderer/pepper/pepper_pdf_host.cc:158:
> static_cast<PP_ResourceString>(string_id);
> There was a discussion about passing enum values over IPC is not safe, though.
> Because the IPC layer doesn't verify that the integer received is a valid enum
> value before casting it.
> 
> Justin: could you please give suggestion about the best practice?
> 
> On 2013/04/03 23:18:21, raymes wrote:
> > I just changed it to send the enum value.

You just need to verify that the enum value is valid before using it. We're
still going back and forth on how to approach that in a more standard way.

[raymes, 2013-04-04 17:07:04]

Thanks Justin. Let me know if you have any comments on the message handlers.

[jschuh, 2013-04-04 18:25:13]

lgtm on message handling. thanks for remembering.

[commit-bot: I haz the power, 2013-04-05 15:49:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/13004012/55001

[commit-bot: I haz the power, 2013-04-05 15:49:57]

Presubmit check for 13004012-55001 failed and returned exit status 1.

INFO:root:Found 13 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome_renderer.gypi

Presubmit checks took 1.4s to calculate.

[raymes, 2013-04-05 15:53:45]

+brettw for chrome_renderer.gypi

[raymes, 2013-04-05 17:41:17]

TBR=brettw for chrome_renderer.gypi

[commit-bot: I haz the power, 2013-04-05 17:41:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/13004012/55001

[commit-bot: I haz the power, 2013-04-05 19:48:08]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-05 20:15:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/13004012/55001

[brettw, 2013-04-05 21:12:51]

.gypi lgtm

[commit-bot: I haz the power, 2013-04-06 00:33:15]

Change committed as 192669