Initialize V8 in PDFium from external files (in-renderer process only).

Initialize V8 in PDFium from external files when plugin is running in the renderer process.

BUG=421063
Committed: https://crrev.com/f28960dabfe03eff699df0e00f7edf7023ae46e8
Cr-Commit-Position: refs/heads/master@{#304020}

[baixo1, 2014-11-04 18:04:06]

baixo@google.com changed reviewers:
+ andrewhayden@chromium.org, baixo@google.com, raymes@chromium.org

[baixo1, 2014-11-04 18:04:07]

This implementation works for the in-renderer case.

[rmcilroy, 2014-11-04 21:58:11]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org
- baixo@google.com

[rmcilroy, 2014-11-04 21:58:12]

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

https://codereview.chromium.org/705623002/diff/1/components/pdf/renderer/ppb_...
components/pdf/renderer/ppb_pdf_impl.cc:333: const char*
GetV8NativesData(PP_Instance instance_id) {
Random drive-by comment: Could we just have a single API call added which
returns all the necessary data in one go (e.g.,
GetV8ExternalSnapshotData(PP_Instance instance_id, char** natives_data_out, int*
natives_size_out, char** snapshot_data_out, int* snapshot_size_out)?

[raymes, 2014-11-04 22:18:13]

I took a first look, overall it looks like the right approach to me :)

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

https://codereview.chromium.org/705623002/diff/1/components/pdf/renderer/ppb_...
components/pdf/renderer/ppb_pdf_impl.cc:333: const char*
GetV8NativesData(PP_Instance instance_id) {
+1

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1416: return
gin::IsolateHolder::GetV8NativesData();
Can we get away with not modifying pepper_plugin_instance_impl.cc and instead
put all the implementation in ppb_pdf_impl.cc?

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc#newcode49
pdf/instance.cc:49: #if !defined(OS_ANDROID) &&
defined(V8_USE_EXTERNAL_STARTUP_DATA)
*You should be able to get rid of all the OS_ANDROID ifdefs everywhere - plugins
aren't built for android at all.
*What is the reason for having the V8_USE_EXTERNAL_STARTUP_DATA ifdef?

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc#newcode50
pdf/instance.cc:50: #include "v8/include/v8.h"
I just want to make sure - does pdfium use this same header (just making sure
we're using the same copy of v8 as the one used in pdfium)

[baixo1, 2014-11-07 21:35:33]

baixo@google.com changed reviewers:
+ baixo@google.com

[baixo1, 2014-11-07 21:35:34]

Addressed comments from Ross and Raymes.

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

https://codereview.chromium.org/705623002/diff/1/components/pdf/renderer/ppb_...
components/pdf/renderer/ppb_pdf_impl.cc:333: const char*
GetV8NativesData(PP_Instance instance_id) {
On 2014/11/04 21:58:12, rmcilroy wrote:
> Random drive-by comment: Could we just have a single API call added which
> returns all the necessary data in one go (e.g.,
> GetV8ExternalSnapshotData(PP_Instance instance_id, char** natives_data_out,
int*
> natives_size_out, char** snapshot_data_out, int* snapshot_size_out)?

Done.

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1416: return
gin::IsolateHolder::GetV8NativesData();
On 2014/11/04 22:18:13, raymes wrote:
> Can we get away with not modifying pepper_plugin_instance_impl.cc and instead
> put all the implementation in ppb_pdf_impl.cc?

I'm afraid not, Raymes.

Correct me if I'm wrong, but ppb_pdf_impl.cc seems to be the "inside the plugin"
part of the API and pepper_plugin_instance_impl.cc is the other side.


plugin -> ppb_pdf_impl  --|--> pepper_plugin_instance_impl -> gin
    INSIDE                |             OUTSIDE
                       interface

So, when we need the V8 data (this is provided by gin) from inside the plugin we
call ppb_pdf_impl.cc which, in turn, interfaces with
pepper_plugin_instance_impl.cc (the outside part) which calls gin to get the
data.

Putting "all the implementation" in ppb_pdf_impl.cc (which involves calling gin)
then mean we can access gin from inside the plugin, which then means we don't
need an API.

Am I missing anything?

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc#newcode49
pdf/instance.cc:49: #if !defined(OS_ANDROID) &&
defined(V8_USE_EXTERNAL_STARTUP_DATA)
On 2014/11/04 22:18:13, raymes wrote:
> *You should be able to get rid of all the OS_ANDROID ifdefs everywhere -
plugins
> aren't built for android at all.
Done.

> *What is the reason for having the V8_USE_EXTERNAL_STARTUP_DATA ifdef?
The feature is not enabled on all platforms yet (like Mac and Windows, for
instance).

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc#newcode50
pdf/instance.cc:50: #include "v8/include/v8.h"
On 2014/11/04 22:18:13, raymes wrote:
> I just want to make sure - does pdfium use this same header (just making sure
> we're using the same copy of v8 as the one used in pdfium)

According to Lei, this file is part of pdf.dll, which contains the V8 copy used
by pdfium (otherwise we shouldn't set V8's snapshot data here either).

[rmcilroy, 2014-11-07 23:46:01]

https://codereview.chromium.org/705623002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/705623002/diff/20001/build/common.gypi#newcod...
build/common.gypi:1020: ['android_webview_build==0 and
android_webview_telemetry_build==0 and chromecast==0 and (OS=="android" or
OS=="linux")', {
Please remove this change from this CL

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
components/pdf/renderer/ppb_pdf_impl.cc:370: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
I don't think you need the ifdef here - it's fine to have the API unconditinally
and only actually call the API if V8_USE_EXTERNAL_STARTUP_DATA I think (unless
Raymes disagrees).

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:47: #ifdef OS_ANDROID
Why does MapV8Files need changing here? Doesn't the old approach of checking
natives_fd work any longer?

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:108: #ifdef OS_ANDROID
Why ifdef OS_ANDROID here?  Ideally we should keep ifdefs to a minimum unless
there is a good reason for them.

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:118: #ifndef OS_ANDROID
why ifndef OS_ANDROID here?  I know we aren't calling it on android currently
but I don't see why it wouldn't work for Android?

[raymes, 2014-11-10 03:29:07]

Thanks for this!

Mainly nits but I think we still /should/ be able to keep the implementation
entirely inside ppb_pdf_impl.cc. AFAIK they should be built into the same dll.
ppb_pdf_impl.cc definitely isn't inside the plugin dll.

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1416: return
gin::IsolateHolder::GetV8NativesData();
No, that shouldn't be true. ppb_pdf_impl and pepper_plugin_instance_impl should
both be inside chrome.dll.

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/705623002/diff/1/pdf/instance.cc#newcode50
pdf/instance.cc:50: #include "v8/include/v8.h"
That sounds right to me. Thanks!

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
components/pdf/renderer/ppb_pdf_impl.cc:342: *natives_size_out =
*snapshot_size_out = 0;
nit: I don't know if this style is common. I'd rather the more verbose version
here with separate lines for each variable :)

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
components/pdf/renderer/ppb_pdf_impl.cc:370: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
+1 We can get rid of the #ifdef above too and bury the #ifdef inside the
implementation in the deepest place possible (either in
pepper_plugin_instance_impl.cc or in gin somewhere which would be my
preference).

https://codereview.chromium.org/705623002/diff/20001/content/public/renderer/...
File content/public/renderer/pepper_plugin_instance.h (right):

https://codereview.chromium.org/705623002/diff/20001/content/public/renderer/...
content/public/renderer/pepper_plugin_instance.h:121: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
nit: we can get rid of this ifdef

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/fake_pepper_plugin_instance.cc (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/fake_pepper_plugin_instance.cc:77: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
and this

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/fake_pepper_plugin_instance.h (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/fake_pepper_plugin_instance.h:44: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
nit: and this

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1444: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
You can move the #ifdef into the function and return error values if it isn't
defined, e.g.

#ifdef V8_USE_EXTERNAL_STARTUP_DATA
gin::IsolateHolder::GetV8ExternalSnapshotData(natives_data_out,
natives_size_out, snapshot_data_out, snapshot_size_out);
#else
*natives_data_out = ...;
...
#endif

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1449: int*
snapshot_size_out) const {
nit: the indentation looks wrong here. I think you just want 4 spaces from the
left.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.h:386: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
and this

https://codereview.chromium.org/705623002/diff/20001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/705623002/diff/20001/pdf/instance.cc#newcode326
pdf/instance.cc:326: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
It would be better not to use the #ifdef here. Instead you could base the
decision to use the snapshot data on the result of
pp::PDF::GetV8ExternalSnapshotData. If NULL is returned, then you can do
nothing.

https://codereview.chromium.org/705623002/diff/20001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/705623002/diff/20001/ppapi/c/private/ppb_pdf....
ppapi/c/private/ppb_pdf.h:173: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
and here

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.cc
File ppapi/cpp/private/pdf.cc (right):

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.cc...
ppapi/cpp/private/pdf.cc:200: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
same here

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.cc...
ppapi/cpp/private/pdf.cc:216: *natives_size_out = *snapshot_size_out = 0;
nit: same here please break up this line

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.h
File ppapi/cpp/private/pdf.h (right):

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.h#...
ppapi/cpp/private/pdf.h:66: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
and here

[baixo1, 2014-11-10 15:59:49]

Addressed the latest comments. Thanks Ross and Raymes!

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1416: return
gin::IsolateHolder::GetV8NativesData();
On 2014/11/10 03:29:05, raymes wrote:
> No, that shouldn't be true. ppb_pdf_impl and pepper_plugin_instance_impl
should
> both be inside chrome.dll.

If this is the case, then we can call gin from ppb_pdf_impl. So yes, we can put
the implementation there. We'd have to include a dependency on gin in
components/pdf though -- is it ok?

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
components/pdf/renderer/ppb_pdf_impl.cc:342: *natives_size_out =
*snapshot_size_out = 0;
On 2014/11/10 03:29:06, raymes wrote:
> nit: I don't know if this style is common. I'd rather the more verbose version
> here with separate lines for each variable :)

Done.

https://codereview.chromium.org/705623002/diff/20001/components/pdf/renderer/...
components/pdf/renderer/ppb_pdf_impl.cc:370: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> +1 We can get rid of the #ifdef above too and bury the #ifdef inside the
> implementation in the deepest place possible (either in
> pepper_plugin_instance_impl.cc or in gin somewhere which would be my
> preference).

Removed both this and the other one above (function definition).

https://codereview.chromium.org/705623002/diff/20001/content/public/renderer/...
File content/public/renderer/pepper_plugin_instance.h (right):

https://codereview.chromium.org/705623002/diff/20001/content/public/renderer/...
content/public/renderer/pepper_plugin_instance.h:121: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> nit: we can get rid of this ifdef

Done.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/fake_pepper_plugin_instance.cc (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/fake_pepper_plugin_instance.cc:77: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> and this

Done.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/fake_pepper_plugin_instance.h (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/fake_pepper_plugin_instance.h:44: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> nit: and this

Done.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1444: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> You can move the #ifdef into the function and return error values if it isn't
> defined, e.g.
> 
> #ifdef V8_USE_EXTERNAL_STARTUP_DATA
> gin::IsolateHolder::GetV8ExternalSnapshotData(natives_data_out,
> natives_size_out, snapshot_data_out, snapshot_size_out);
> #else
> *natives_data_out = ...;
> ...
> #endif

Done.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1449: int*
snapshot_size_out) const {
On 2014/11/10 03:29:06, raymes wrote:
> nit: the indentation looks wrong here. I think you just want 4 spaces from the
> left.

Done.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.h:386: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> and this

Done.

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:47: #ifdef OS_ANDROID
On 2014/11/07 23:46:00, rmcilroy wrote:
> Why does MapV8Files need changing here? Doesn't the old approach of checking
> natives_fd work any longer?

Yes, it does work. I undid the changes.

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:108: #ifdef OS_ANDROID
On 2014/11/07 23:46:01, rmcilroy wrote:
> Why ifdef OS_ANDROID here?  Ideally we should keep ifdefs to a minimum unless
> there is a good reason for them.

Done.

https://codereview.chromium.org/705623002/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:118: #ifndef OS_ANDROID
On 2014/11/07 23:46:01, rmcilroy wrote:
> why ifndef OS_ANDROID here?  I know we aren't calling it on android currently
> but I don't see why it wouldn't work for Android?

Done.

https://codereview.chromium.org/705623002/diff/20001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/705623002/diff/20001/pdf/instance.cc#newcode326
pdf/instance.cc:326: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> It would be better not to use the #ifdef here. Instead you could base the
> decision to use the snapshot data on the result of
> pp::PDF::GetV8ExternalSnapshotData. If NULL is returned, then you can do
> nothing.

Done.

https://codereview.chromium.org/705623002/diff/20001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/705623002/diff/20001/ppapi/c/private/ppb_pdf....
ppapi/c/private/ppb_pdf.h:173: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> and here

Done.

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.cc
File ppapi/cpp/private/pdf.cc (right):

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.cc...
ppapi/cpp/private/pdf.cc:200: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:06, raymes wrote:
> same here

Done.

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.cc...
ppapi/cpp/private/pdf.cc:216: *natives_size_out = *snapshot_size_out = 0;
On 2014/11/10 03:29:07, raymes wrote:
> nit: same here please break up this line

Done.

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.h
File ppapi/cpp/private/pdf.h (right):

https://codereview.chromium.org/705623002/diff/20001/ppapi/cpp/private/pdf.h#...
ppapi/cpp/private/pdf.h:66: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 03:29:07, raymes wrote:
> and here

Done.

[baixo1, 2014-11-10 17:16:49]

Bury ifdefs in gin.

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1444: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/10 15:59:48, baixo1 wrote:
> On 2014/11/10 03:29:06, raymes wrote:
> > You can move the #ifdef into the function and return error values if it
isn't
> > defined, e.g.
> > 
> > #ifdef V8_USE_EXTERNAL_STARTUP_DATA
> > gin::IsolateHolder::GetV8ExternalSnapshotData(natives_data_out,
> > natives_size_out, snapshot_data_out, snapshot_size_out);
> > #else
> > *natives_data_out = ...;
> > ...
> > #endif
> 
> Done.

Better yet, I made gin handle this :)

[raymes, 2014-11-11 02:40:20]

Looks good, but let's make the change to move the implementation to
ppb_pdf_impl.cc This will save us a bunch of plumbing/boilerplate.

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/705623002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1416: return
gin::IsolateHolder::GetV8NativesData();
Yep that should be fine. Thanks!

[baixo1, 2014-11-11 13:58:03]

baixo@google.com changed reviewers:
+ jochen@chromium.org

[baixo1, 2014-11-11 13:58:03]

+jochen@

Moved implementation to ppb_pdf_impl.cc

Removed pieces that don't belong to this CL.

[raymes, 2014-11-11 22:56:51]

lgtm

https://codereview.chromium.org/705623002/diff/100001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/705623002/diff/100001/ppapi/c/private/ppb_pdf...
ppapi/c/private/ppb_pdf.h:173: void (*GetV8ExternalSnapshotData)(const char**
natives_data_out,
nit: Please add a comment about what this is for

[baixo1, 2014-11-12 13:33:57]

baixo@google.com changed reviewers:
+ dcarney@chromium.org

[baixo1, 2014-11-12 13:33:58]

dcarney@: could you please take a look at gin/ ?

https://codereview.chromium.org/705623002/diff/100001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/705623002/diff/100001/ppapi/c/private/ppb_pdf...
ppapi/c/private/ppb_pdf.h:173: void (*GetV8ExternalSnapshotData)(const char**
natives_data_out,
On 2014/11/11 22:56:51, raymes wrote:
> nit: Please add a comment about what this is for

Done.

[dcarney, 2014-11-12 13:40:37]

On 2014/11/12 13:33:58, baixo1 wrote:
> dcarney@: could you please take a look at gin/ ?

lgtm

[jochen (gone - plz use gerrit), 2014-11-12 15:01:54]

lgtm with nits

https://codereview.chromium.org/705623002/diff/120001/components/pdf/renderer...
File components/pdf/renderer/DEPS (right):

https://codereview.chromium.org/705623002/diff/120001/components/pdf/renderer...
components/pdf/renderer/DEPS:3: "+gin",
plz update the pdf.gypi and BUILD.gn files to depend on gin

[baixo1, 2014-11-12 18:05:22]

Addressed comments from Jochen and Raymes.

https://codereview.chromium.org/705623002/diff/120001/components/pdf/renderer...
File components/pdf/renderer/DEPS (right):

https://codereview.chromium.org/705623002/diff/120001/components/pdf/renderer...
components/pdf/renderer/DEPS:3: "+gin",
On 2014/11/12 15:01:53, jochen (slow) wrote:
> plz update the pdf.gypi and BUILD.gn files to depend on gin

Done.

[baixo1, 2014-11-12 18:10:24]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-11-12 18:11:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/705623002/140001

[commit-bot: I haz the power, 2014-11-12 19:29:51]

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

[commit-bot: I haz the power, 2014-11-12 19:29:52]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[baixo1, 2014-11-13 11:46:07]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-11-13 11:46:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/705623002/160001

[commit-bot: I haz the power, 2014-11-13 12:41:12]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-11-13 12:42:27]

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